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
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. Therefore, add shutdown function
used to disconnect the usb mass storage device to avoid data transferring under
the stopped status of PCIe device.
Signed-off-by: Meng Li <[email protected]>
---
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,
.id_table = usb_storage_usb_ids,
.supports_autosuspend = 1,
.soft_unbind = 1,
--
2.34.1
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 <[email protected]>
> ---
> 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
> -----Original Message-----
> From: Alan Stern <[email protected]>
> Sent: Tuesday, October 24, 2023 3:12 AM
> To: Li, Meng <[email protected]>
> Cc: [email protected]; [email protected]; usb-
> [email protected]; [email protected]
> 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 <[email protected]>
> > ---
> > 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
On Tue, Oct 24, 2023 at 03:43:56AM +0000, Li, Meng wrote:
>
>
> > -----Original Message-----
> > From: Alan Stern <[email protected]>
> > 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
> > > 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"
Okay, that's a different matter. In fact, I don't know what is supposed
to happen during a clean reboot.
Greg, do you know? Should we take the time to disconnect all the USB
devices during a system shutdown? What happens with non-USB disk
drives? Or other removable devices?
Alan Stern
On Tue, Oct 24, 2023 at 11:35:19AM -0400, Alan Stern wrote:
> On Tue, Oct 24, 2023 at 03:43:56AM +0000, Li, Meng wrote:
> >
> >
> > > -----Original Message-----
> > > From: Alan Stern <[email protected]>
>
> > > 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
>
> > > > 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"
>
> Okay, that's a different matter. In fact, I don't know what is supposed
> to happen during a clean reboot.
Define "clean" :)
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.
> 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?
thanks,
greg k-h
On Tue, Oct 24, 2023 at 05:45:40PM +0200, [email protected] 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.
> 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?
Alan Stern
On Tue, Oct 24, 2023 at 11:58:37AM -0400, Alan Stern wrote:
> On Tue, Oct 24, 2023 at 05:45:40PM +0200, [email protected] 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
On Tue, Oct 24, 2023 at 07:11:31PM +0200, [email protected] wrote:
> On Tue, Oct 24, 2023 at 11:58:37AM -0400, Alan Stern wrote:
> > On Tue, Oct 24, 2023 at 05:45:40PM +0200, [email protected] 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.
> > > > 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
> -----Original Message-----
> From: Alan Stern <[email protected]>
> Sent: Wednesday, October 25, 2023 3:23 AM
> To: [email protected]
> Cc: Li, Meng <[email protected]>; [email protected]; usb-
> [email protected]; [email protected]
> 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, [email protected]
> wrote:
> > On Tue, Oct 24, 2023 at 11:58:37AM -0400, Alan Stern wrote:
> > > On Tue, Oct 24, 2023 at 05:45:40PM +0200, [email protected]
> 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
On 24.10.23 21:23, Alan Stern wrote:
> 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.
Does this solve the issue? You'd have to flush the cache on the SCSI
device further down in the tree, if you want this done properly.
Regards
Oliver
On Wed, Oct 25, 2023 at 02:25:17AM +0000, Li, Meng wrote:
> 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.
All right, then we will consider the issue closed. Thanks.
Alan Stern
On Wed, Oct 25, 2023 at 11:07:07AM +0200, Oliver Neukum wrote:
> On 24.10.23 21:23, Alan Stern wrote:
> > 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.
>
> Does this solve the issue? You'd have to flush the cache on the SCSI
> device further down in the tree, if you want this done properly.
Depends on what issue you're talking about. The purpose of the proposed
patch was not to make sure that the reboot happens cleanly; it was to
prevent certain error messages from showing up in the system log.
Besides, Meng Li has decided to withdraw the patch submission.
Alan Stern