| 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
| ||
|
Date: Wed, 3 Aug 2022 00:01:14 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Elson Roy Serrao <quic_eserrao@...cinc.com>,
"balbi@...nel.org" <balbi@...nel.org>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC: "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>,
"quic_mrana@...cinc.com" <quic_mrana@...cinc.com>,
Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Subject: Re: [PATCH 1/5] usb: dwc3: Add remote wakeup handling
On 8/2/2022, Elson Roy Serrao wrote:
> An usb device can initate a remote wakeup and bring the link out of
> suspend as dictated by the DEVICE_REMOTE_WAKEUP feature selector.
> Add support to handle this packet and set the remote wakeup capability
> accordingly.
>
> Some hosts may take longer time to initiate the resume
> signaling after device triggers a remote wakeup. So improve the
> gadget_wakeup op to interrupt based rather than polling based by
> enabling link status change events.
>
> Signed-off-by: Elson Roy Serrao <quic_eserrao@...cinc.com>
> ---
> drivers/usb/dwc3/core.h | 4 +++
> drivers/usb/dwc3/ep0.c | 4 +++
> drivers/usb/dwc3/gadget.c | 69 ++++++++++++++++++++++++++++++++++++++++++++---
> 3 files changed, 74 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 4fe4287..3306b1c 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1113,6 +1113,8 @@ struct dwc3_scratchpad_array {
> * address.
> * @num_ep_resized: carries the current number endpoints which have had its tx
> * fifo resized.
> + * @is_remote_wakeup_enabled: remote wakeup status from host perspective
> + * @is_gadget_wakeup: remote wakeup requested via gadget op.
> */
> struct dwc3 {
> struct work_struct drd_work;
> @@ -1326,6 +1328,8 @@ struct dwc3 {
> int max_cfg_eps;
> int last_fifo_depth;
> int num_ep_resized;
> + bool is_remote_wakeup_enabled;
> + bool is_gadget_wakeup;
> };
>
> #define INCRX_BURST_MODE 0
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 197af63..4cc3d3a 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -353,6 +353,9 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc,
> usb_status |= 1 << USB_DEV_STAT_U1_ENABLED;
> if (reg & DWC3_DCTL_INITU2ENA)
> usb_status |= 1 << USB_DEV_STAT_U2_ENABLED;
> + } else {
> + usb_status |= dwc->is_remote_wakeup_enabled <<
> + USB_DEVICE_REMOTE_WAKEUP;
You need to create a new macro for function remote enabled. Name it
something like USB_DEV_STAT_FUNC_RW_ENABLED to match with others in ch9.h
> }
>
> break;
> @@ -473,6 +476,7 @@ static int dwc3_ep0_handle_device(struct dwc3 *dwc,
>
> switch (wValue) {
> case USB_DEVICE_REMOTE_WAKEUP:
> + dwc->is_remote_wakeup_enabled = set;
> break;
> /*
> * 9.4.1 says only for SS, in AddressState only for
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 4366c45..d6697da 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2232,6 +2232,22 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = {
>
> /* -------------------------------------------------------------------------- */
>
> +static void linksts_change_events_set(struct dwc3 *dwc, bool set)
It should be named something that's consistent with the other functions.
Something like dwc3_gadget_enable_linkstate_change_event()
> +{
> + u32 reg;
> +
> + reg = dwc3_readl(dwc->regs, DWC3_DEVTEN);
> + if (set)
> + reg |= DWC3_DEVTEN_ULSTCNGEN;
> + else
> + reg &= ~DWC3_DEVTEN_ULSTCNGEN;
> +
> + dwc3_writel(dwc->regs, DWC3_DEVTEN, reg);
> +
> + /* Required to complete this operation before returning */
> + mb();
Why do we need memory barrier here? It's just register write, and we
already have io barrier for that.
> +}
> +
> static int dwc3_gadget_get_frame(struct usb_gadget *g)
> {
> struct dwc3 *dwc = gadget_to_dwc(g);
> @@ -2270,9 +2286,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
> return -EINVAL;
> }
>
> + linksts_change_events_set(dwc, true);
> +
> ret = dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RECOV);
> if (ret < 0) {
> dev_err(dwc->dev, "failed to put link in Recovery\n");
> + linksts_change_events_set(dwc, false);
> + dwc->is_gadget_wakeup = false;
> return ret;
> }
>
> @@ -2284,9 +2304,15 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
> dwc3_writel(dwc->regs, DWC3_DCTL, reg);
> }
>
> + /* If remote wakeup is triggered from function driver, bail out.
Follow this type of doc
/*
* xxxx
* yyyy
*/
> + * Since link status change events are enabled we would receive
> + * an U0 event when wakeup is successful.
> + */
> + if (dwc->is_gadget_wakeup)
> + return -EAGAIN;
> +
> /* poll until Link State changes to ON */
> retries = 20000;
> -
> while (retries--) {
> reg = dwc3_readl(dwc->regs, DWC3_DSTS);
>
> @@ -2295,6 +2321,8 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
> break;
> }
>
> + linksts_change_events_set(dwc, false);
> +
> if (DWC3_DSTS_USBLNKST(reg) != DWC3_LINK_STATE_U0) {
> dev_err(dwc->dev, "failed to send remote wakeup\n");
> return -EINVAL;
> @@ -2310,7 +2338,20 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
> int ret;
>
> spin_lock_irqsave(&dwc->lock, flags);
> + if (g->speed < USB_SPEED_SUPER && !dwc->is_remote_wakeup_enabled) {
> + dev_err(dwc->dev, "%s:remote wakeup not supported\n", __func__);
> + ret = -EPERM;
> + goto out;
> + }
> + if (dwc->is_gadget_wakeup) {
> + dev_err(dwc->dev, "%s: remote wakeup in progress\n", __func__);
> + ret = -EINVAL;
> + goto out;
> + }
> + dwc->is_gadget_wakeup = true;
> ret = __dwc3_gadget_wakeup(dwc);
> +
> +out:
> spin_unlock_irqrestore(&dwc->lock, flags);
>
> return ret;
> @@ -2766,6 +2807,9 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>
> spin_lock_irqsave(&dwc->lock, flags);
> dwc->gadget_driver = driver;
> + linksts_change_events_set(dwc, false);
> + dwc->is_remote_wakeup_enabled = false;
> + dwc->is_gadget_wakeup = false;
> spin_unlock_irqrestore(&dwc->lock, flags);
>
> return 0;
> @@ -2785,6 +2829,9 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>
> spin_lock_irqsave(&dwc->lock, flags);
> dwc->gadget_driver = NULL;
> + linksts_change_events_set(dwc, false);
> + dwc->is_remote_wakeup_enabled = false;
> + dwc->is_gadget_wakeup = false;
> dwc->max_cfg_eps = 0;
> spin_unlock_irqrestore(&dwc->lock, flags);
>
> @@ -3768,6 +3815,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
> usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
>
> dwc->connected = false;
> + linksts_change_events_set(dwc, false);
> + dwc->is_gadget_wakeup = false;
> }
>
> static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
> @@ -3855,6 +3904,10 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
> reg = dwc3_readl(dwc->regs, DWC3_DCFG);
> reg &= ~(DWC3_DCFG_DEVADDR_MASK);
> dwc3_writel(dwc->regs, DWC3_DCFG, reg);
> +
> + dwc->is_remote_wakeup_enabled = false;
> + linksts_change_events_set(dwc, false);
> + dwc->is_gadget_wakeup = false;
> }
>
> static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
> @@ -3998,8 +4051,9 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
> */
> }
>
> -static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
> +static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo)
> {
> + enum dwc3_link_state next = evtinfo & DWC3_LINK_STATE_MASK;
> /*
> * TODO take core out of low power mode when that's
> * implemented.
> @@ -4010,6 +4064,8 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
> dwc->gadget_driver->resume(dwc->gadget);
> spin_lock(&dwc->lock);
> }
> +
> + dwc->link_state = next;
> }
>
> static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
> @@ -4091,6 +4147,13 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
> }
>
> switch (next) {
> + case DWC3_LINK_STATE_U0:
> + if (dwc->is_gadget_wakeup) {
> + linksts_change_events_set(dwc, false);
> + dwc3_resume_gadget(dwc);
> + dwc->is_gadget_wakeup = false;
> + }
> + break;
> case DWC3_LINK_STATE_U1:
> if (dwc->speed == USB_SPEED_SUPER)
> dwc3_suspend_gadget(dwc);
> @@ -4159,7 +4222,7 @@ static void dwc3_gadget_interrupt(struct dwc3 *dwc,
> dwc3_gadget_conndone_interrupt(dwc);
> break;
> case DWC3_DEVICE_EVENT_WAKEUP:
> - dwc3_gadget_wakeup_interrupt(dwc);
> + dwc3_gadget_wakeup_interrupt(dwc, event->event_info);
> break;
> case DWC3_DEVICE_EVENT_HIBER_REQ:
> if (dev_WARN_ONCE(dwc->dev, !dwc->has_hibernation,
Powered by blists - more mailing lists