Message-ID: <CO1PR11MB5188998FE62B30C3CA83A6F8F1DEA@CO1PR11MB5188.namprd11.prod.outlook.com>
Date:   Wed, 25 Oct 2023 02:25:17 +0000
From:   "Li, Meng" <Meng.Li@...driver.com>
To:     Alan Stern <stern@...land.harvard.edu>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:     "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "usb-storage@...ts.one-eyed-alien.net" 
        <usb-storage@...ts.one-eyed-alien.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] usb: storage: add shutdown function for usb storage
 driver



> -----Original Message-----
> From: Alan Stern <stern@...land.harvard.edu>
> Sent: Wednesday, October 25, 2023 3:23 AM
> To: gregkh@...uxfoundation.org
> Cc: Li, Meng <Meng.Li@...driver.com>; linux-usb@...r.kernel.org; usb-
> storage@...ts.one-eyed-alien.net; linux-kernel@...r.kernel.org
> 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 Tue, Oct 24, 2023 at 07:11:31PM +0200, gregkh@...uxfoundation.org
> wrote:
> > On Tue, Oct 24, 2023 at 11:58:37AM -0400, Alan Stern wrote:
> > > On Tue, Oct 24, 2023 at 05:45:40PM +0200, gregkh@...uxfoundation.org
> wrote:
> > > > On Tue, Oct 24, 2023 at 11:35:19AM -0400, Alan Stern wrote:
> > > > > Okay, that's a different matter.  In fact, I don't know what is
> > > > > supposed to happen during a clean reboot.
> > > >
> > > > Define "clean" :)
> > >
> > > In this case, I mean what happens when you give the "reboot" command.
> >
> > That's a userspace binary/script/whatever that can do loads of
> > different things not involving the kernel, so it all depends on the
> > user's system as to what will happen here.
> >
> > Many "good" userspace implementation of reboot will go and sync and
> > unmount all mounted disks in the correct order, before the kernel is
> > told to reboot.
> 
> Even if the filesystems are unmounted, the kernel will still probe the drive
> periodically (once every few seconds) if it claims to have removable media.
> Failure of those probes won't hurt anything, but it is likely to generate an error
> message.  I don't know if that's what's happening in this case, though.
> 
> > All we can do in the kernel is act on the reboot system call.
> >
> > So perhaps the original poster here can see why his userspace isn't
> > correctly shutting down their storage devices?
> 
> Meng, can you do this?  Maybe you can fix the problem by adding a script to
> be executed by the "reboot" command.  If the script writes to the "remove"
> attribute file in the drive's sysfs directory, that will unbind usb-storage from
> the device.  It should give the same result as your patch, for clean reboots.  It
> won't help "reboot -f", though.
> 

Ok! Got it.
In fact, the reported error is not critical one. In my real work environment, there is not the error because there is not PCIe-TO-USB card installed on board.
The issue is reported to me from our tester who used the PCIe-TO-USB card to test the PCIe feature, not test USB feature.
I am ok to NAK this patch. The primary intention of sending this patch is to raise up the discussion about whether shutdown function should be added for usb mass storage driver, and I have got what I want.

Thanks,
Limeng

> > > > > What happens with non-USB disk drives?  Or other removable devices?
> > > >
> > > > It would have to come from "above" in the device tree, so does the
> > > > PCI or platform bus say that they should be shut down and their
> > > > child devices?
> > >
> > > Well, the PCI layer invokes the HCD's ->shutdown callback.  But the
> > > usb-storage driver and usbcore don't know this has happened, so they
> > > start logging errors because they are suddenly unable to communicate
> > > with a USB drive.  Meng Li is unhappy about these error messages.
> > >
> > > Adding a shutdown callback of sorts to usb-storage allows the driver
> > > to know that it shouldn't communicate with the drive any more, which
> > > prevents the error message from appearing.  That's what this patch does.
> > >
> > > But that's all it does.  Basically it creates a layering violation
> > > just to prevent some error messages from showing up in the system
> > > log during a shutdown or reboot.  The question is whether we want to
> > > do this at all, and if we do, shouldn't it be handled at the usbcore
> > > level rather than just within usb-storage?
> >
> > We should do this within the usb core if we care about it, but why did
> > the USB device suddenly go away before the USB storage driver was told
> > about it?  That feels like something else is pulling the power on the
> > device that is out-of-band here.
> 
> The device went away because the HCD shut down the host controller,
> thereby stopping all USB communication.  The usb-storage driver wasn't
> informed because this all happened inside the HCD's PCI ->shutdown callback.
> HCD shutdown doesn't do anything to the USB bus -- in particular, it doesn't
> remove the root hub or anything else -- it just turns off the host controller.
> 
> Since USB class-device drivers don't have ->shutdown callbacks (there is no
> shutdown() method in struct usb_driver), they don't know what's going on
> while a shutdown or reboot is in progress.  All they see is a bunch of errors.
> 
> Alan Stern

Powered by blists - more mailing lists