On Wed, Oct 23, 2024 at 11:44:57AM +0800, huanglei814 wrote:
> From: huanglei <huanglei@xxxxxxxxxx>
>
> 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.
Here you should explain _why_ this is desirable. People will want to
know, so that they can judge whether this is a good thing to do.
> Signed-off-by: huanglei <huanglei@xxxxxxxxxx>
> ---
> v3->v4:
> - Cancel SUSPENDED state judgment when enter suspend，because udev will not enter.
> - Change "usb-skip-suspend" to "usb-never-suspend"
> - Change dev_err to dev_info.
> - Remove Kconfig option, it's redundant indeed.
> - Update commit message style.
> v2->v3:
> - Rebase and update commit message.
> v1->v2:
> - Change to bool type for skip_suspend.
> - Kconfig remove "default n", 'n' is the default.
> ---
> drivers/usb/core/driver.c | 8 ++++++++
> drivers/usb/host/xhci-plat.c | 5 +++++
> include/linux/usb.h | 7 +++++++
> 3 files changed, 20 insertions(+)
>
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 0c3f12daac79..93137c7c34df 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -1583,6 +1583,11 @@ int usb_suspend(struct device *dev, pm_message_t msg)
> struct usb_device *udev = to_usb_device(dev);
> int r;
>
> + if (udev->bus->skip_suspend && (msg.event == PM_EVENT_SUSPEND)) {
> + dev_info(dev, "abort suspend\n");
> + return 0;
Do you really need this message at all? Maybe it should be dev_dbg,
because it's useful only to developers, not to normal users.
Also, you're not really aborting the suspend; you're skipping it.
> + }
> +
> unbind_no_pm_drivers_interfaces(udev);
>
> /* From now on we are sure all drivers support suspend/resume
> @@ -1619,6 +1624,9 @@ int usb_resume(struct device *dev, pm_message_t msg)
> struct usb_device *udev = to_usb_device(dev);
> int status;
>
> + if (udev->bus->skip_suspend && (msg.event == PM_EVENT_RESUME))
> + return 0;
> +
> /* 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..35062aa19a32 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -265,6 +265,11 @@ 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;
>
> + if (device_property_read_bool(tmpdev, "usb-never-suspend")) {
Hmmm. "usb-never-suspend" implies that the bus won't even be put into
runtime suspend, but that's not what you mean. What you really mean is
more like "stay-powered-during-system-suspend", but that name is a bit
long. Maybe you can think of something better.
Also, this sounds more like the manufacturer's policy decision rather
than a hardware limitation. Is that correct? If it is, I doubt that
the setting really belongs in DT.
> + hcd_to_bus(hcd)->skip_suspend = true;
> + hcd_to_bus(xhci->shared_hcd)->skip_suspend = true;
> + }
> +
> 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..c854d1f622ec 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -487,6 +487,13 @@ struct usb_bus {
> struct mon_bus *mon_bus; /* non-null when associated */
> int monitored; /* non-zero when monitored */
> #endif
> +
> + 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.
> + */
This is not a good comment. It doesn't say what the skip_suspend flag
means. You don't need to explain the reason for the flag here; just
explain what it does.
Alan Stern
> };
>
> struct usb_dev_state;
> --
> 2.17.1
>
>
