| 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, 2 Aug 2022 23:44:19 +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 3/5] usb: dwc3: Add function suspend and function wakeup
support
On 8/2/2022, Elson Roy Serrao wrote:
> USB host sends function suspend and resume notifications to
> device through SET_FEATURE setup packet. This packet is directed
> to the interface to which function suspend/resume is intended to.
> Add support to handle this packet by delegating the request to composite
> layer. To exit from function suspend the interface needs to trigger a
> function wakeup in case it wants to initate a data transfer. Expose a
> new gadget op so that an interface can send function wakeup request to
> the host.
>
> Signed-off-by: Elson Roy Serrao <quic_eserrao@...cinc.com>
> ---
> drivers/usb/dwc3/core.h | 1 +
> drivers/usb/dwc3/ep0.c | 12 ++++--------
> drivers/usb/dwc3/gadget.c | 30 ++++++++++++++++++++++++++++++
> 3 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 3306b1c..e08e522 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -519,6 +519,7 @@
> #define DWC3_DGCMD_SET_SCRATCHPAD_ADDR_LO 0x04
> #define DWC3_DGCMD_SET_SCRATCHPAD_ADDR_HI 0x05
>
> +#define DWC3_DGCMD_XMIT_DEV 0x07
This is device notification, so just name this to
DWC_DGCMD_DEV_NOTIFICATION. Also update debug.h to print the proper
string for this command.
> #define DWC3_DGCMD_SELECTED_FIFO_FLUSH 0x09
> #define DWC3_DGCMD_ALL_FIFO_FLUSH 0x0a
> #define DWC3_DGCMD_SET_ENDPOINT_NRDY 0x0c
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 4cc3d3a..cedc890 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -30,6 +30,8 @@
> static void __dwc3_ep0_do_control_status(struct dwc3 *dwc, struct dwc3_ep *dep);
> static void __dwc3_ep0_do_control_data(struct dwc3 *dwc,
> struct dwc3_ep *dep, struct dwc3_request *req);
> +static int dwc3_ep0_delegate_req(struct dwc3 *dwc,
> + struct usb_ctrlrequest *ctrl);
>
> static void dwc3_ep0_prepare_one_trb(struct dwc3_ep *dep,
> dma_addr_t buf_dma, u32 len, u32 type, bool chain)
> @@ -365,7 +367,7 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc,
> * Function Remote Wake Capable D0
> * Function Remote Wakeup D1
> */
> - break;
> + return dwc3_ep0_delegate_req(dwc, ctrl);
Why do we need to delegate to gadget driver?
We need to check if the host arms the remote wakeup, and we need to
check if the gadget is capable of remote wakeup. For example, it's odd
for mass_storage device to be remote capable.
So we need a flag in usb_gadget for that. Have dwc3 check against that
flag before setting it.
Something like this:
if (dwc->gadget->rw_capable) {
usb_status = USB_INTRF_STAT_FUNC_RW_CAP;
if (dwc->rw_armed)
usb_status |= USB_INTERF_STAT_FUNC_RW;
}
>
> case USB_RECIP_ENDPOINT:
> dep = dwc3_wIndex_to_dep(dwc, ctrl->wIndex);
> @@ -511,13 +513,7 @@ static int dwc3_ep0_handle_intf(struct dwc3 *dwc,
>
> switch (wValue) {
> case USB_INTRF_FUNC_SUSPEND:
> - /*
> - * REVISIT: Ideally we would enable some low power mode here,
> - * however it's unclear what we should be doing here.
> - *
> - * For now, we're not doing anything, just making sure we return
> - * 0 so USB Command Verifier tests pass without any errors.
> - */
> + ret = dwc3_ep0_delegate_req(dwc, ctrl);
Again, why are we delegating this to the gadget driver? This can be done
here as something like this:
if (dwc->gadget->rw_capable && set &&
(wIndex & USB_INTRF_FUNC_SUSPEND_RW))
dwc->rw_armed = 1;
else
dwc->rw_armed = 0;
dwc->rw_selected_interface = wIndex & 0xff;
> break;
> default:
> ret = -EINVAL;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index d6697da..0b2947e 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2357,6 +2357,35 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
> return ret;
> }
>
> +static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int interface_id)
Don't pass this work to the gadget driver. The upperlayer doesn't know
when to send the function wakeup. The dwc3 driver should send the
function wakeup when the device is in the state U0, and the dwc3 driver
gets that info first hand.
> +{
> + int ret = 0;
> + u32 reg;
> + struct dwc3 *dwc = gadget_to_dwc(g);
> +
> + reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> +
> + /*
> + * If the link is in LPM, first bring the link to U0
> + * before triggering function wakeup. Ideally this
> + * needs to be expanded to other LPMs as well in
> + * addition to U3
> + */
> + if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U3) {
You're not doing U0 link check here.
> + dwc3_gadget_wakeup(g);
> + return -EAGAIN;
> + }
> +
> + ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_XMIT_DEV,
> + 0x1 | (interface_id << 4));
The interface_id here is the dwc->rw_selected_interface as noted earlier.
> +
> + if (ret)
> + dev_err(dwc->dev, "Function wakeup HW command failed, ret %d\n",
> + ret);
> +
> + return ret;
> +}
> +
> static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
> int is_selfpowered)
> {
> @@ -2978,6 +3007,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,
> + .func_wakeup = dwc3_gadget_func_wakeup,
> .set_selfpowered = dwc3_gadget_set_selfpowered,
> .pullup = dwc3_gadget_pullup,
> .udc_start = dwc3_gadget_start,
BR,
Thinh
Powered by blists - more mailing lists