| 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: <20230215010950.ufazmdev7cvtef3n@synopsys.com>
Date: Wed, 15 Feb 2023 01:09:55 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Elson Roy Serrao <quic_eserrao@...cinc.com>
CC: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
"balbi@...nel.org" <balbi@...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 v4 2/5] usb: dwc3: Add remote wakeup handling
On Mon, Feb 13, 2023, 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.
>
> Some hosts may take longer time to initiate the resume signaling after
> device triggers a remote wakeup. So add async support to the wakeup API
> by enabling link status change events.
>
> Signed-off-by: Elson Roy Serrao <quic_eserrao@...cinc.com>
> ---
> drivers/usb/dwc3/core.h | 2 ++
> drivers/usb/dwc3/ep0.c | 4 +++
> drivers/usb/dwc3/gadget.c | 74 +++++++++++++++++++++++++++++++++++++++++++----
> 3 files changed, 74 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 582ebd9..cc78236 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1110,6 +1110,7 @@ struct dwc3_scratchpad_array {
> * 3 - Reserved
> * @dis_metastability_quirk: set to disable metastability quirk.
> * @dis_split_quirk: set to disable split boundary.
> + * @wakeup_configured: set if the device is configured for remote wakeup.
> * @imod_interval: set the interrupt moderation interval in 250ns
> * increments or 0 to disable.
> * @max_cfg_eps: current max number of IN eps used across all USB configs.
> @@ -1327,6 +1328,7 @@ struct dwc3 {
>
> unsigned dis_split_quirk:1;
> unsigned async_callbacks:1;
> + unsigned wakeup_configured:1;
>
> u16 imod_interval;
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 61de693..f90fa55 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -356,6 +356,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->gadget->wakeup_armed <<
> + USB_DEVICE_REMOTE_WAKEUP;
> }
>
> break;
> @@ -476,6 +479,7 @@ static int dwc3_ep0_handle_device(struct dwc3 *dwc,
>
> switch (wValue) {
> case USB_DEVICE_REMOTE_WAKEUP:
> + dwc->gadget->wakeup_armed = 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 3c63fa9..b7fef5d 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -258,7 +258,7 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
> return ret;
> }
>
> -static int __dwc3_gadget_wakeup(struct dwc3 *dwc);
> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async);
>
> /**
> * dwc3_send_gadget_ep_cmd - issue an endpoint command
> @@ -325,7 +325,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
>
> fallthrough;
> case DWC3_LINK_STATE_U3:
> - ret = __dwc3_gadget_wakeup(dwc);
> + ret = __dwc3_gadget_wakeup(dwc, false);
> dev_WARN_ONCE(dwc->dev, ret, "wakeup failed --> %d\n",
> ret);
> break;
> @@ -2269,6 +2269,22 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = {
>
> /* -------------------------------------------------------------------------- */
>
> +static void dwc3_gadget_enable_linksts_evts(struct dwc3 *dwc, bool set)
> +{
> + u32 reg;
> +
> + if (DWC3_VER_IS_PRIOR(DWC3, 250A))
> + return;
> +
> + 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);
> +}
> +
> static int dwc3_gadget_get_frame(struct usb_gadget *g)
> {
> struct dwc3 *dwc = gadget_to_dwc(g);
> @@ -2276,7 +2292,7 @@ static int dwc3_gadget_get_frame(struct usb_gadget *g)
> return __dwc3_gadget_get_frame(dwc);
> }
>
> -static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
> {
> int retries;
>
> @@ -2307,9 +2323,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
> return -EINVAL;
> }
>
> + if (async)
> + dwc3_gadget_enable_linksts_evts(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");
> + dwc3_gadget_enable_linksts_evts(dwc, false);
> return ret;
> }
>
> @@ -2321,6 +2341,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
> dwc3_writel(dwc->regs, DWC3_DCTL, reg);
> }
>
> + /*
> + * Since link status change events are enabled we will receive
> + * an U0 event when wakeup is successful. So bail out.
> + */
> + if (async)
> + return 0;
> +
> /* poll until Link State changes to ON */
> retries = 20000;
>
> @@ -2346,13 +2373,36 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
> unsigned long flags;
> int ret;
>
> + if (!dwc->wakeup_configured) {
> + dev_err(dwc->dev, "remote wakeup not configured\n");
> + return -EINVAL;
> + }
> +
> spin_lock_irqsave(&dwc->lock, flags);
> - ret = __dwc3_gadget_wakeup(dwc);
> + 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);
> +
> spin_unlock_irqrestore(&dwc->lock, flags);
>
> return ret;
> }
>
> +static int dwc3_gadget_set_remote_wakeup(struct usb_gadget *g, int set)
> +{
> + struct dwc3 *dwc = gadget_to_dwc(g);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dwc->lock, flags);
> + dwc->wakeup_configured = !!set;
> + spin_unlock_irqrestore(&dwc->lock, flags);
> +
> + return 0;
> +}
> +
> static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
> int is_selfpowered)
> {
> @@ -2978,6 +3028,7 @@ static void dwc3_gadget_async_callbacks(struct usb_gadget *g, bool enable)
> static const struct usb_gadget_ops dwc3_gadget_ops = {
> .get_frame = dwc3_gadget_get_frame,
> .wakeup = dwc3_gadget_wakeup,
> + .set_remote_wakeup = dwc3_gadget_set_remote_wakeup,
> .set_selfpowered = dwc3_gadget_set_selfpowered,
> .pullup = dwc3_gadget_pullup,
> .udc_start = dwc3_gadget_start,
> @@ -3819,6 +3870,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
>
> dwc->gadget->speed = USB_SPEED_UNKNOWN;
> dwc->setup_packet_pending = false;
> + dwc->gadget->wakeup_armed = false;
> + dwc3_gadget_enable_linksts_evts(dwc, false);
> usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
>
> if (dwc->ep0state != EP0_SETUP_PHASE) {
> @@ -3912,6 +3965,8 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
> reg &= ~DWC3_DCTL_TSTCTRL_MASK;
> dwc3_gadget_dctl_write_safe(dwc, reg);
> dwc->test_mode = false;
> + dwc->gadget->wakeup_armed = false;
> + dwc3_gadget_enable_linksts_evts(dwc, false);
> dwc3_clear_stall_all_ep(dwc);
>
> /* Reset device address to zero */
> @@ -4064,7 +4119,7 @@ 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)
> {
> /*
> * TODO take core out of low power mode when that's
> @@ -4076,6 +4131,8 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
> dwc->gadget_driver->resume(dwc->gadget);
> spin_lock(&dwc->lock);
> }
> +
> + dwc->link_state = evtinfo & DWC3_LINK_STATE_MASK;
> }
>
> static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
> @@ -4157,6 +4214,10 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
> }
>
> switch (next) {
> + case DWC3_LINK_STATE_U0:
> + dwc3_gadget_enable_linksts_evts(dwc, false);
> + dwc3_resume_gadget(dwc);
I don't think this should be a problem for dwc_usb3 2.50a and prior. But
for a device that enables link state change event all the time, it would
call this every time link transitions to U0. I'm not sure what the side
effect can be. Can we add a wakeup_armed check here to prevent possible
regression?
Thanks,
Thinh
> + break;
> case DWC3_LINK_STATE_U1:
> if (dwc->speed == USB_SPEED_SUPER)
> dwc3_suspend_gadget(dwc);
> @@ -4225,7 +4286,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,
> @@ -4485,6 +4546,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
> dwc->gadget->sg_supported = true;
> dwc->gadget->name = "dwc3-gadget";
> dwc->gadget->lpm_capable = !dwc->usb2_gadget_lpm_disable;
> + dwc->gadget->wakeup_capable = true;
>
> /*
> * FIXME We might be setting max_speed to <SUPER, however versions
> --
> 2.7.4
>
Powered by blists - more mailing lists