| Message ID | 20241022090905.9806-1-huanglei814@163.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] usb: core: adds support for PM control of specific USB dev skip suspend. | expand |
On Tue, Oct 22, 2024 at 05:09:05PM +0800, huanglei814 wrote: > From: huanglei <huanglei@kylinos.cn> > > All USB devices are brought into suspend power state after system suspend. > It is desirable for some specific manufacturers buses to keep their devices > in normal state even after system suspend. > > v2: Change to bool type for skip_suspend. > v3: Rebase and update commit message. As per the documentation, this needs to go below the --- line. Also, you changed more than just the bool type in v2 :( Please fix up and do a v4 after waiting a while for others to review this. thanks, greg k-h
On 22/10/2024 11:09, huanglei814 wrote: > +#ifdef CONFIG_USB_SKIP_SUSPEND > + if (device_property_read_bool(tmpdev, "usb-skip-suspend")) { You are adding this for DTS, so you need bindings and describe hardware characteristic, not OS/driver policy. Best regards, Krzysztof
On Tue, Oct 22, 2024 at 05:09:05PM +0800, huanglei814 wrote: > From: huanglei <huanglei@kylinos.cn> > > All USB devices are brought into suspend power state after system suspend. > It is desirable for some specific manufacturers buses to keep their devices > in normal state even after system suspend. Why is it desirable for devices on these buses to remain at full power during system suspend? What about wakeup requests? If the device isn't suspended, it won't be able to send a wakeup request if it needs to tell the system to return to full power. What about runtime suspend? Are the devices on these buses supposed to remain at full power all the time, or only during system suspend? > v2: Change to bool type for skip_suspend. > v3: Rebase and update commit message. > > Signed-off-by: huanglei <huanglei@kylinos.cn> > --- > drivers/usb/core/Kconfig | 11 +++++++++++ > drivers/usb/core/driver.c | 14 ++++++++++++++ > drivers/usb/host/xhci-plat.c | 7 +++++++ > include/linux/usb.h | 9 +++++++++ > 4 files changed, 41 insertions(+) > > diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig > index 58e3ca7e4793..69778aa7b913 100644 > --- a/drivers/usb/core/Kconfig > +++ b/drivers/usb/core/Kconfig > @@ -143,3 +143,14 @@ config USB_DEFAULT_AUTHORIZATION_MODE > ACPI selecting value 2 is analogous to selecting value 0. > > If unsure, keep the default value. > + > +config USB_SKIP_SUSPEND > + bool "Vendor USB support skip suspend" > + depends on USB > + help > + Select this the associate USB devices will skip suspend when pm control. > + > + This option adds support skip suspend for PM control of USB devices > + in specific manufacturers platforms. > + > + If unsure, keep the default value. Why does this need to be a Kconfig option? Why can't it be enabled all the time? > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c > index 0c3f12daac79..05fe921f8297 100644 > --- a/drivers/usb/core/driver.c > +++ b/drivers/usb/core/driver.c > @@ -1583,6 +1583,15 @@ int usb_suspend(struct device *dev, pm_message_t msg) > struct usb_device *udev = to_usb_device(dev); > int r; > > +#ifdef CONFIG_USB_SKIP_SUSPEND > + if (udev->bus->skip_suspend && (msg.event == PM_EVENT_SUSPEND)) { > + if (udev->state != USB_STATE_SUSPENDED) > + dev_err(dev, "abort suspend\n"); You should not use dev_err() because this isn't an error. It is the expected behavior. Why do you test for PM_EVENT_SUSPEND? Don't you want the device to remain at full power during other sorts of PM events also? Why do you test udev->state? Don't you already know that udev is not going to be in the SUSPENDED state? > + > + return 0; > + } > +#endif > + > unbind_no_pm_drivers_interfaces(udev); > > /* From now on we are sure all drivers support suspend/resume > @@ -1619,6 +1628,11 @@ int usb_resume(struct device *dev, pm_message_t msg) > struct usb_device *udev = to_usb_device(dev); > int status; > > +#ifdef CONFIG_USB_SKIP_SUSPEND > + if (udev->bus->skip_suspend && (msg.event == PM_EVENT_RESUME)) > + return 0; > +#endif > + > /* For all calls, take the device back to full power and > * tell the PM core in case it was autosuspended previously. > * Unbind the interfaces that will need rebinding later, > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > index ecaa75718e59..8cbc666ab5c6 100644 > --- a/drivers/usb/host/xhci-plat.c > +++ b/drivers/usb/host/xhci-plat.c > @@ -265,6 +265,13 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s > if (device_property_read_bool(tmpdev, "xhci-skip-phy-init-quirk")) > xhci->quirks |= XHCI_SKIP_PHY_INIT; > > +#ifdef CONFIG_USB_SKIP_SUSPEND > + if (device_property_read_bool(tmpdev, "usb-skip-suspend")) { > + hcd_to_bus(hcd)->skip_suspend = true; > + hcd_to_bus(xhci->shared_hcd)->skip_suspend = true; > + } > +#endif "usb-skip-suspend" is an odd name for this. "usb-never-suspend" would be better, in my opinion. > + > device_property_read_u32(tmpdev, "imod-interval-ns", > &xhci->imod_interval); > } > diff --git a/include/linux/usb.h b/include/linux/usb.h > index 672d8fc2abdb..3074c89ed921 100644 > --- a/include/linux/usb.h > +++ b/include/linux/usb.h > @@ -487,6 +487,15 @@ struct usb_bus { > struct mon_bus *mon_bus; /* non-null when associated */ > int monitored; /* non-zero when monitored */ > #endif > + > +#ifdef CONFIG_USB_SKIP_SUSPEND > + bool skip_suspend; /* All USB devices are brought into suspend > + * power state after system suspend. It is > + * desirable for some specific manufacturers > + * buses to keep their devices in normal > + * state even after system suspend. > + */ > +#endif > }; This patch will prevent the USB devices on the bus from being suspended. But what about the host controller? Don't you need to prevent it from suspending? After all, a USB-2 device will go into low-power suspend mode whenever the host controller stops sending packets -- even if it's connected to a USB-3 host controller. Alan Stern
Yes, to be precise, it's a hardware feature, not an OS/driver policy. At 2024-10-22 20:14:49, "Krzysztof Kozlowski" <krzk@kernel.org> wrote: >On 22/10/2024 11:09, huanglei814 wrote: >> +#ifdef CONFIG_USB_SKIP_SUSPEND >> + if (device_property_read_bool(tmpdev, "usb-skip-suspend")) { > >You are adding this for DTS, so you need bindings and describe hardware >characteristic, not OS/driver policy. > >Best regards, >Krzysztof
Thank you for your reply！ Please refer to the following content for detailed answers. At 2024-10-22 23:00:01, "Alan Stern" <stern@rowland.harvard.edu> wrote: >On Tue, Oct 22, 2024 at 05:09:05PM +0800, huanglei814 wrote: >> From: huanglei <huanglei@kylinos.cn> >> >> All USB devices are brought into suspend power state after system suspend. >> It is desirable for some specific manufacturers buses to keep their devices >> in normal state even after system suspend. > >Why is it desirable for devices on these buses to remain at full power >during system suspend? > >Just to ensure the stability of USB devices which connected to the USB controller bus. > >What about wakeup requests? If the device isn't suspended, it won't be >able to send a wakeup request if it needs to tell the system to return >to full power. > >So When wakeup and execute usb_resume, if skip_suspend, will do nothing. > >What about runtime suspend? Are the devices on these buses supposed to >remain at full power all the time, or only during system suspend? > >Yes, only during system suspend. The difference with runtime suspend is that this avoid all devices which connected on the USB controllers bus enter suspend, but runtime suspend only for one device. > >> v2: Change to bool type for skip_suspend. >> v3: Rebase and update commit message. >> >> Signed-off-by: huanglei <huanglei@kylinos.cn> >> --- >> drivers/usb/core/Kconfig | 11 +++++++++++ >> drivers/usb/core/driver.c | 14 ++++++++++++++ >> drivers/usb/host/xhci-plat.c | 7 +++++++ >> include/linux/usb.h | 9 +++++++++ >> 4 files changed, 41 insertions(+) >> >> diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig >> index 58e3ca7e4793..69778aa7b913 100644 >> --- a/drivers/usb/core/Kconfig >> +++ b/drivers/usb/core/Kconfig >> @@ -143,3 +143,14 @@ config USB_DEFAULT_AUTHORIZATION_MODE >> ACPI selecting value 2 is analogous to selecting value 0. >> >> If unsure, keep the default value. >> + >> +config USB_SKIP_SUSPEND >> + bool "Vendor USB support skip suspend" >> + depends on USB >> + help >> + Select this the associate USB devices will skip suspend when pm control. >> + >> + This option adds support skip suspend for PM control of USB devices >> + in specific manufacturers platforms. >> + >> + If unsure, keep the default value. > >Why does this need to be a Kconfig option? Why can't it be enabled >all the time? > >Enabled all the time won't be any problem, patch v4 will remove Kconfig option. > >> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c >> index 0c3f12daac79..05fe921f8297 100644 >> --- a/drivers/usb/core/driver.c >> +++ b/drivers/usb/core/driver.c >> @@ -1583,6 +1583,15 @@ int usb_suspend(struct device *dev, pm_message_t msg) >> struct usb_device *udev = to_usb_device(dev); >> int r; >> >> +#ifdef CONFIG_USB_SKIP_SUSPEND >> + if (udev->bus->skip_suspend && (msg.event == PM_EVENT_SUSPEND)) { >> + if (udev->state != USB_STATE_SUSPENDED) >> + dev_err(dev, "abort suspend\n"); > >You should not use dev_err() because this isn't an error. It is the >expected behavior. > >patch v4 will change to dev_info. > >Why do you test for PM_EVENT_SUSPEND? Don't you want the device to >remain at full power during other sorts of PM events also? > >Looks like runtime pm just used for suspend, Other PM events is no need， such as s4(hibernate)， all devices must power off. > >Why do you test udev->state? Don't you already know that udev is not >going to be in the SUSPENDED state? > >yes, SUSPENDED state judgment is redundant.patch v4 will modify > >> + >> + return 0; >> + } >> +#endif >> + >> unbind_no_pm_drivers_interfaces(udev); >> >> /* From now on we are sure all drivers support suspend/resume >> @@ -1619,6 +1628,11 @@ int usb_resume(struct device *dev, pm_message_t msg) >> struct usb_device *udev = to_usb_device(dev); >> int status; >> >> +#ifdef CONFIG_USB_SKIP_SUSPEND >> + if (udev->bus->skip_suspend && (msg.event == PM_EVENT_RESUME)) >> + return 0; >> +#endif >> + >> /* For all calls, take the device back to full power and >> * tell the PM core in case it was autosuspended previously. >> * Unbind the interfaces that will need rebinding later, >> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c >> index ecaa75718e59..8cbc666ab5c6 100644 >> --- a/drivers/usb/host/xhci-plat.c >> +++ b/drivers/usb/host/xhci-plat.c >> @@ -265,6 +265,13 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s >> if (device_property_read_bool(tmpdev, "xhci-skip-phy-init-quirk")) >> xhci->quirks |= XHCI_SKIP_PHY_INIT; >> >> +#ifdef CONFIG_USB_SKIP_SUSPEND >> + if (device_property_read_bool(tmpdev, "usb-skip-suspend")) { >> + hcd_to_bus(hcd)->skip_suspend = true; >> + hcd_to_bus(xhci->shared_hcd)->skip_suspend = true; >> + } >> +#endif > >"usb-skip-suspend" is an odd name for this. "usb-never-suspend" >would be better, in my opinion. > >patch v4 will modify it > >> + >> device_property_read_u32(tmpdev, "imod-interval-ns", >> &xhci->imod_interval); >> } >> diff --git a/include/linux/usb.h b/include/linux/usb.h >> index 672d8fc2abdb..3074c89ed921 100644 >> --- a/include/linux/usb.h >> +++ b/include/linux/usb.h >> @@ -487,6 +487,15 @@ struct usb_bus { >> struct mon_bus *mon_bus; /* non-null when associated */ >> int monitored; /* non-zero when monitored */ >> #endif >> + >> +#ifdef CONFIG_USB_SKIP_SUSPEND >> + bool skip_suspend; /* All USB devices are brought into suspend >> + * power state after system suspend. It is >> + * desirable for some specific manufacturers >> + * buses to keep their devices in normal >> + * state even after system suspend. >> + */ >> +#endif >> }; > >This patch will prevent the USB devices on the bus from being suspended. >But what about the host controller? Don't you need to prevent it from >suspending? After all, a USB-2 device will go into low-power suspend >mode whenever the host controller stops sending packets -- even if it's >connected to a USB-3 host controller. > >Host controller will not changed, The essence is specific controller to avoid cause stability issues (such as response slowly or no response) of external devices when they enter suspend/resume mode, rather than making the controllers and external devices to continue working normally when enter suspend. > >Alan Stern best regard!
On 23/10/2024 05:32, huanglei wrote: > > Yes, to be precise, it's a hardware feature, not an OS/driver policy. ??? No, you did not describe it as hardware features. You need to fix this. It's a NAK so far. Please don't top-post. Best regards, Krzysztof
diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig index 58e3ca7e4793..69778aa7b913 100644 --- a/drivers/usb/core/Kconfig +++ b/drivers/usb/core/Kconfig @@ -143,3 +143,14 @@ config USB_DEFAULT_AUTHORIZATION_MODE ACPI selecting value 2 is analogous to selecting value 0. If unsure, keep the default value. + +config USB_SKIP_SUSPEND + bool "Vendor USB support skip suspend" + depends on USB + help + Select this the associate USB devices will skip suspend when pm control. + + This option adds support skip suspend for PM control of USB devices + in specific manufacturers platforms. + + If unsure, keep the default value. diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index 0c3f12daac79..05fe921f8297 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -1583,6 +1583,15 @@ int usb_suspend(struct device *dev, pm_message_t msg) struct usb_device *udev = to_usb_device(dev); int r; +#ifdef CONFIG_USB_SKIP_SUSPEND + if (udev->bus->skip_suspend && (msg.event == PM_EVENT_SUSPEND)) { + if (udev->state != USB_STATE_SUSPENDED) + dev_err(dev, "abort suspend\n"); + + return 0; + } +#endif + unbind_no_pm_drivers_interfaces(udev); /* From now on we are sure all drivers support suspend/resume @@ -1619,6 +1628,11 @@ int usb_resume(struct device *dev, pm_message_t msg) struct usb_device *udev = to_usb_device(dev); int status; +#ifdef CONFIG_USB_SKIP_SUSPEND + if (udev->bus->skip_suspend && (msg.event == PM_EVENT_RESUME)) + return 0; +#endif + /* For all calls, take the device back to full power and * tell the PM core in case it was autosuspended previously. * Unbind the interfaces that will need rebinding later, diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index ecaa75718e59..8cbc666ab5c6 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -265,6 +265,13 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s if (device_property_read_bool(tmpdev, "xhci-skip-phy-init-quirk")) xhci->quirks |= XHCI_SKIP_PHY_INIT; +#ifdef CONFIG_USB_SKIP_SUSPEND + if (device_property_read_bool(tmpdev, "usb-skip-suspend")) { + hcd_to_bus(hcd)->skip_suspend = true; + hcd_to_bus(xhci->shared_hcd)->skip_suspend = true; + } +#endif + device_property_read_u32(tmpdev, "imod-interval-ns", &xhci->imod_interval); } diff --git a/include/linux/usb.h b/include/linux/usb.h index 672d8fc2abdb..3074c89ed921 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -487,6 +487,15 @@ struct usb_bus { struct mon_bus *mon_bus; /* non-null when associated */ int monitored; /* non-zero when monitored */ #endif + +#ifdef CONFIG_USB_SKIP_SUSPEND + bool skip_suspend; /* All USB devices are brought into suspend + * power state after system suspend. It is + * desirable for some specific manufacturers + * buses to keep their devices in normal + * state even after system suspend. + */ +#endif }; struct usb_dev_state;