| 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: <761d77bf-7ce0-d182-41d2-ab4a7691fef5@quicinc.com>
Date: Tue, 11 Jul 2023 16:22:12 -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:51 PM, Elson Serrao wrote:
>
>
> 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.
>
Perhaps we can make it more optimal by checking the return value like below?
void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
{
irqreturn_t ret;
if (dwc->pending_events) {
ret = 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.
*/
if (ret == IRQ_WAKE_THREAD)
dwc3_thread_interrupt(dwc->irq_gadget,
dwc->ev_buf);
}
}
Powered by blists - more mailing lists