| 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
| ||
|
Message-ID: <20230711220748.vmnvwwcu5nhrvyvi@synopsys.com>
Date: Tue, 11 Jul 2023 22:07:51 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Elson Roy Serrao <quic_eserrao@...cinc.com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
"stern@...land.harvard.edu" <stern@...land.harvard.edu>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"rogerq@...nel.org" <rogerq@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"quic_wcheng@...cinc.com" <quic_wcheng@...cinc.com>,
"quic_jackp@...cinc.com" <quic_jackp@...cinc.com>
Subject: Re: [PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus
suspend
On Tue, Jul 11, 2023, Elson Roy Serrao wrote:
> The current implementation blocks the runtime pm operations when cable
> is connected. This would block platforms from entering system wide suspend
> during bus suspend scenario. Modify the runtime pm ops to handle bus
> suspend case for such platforms where the controller low power mode
> entry/exit is handled by the glue driver. This enablement is controlled
> through a dt property and platforms capable of detecting bus resume can
> benefit from this feature. Also modify the remote wakeup operations to
> trigger runtime resume before sending wakeup signal.
>
> Signed-off-by: Elson Roy Serrao <quic_eserrao@...cinc.com>
> ---
> drivers/usb/dwc3/core.c | 26 ++++++++++++++++++++++---
> drivers/usb/dwc3/core.h | 3 +++
> drivers/usb/dwc3/gadget.c | 40 ++++++++++++++++++++++++++++++++-------
> 3 files changed, 59 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index f6689b731718..898c0f68e190 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1534,6 +1534,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
> dwc->dis_split_quirk = device_property_read_bool(dev,
> "snps,dis-split-quirk");
>
> + dwc->allow_rtsusp_on_u3 = device_property_read_bool(dev,
> + "snps,allow-rtsusp-on-u3");
> +
> dwc->lpm_nyet_threshold = lpm_nyet_threshold;
> dwc->tx_de_emphasis = tx_de_emphasis;
>
> @@ -1984,11 +1987,21 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> {
> unsigned long flags;
> u32 reg;
> + int link_state;
>
> switch (dwc->current_dr_role) {
> case DWC3_GCTL_PRTCAP_DEVICE:
> if (pm_runtime_suspended(dwc->dev))
> break;
> +
> + if (dwc->connected) {
> + link_state = dwc3_gadget_get_link_state(dwc);
> + /* bus suspend case */
> + if (dwc->allow_rtsusp_on_u3 &&
> + link_state == DWC3_LINK_STATE_U3)
> + break;
> + return -EBUSY;
> + }
> dwc3_gadget_suspend(dwc);
> synchronize_irq(dwc->irq_gadget);
> dwc3_core_exit(dwc);
> @@ -2045,6 +2058,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>
> switch (dwc->current_dr_role) {
> case DWC3_GCTL_PRTCAP_DEVICE:
> + /* bus resume case */
> + if (dwc->connected)
> + break;
> ret = dwc3_core_init_for_resume(dwc);
> if (ret)
> return ret;
> @@ -2123,9 +2139,6 @@ static int dwc3_runtime_suspend(struct device *dev)
> struct dwc3 *dwc = dev_get_drvdata(dev);
> int ret;
>
> - if (dwc3_runtime_checks(dwc))
> - return -EBUSY;
> -
> ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND);
> if (ret)
> return ret;
> @@ -2160,9 +2173,15 @@ static int dwc3_runtime_resume(struct device *dev)
> static int dwc3_runtime_idle(struct device *dev)
> {
> struct dwc3 *dwc = dev_get_drvdata(dev);
> + int link_state;
>
> switch (dwc->current_dr_role) {
> case DWC3_GCTL_PRTCAP_DEVICE:
> + link_state = dwc3_gadget_get_link_state(dwc);
> + /* for bus suspend case return success */
> + if (dwc->allow_rtsusp_on_u3 && dwc->connected &&
> + link_state == DWC3_LINK_STATE_U3)
> + goto autosuspend;
> if (dwc3_runtime_checks(dwc))
> return -EBUSY;
> break;
> @@ -2172,6 +2191,7 @@ static int dwc3_runtime_idle(struct device *dev)
> break;
> }
>
> +autosuspend:
> pm_runtime_mark_last_busy(dev);
> pm_runtime_autosuspend(dev);
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 8b1295e4dcdd..33b2ccbbd963 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1127,6 +1127,8 @@ struct dwc3_scratchpad_array {
> * @num_ep_resized: carries the current number endpoints which have had its tx
> * fifo resized.
> * @debug_root: root debugfs directory for this device to put its files in.
> + * @allow_rtsusp_on_u3: true if dwc3 runtime suspend is allowed during bus
> + * suspend scenario.
> */
> struct dwc3 {
> struct work_struct drd_work;
> @@ -1343,6 +1345,7 @@ struct dwc3 {
> int last_fifo_depth;
> int num_ep_resized;
> struct dentry *debug_root;
> + bool allow_rtsusp_on_u3;
> };
>
> #define INCRX_BURST_MODE 0
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 5fd067151fbf..0797cffa2d48 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2401,15 +2401,21 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
> return -EINVAL;
> }
>
> - spin_lock_irqsave(&dwc->lock, flags);
> if (!dwc->gadget->wakeup_armed) {
> dev_err(dwc->dev, "not armed for remote wakeup\n");
> - spin_unlock_irqrestore(&dwc->lock, flags);
> return -EINVAL;
> }
> - ret = __dwc3_gadget_wakeup(dwc, true);
>
> + ret = pm_runtime_resume_and_get(dwc->dev);
> + if (ret < 0) {
> + pm_runtime_set_suspended(dwc->dev);
> + return ret;
> + }
> +
> + spin_lock_irqsave(&dwc->lock, flags);
> + ret = __dwc3_gadget_wakeup(dwc, true);
> spin_unlock_irqrestore(&dwc->lock, flags);
> + pm_runtime_put_noidle(dwc->dev);
>
> return ret;
> }
> @@ -2428,6 +2434,12 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
> return -EINVAL;
> }
>
> + ret = pm_runtime_resume_and_get(dwc->dev);
> + if (ret < 0) {
> + pm_runtime_set_suspended(dwc->dev);
> + return ret;
> + }
> +
> spin_lock_irqsave(&dwc->lock, flags);
> /*
> * If the link is in U3, signal for remote wakeup and wait for the
> @@ -2438,6 +2450,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
> ret = __dwc3_gadget_wakeup(dwc, false);
> if (ret) {
> spin_unlock_irqrestore(&dwc->lock, flags);
> + pm_runtime_put_noidle(dwc->dev);
> return -EINVAL;
> }
> dwc3_resume_gadget(dwc);
> @@ -2452,6 +2465,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
> dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret);
>
> spin_unlock_irqrestore(&dwc->lock, flags);
> + pm_runtime_put_noidle(dwc->dev);
>
> return ret;
> }
> @@ -2732,21 +2746,23 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> /*
> * Avoid issuing a runtime resume if the device is already in the
> * suspended state during gadget disconnect. DWC3 gadget was already
> - * halted/stopped during runtime suspend.
> + * halted/stopped during runtime suspend except for bus suspend case
> + * where we would have skipped the controller halt.
> */
> if (!is_on) {
> pm_runtime_barrier(dwc->dev);
> - if (pm_runtime_suspended(dwc->dev))
> + if (pm_runtime_suspended(dwc->dev) && !dwc->connected)
> return 0;
> }
>
> /*
> * Check the return value for successful resume, or error. For a
> * successful resume, the DWC3 runtime PM resume routine will handle
> - * the run stop sequence, so avoid duplicate operations here.
> + * the run stop sequence except for bus resume case, so avoid
> + * duplicate operations here.
> */
> ret = pm_runtime_get_sync(dwc->dev);
> - if (!ret || ret < 0) {
> + if ((!ret && !dwc->connected) || ret < 0) {
> pm_runtime_put(dwc->dev);
> if (ret < 0)
> pm_runtime_set_suspended(dwc->dev);
> @@ -4331,6 +4347,8 @@ static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
> }
>
> dwc->link_state = next;
> + pm_runtime_mark_last_busy(dwc->dev);
> + pm_request_autosuspend(dwc->dev);
> }
>
> static void dwc3_gadget_interrupt(struct dwc3 *dwc,
> @@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
> {
> if (dwc->pending_events) {
> dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
> + pm_runtime_put(dwc->dev);
> dwc->pending_events = false;
> enable_irq(dwc->irq_gadget);
> + /*
> + * We have only stored the pending events as part
> + * of dwc3_interrupt() above, but those events are
> + * not yet handled. So explicitly invoke the
> + * interrupt handler for handling those events.
> + */
> + dwc3_thread_interrupt(dwc->irq_gadget, dwc->ev_buf);
Why do we have to do this? If there are events, the threaded interrupt
should be woken up.
Thanks,
Thinh
> }
> }
> --
> 2.17.1
>
Powered by blists - more mailing lists