Re: [PATCH] usb: core: adds support for PM control of specific USB dev skip suspend.

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Oct 21, 2024 at 08:47:41PM +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 desireable to do that?  Why can't you just do so from
userspace today?

What hardware requires this?

> Signed-off-by: huanglei <huanglei@kylinos.cn>
> ---
>  drivers/usb/core/Kconfig     | 12 ++++++++++++
>  drivers/usb/core/driver.c    | 14 ++++++++++++++
>  drivers/usb/host/xhci-plat.c |  7 +++++++
>  include/linux/usb.h          |  9 +++++++++
>  4 files changed, 42 insertions(+)
> 
> diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
> index 58e3ca7e4793..fe178c90d167 100644
> --- a/drivers/usb/core/Kconfig
> +++ b/drivers/usb/core/Kconfig
> @@ -143,3 +143,15 @@ 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
> +	default n

No need for this line, 'n' is the default.

> +	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

Please do not put #ifdef lines in .c files if at all possible.

> +
>  	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

Why are you only doing this for xhci platform drivers?


> +
>  		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..5f88850fc42d 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
> +	unsigned int 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.
> +					 */

Shouldn't this be a boolean?

thanks,

greg k-h