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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <561a32d3-a37a-4b16-a214-8f4bacc17b8a@rowland.harvard.edu>
Date: Wed, 23 Oct 2024 10:25:37 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: huanglei814 <huanglei814@....com>
Cc: gregkh@...uxfoundation.org, mathias.nyman@...el.com,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	huanglei <huanglei@...inos.cn>
Subject: Re: [PATCH v4] usb: core: adds support for PM control of specific
 USB dev skip suspend.

On Wed, Oct 23, 2024 at 11:44:57AM +0800, huanglei814 wrote:
> From: huanglei <huanglei@...inos.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.

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@...inos.cn>
> ---
> 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
> 
>