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

[gregkh@xxxxxxxxxxxxxxxxxxx]

On Tue, Oct 24, 2023 at 11:58:37AM -0400, Alan Stern wrote:
> On Tue, Oct 24, 2023 at 05:45:40PM +0200, gregkh@xxxxxxxxxxxxxxxxxxx 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.

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?

> > reboot is a system thing that happens before the reboot syscall happens.
> > So which are we talking nabout here?
> > 
> > > Greg, do you know?  Should we take the time to disconnect all the USB 
> > > devices during a system shutdown?
> > 
> > In the past we have not.  And if we switch to do so, we might get some
> > complaints as we would now delaying the shutdown process to be longer
> > than before.
> 
> Yes, that's what I'm afraid of.
> 
> > > 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.

thanks,

greg k-h