| 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: <af332749-fdd7-e744-16f1-21f972161589@quicinc.com>
Date: Tue, 11 Jul 2023 15:51:35 -0700
From: Elson Serrao <quic_eserrao@...cinc.com>
To: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
CC: "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 7/11/2023 3:07 PM, Thinh Nguyen wrote:
> 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.
>
dwc3_thread_interrupt will be woken up only if dwc3_interrupt() handler
is invoked by the interrupt framework when the return value of
IRQ_WAKE_THREAD is handled. But while processing the pending events the
interrupt framework is not involved. We explicitly invoke the
dwc3_interrupt() above within the dwc3 driver. So the
dwc3_thread_interrupt() has to be explicitly invoked as well for
processing those pending events.
Thanks
Elson
Powered by blists - more mailing lists