| 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: Tue, 14 Feb 2023 18:08:30 -0800
From: Elson Serrao <quic_eserrao@...cinc.com>
To: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
CC: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"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 2/14/2023 5:09 PM, Thinh Nguyen wrote:
> 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
>
Sure. I will upload v5 with this modification.
Thanks
Elson
>> + 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