> -----Original Message----- > From: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > Sent: Wednesday, October 25, 2023 3:23 AM > To: gregkh@xxxxxxxxxxxxxxxxxxx > Cc: Li, Meng <Meng.Li@xxxxxxxxxxxxx>; 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 Tue, Oct 24, 2023 at 07:11:31PM +0200, gregkh@xxxxxxxxxxxxxxxxxxx > 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@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. > > 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
