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

[Li, Meng]

> -----Original Message-----
> From: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Sent: Tuesday, October 24, 2023 3:12 AM
> To: Li, Meng <Meng.Li@xxxxxxxxxxxxx>
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; usb-
> storage@xxxxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] usb: storage: add shutdown function for usb storage
> driver
> 
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and
> know the content is safe.
> 
> 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?
> 

Understand why you ask me to clarify the time of reporting error.
Only when the reboot action is taking place, there is error reported, and then system reset normally, and there is no error in the boot log of the next time.
So, I am not sure whether it is meaningful and worth to fix this issue or not.


> 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?
> 

I add an WARN_ON() in USB core code
Call trace as below:
 hub_port_init+0xae0/0xcf0
 usb_reset_and_verify_device+0xe8/0x3e4
 usb_reset_device+0x118/0x24c
 usb_stor_port_reset+0x70/0x80
 usb_stor_invoke_transport+0x234/0x530
 usb_stor_transparent_scsi_command+0x18/0x24
 usb_stor_control_thread+0x158/0x25c
 kthread+0x120/0x124
 ret_from_fork+0x10/0x20

> > 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.
> 

As my above said, I understand what you mean. I also thought over what you said.
I am not sure, but I still sent patch to upstream community, and want to get some suggest from more authoritative maintainer.

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

There is also error report even if I use command "reboot"

> >  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?
> 

After adding the mass storage shutdown function usb_stor_shutdown(), it will release resource with bellow call logic.
usb_stor_shutdown()->usb_stor_disconnect->usb_stor_release_resources()
in the usb_stor_release_resources(), usb_stor_control_thread thread() is stopped, and there will no control data transferring.

> > 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.
> 
It looks like a little uncomfortably indeed.

> 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...
> 

I add the shutdown callback refer to uas driver that has the similar shutdown function.
About the usb serial driver, there has been serial_port_shutdown() function at a more reasonable location.
I am able to test all the cases with PCIe to USB card, so I am not sure whether there is also the same issue with other drivers.

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

Maybe. But this issue is caused by usb_stor_control_thread() thread that is in the use mass storage driver.
So, I would like to fixed it only in use mass storage driver.
Based on my current understanding of usb code, I don't know whether there is a unified usb core interface that can fix this issue of all the usb driver.
I don't have ability to touch use core code that has so widespread influence.

Thanks,
LImeng

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