Re: [PATCH] usb: storage: add shutdown function for usb storage driver

[Alan Stern]

On Mon, Oct 23, 2023 at 01:41:11PM +0800, Meng Li wrote:
> On ls1043/ls1046 rdb platform, if a PCIe-USB host controller is installed, and
> an USB disk is also installed on the PCIe card, when executing "reboot -f" to
> reset the board, there will be below error reported:
> usb 2-2: device not accepting address 2, error -108

Do you mean that this error occurs after the system has rebooted?  Or do 
you mean that the error is reported while the reboot is taking place?

That "device not accepting address" error message is generated by the 
USB core, not by the usb-storage driver.  How will changing usb-storage 
help fix the problem?

> This issue is introduced by linux-yocto commit 837547b64a34("driver: net:
> dpaa: release resource when executing kexec") that cause to spend more
> time on shutdown operation. So, the 2 platforms with DPAA are not reset
> immediately after executing force reboot command. Moreover, the usb-storage
> thread is still in active status, there is still control data transferred between
> USB disk and PCIe host controller. But now the shutdown callback of usb pci driver
> had been invoked to stop the PCIe host controller completely. In this situation,
> the data transferring failed and report error.

That's _supposed_ to happen.  By design, the "reboot -f" command is 
meant to carry out an immediate reboot, without using the init system, 
unmounting filesystems, or doing other cleanup operations.

If you want a clean reboot with no errors, don't use the "-f" option.

>  Therefore, add shutdown function
> used to disconnect the usb mass storage device to avoid data transferring under
> the stopped status of PCIe device.

I don't see how this will fix the problems associated with a forced 
reboot.  How is preventing data from being transferred any better than 
getting an error when you do try to transfer it?

> Signed-off-by: Meng Li <Meng.Li@xxxxxxxxxxxxx>
> ---
>  drivers/usb/storage/usb.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
> index ed7c6ad96a74..e076d7e3784f 100644
> --- a/drivers/usb/storage/usb.c
> +++ b/drivers/usb/storage/usb.c
> @@ -1142,6 +1142,15 @@ static int storage_probe(struct usb_interface *intf,
>  	return result;
>  }
>  
> +static void usb_stor_shutdown(struct device *dev)
> +{
> +	struct usb_driver *driver = to_usb_driver(dev->driver);
> +	struct usb_interface *intf = to_usb_interface(dev);
> +
> +	if (driver->disconnect)
> +		driver->disconnect(intf);
> +}
> +
>  static struct usb_driver usb_storage_driver = {
>  	.name =		DRV_NAME,
>  	.probe =	storage_probe,
> @@ -1151,6 +1160,7 @@ static struct usb_driver usb_storage_driver = {
>  	.reset_resume =	usb_stor_reset_resume,
>  	.pre_reset =	usb_stor_pre_reset,
>  	.post_reset =	usb_stor_post_reset,
> +	.drvwrap.driver.shutdown = usb_stor_shutdown,

This definitely looks like a layering violation.  If devices are to be 
disconnected during a system shutdown, the USB core should take care of 
it.  Not the individual device drivers.

What will happen if you make this change to usb-storage?  In a little 
while you'll want to do the same thing to the uas driver.  And then the 
usbhid driver.  And the usb serial drivers.  And so on...

This does not seem like the best solution to whatever problem you want 
to solve.

>  	.id_table =	usb_storage_usb_ids,
>  	.supports_autosuspend = 1,
>  	.soft_unbind =	1,

Alan Stern