| 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: <CALAqxLW3W=VEHSqvWQEKgMBwXk=CTBFuXFm_t2or3s2484vrDQ@mail.gmail.com>
Date: Tue, 8 Nov 2016 12:37:19 -0800
From: John Stultz <john.stultz@...aro.org>
To: Felipe Balbi <felipe.balbi@...ux.intel.com>
Cc: lkml <linux-kernel@...r.kernel.org>, Wei Xu <xuwei5@...ilicon.com>,
Guodong Xu <guodong.xu@...aro.org>,
Chen Yu <chenyu56@...wei.com>,
Amit Pundir <amit.pundir@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
John Youn <johnyoun@...opsys.com>,
Douglas Anderson <dianders@...omium.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Linux USB List <linux-usb@...r.kernel.org>
Subject: Re: [RFC][PATCH] usb: dwc2: Make sure we disconnect the gadget state
on reset
On Mon, Oct 31, 2016 at 4:18 AM, Felipe Balbi
<felipe.balbi@...ux.intel.com> wrote:
> John Stultz <john.stultz@...aro.org> writes:
>> I had seen some odd behavior with HiKey's usb-gadget interface
>> that I finally seemed to have chased down. Basically every other
>> time I pluged in the OTG port, the gadget interface would
>> properly initialize. The other times, I'd get a big WARN_ON
>> in dwc2_hsotg_init_fifo() about the fifo_map not being clear.
>>
>> Ends up If we don't disconnect the gadget state on reset, the
>> fifo-map doesn't get cleared properly, which causes WARN_ON
>> messages and also results in the device not properly being
>> setup as a gadget every other time the OTG port is connected.
>>
>> So this patch adds a call to dwc2_hsotg_disconnect() in the
>> reset path so the state is properly cleared.
>>
>> With it, the gadget interface initializes properly on every
>> plug in.
>>
>> I don't know if this is actually the right fix, but it seems
>> to work well. Feedback would be greatly appreciated!
>>
[snip]
>> ---
>> drivers/usb/dwc2/gadget.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 24fbebc..5505001 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -2519,6 +2519,8 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
>>
>> /* Kill any ep0 requests as controller will be reinitialized */
>> kill_all_requests(hsotg, hsotg->eps_out[0], -ECONNRESET);
>> + /* Make sure everything is disconnected */
>> + dwc2_hsotg_disconnect(hsotg);
>
> Dunno, seems like you're actually working around a different
> bug. Looking at USB Reset handler we have:
>
> if (gintsts & (GINTSTS_USBRST | GINTSTS_RESETDET)) {
>
> u32 usb_status = dwc2_readl(hsotg->regs + GOTGCTL);
> u32 connected = hsotg->connected;
>
> dev_dbg(hsotg->dev, "%s: USBRst\n", __func__);
> dev_dbg(hsotg->dev, "GNPTXSTS=%08x\n",
> dwc2_readl(hsotg->regs + GNPTXSTS));
>
> dwc2_writel(GINTSTS_USBRST, hsotg->regs + GINTSTS);
>
> /* Report disconnection if it is not already done. */
> dwc2_hsotg_disconnect(hsotg);
>
> if (usb_status & GOTGCTL_BSESVLD && connected)
> dwc2_hsotg_core_init_disconnected(hsotg, true);
> }
>
> so, dwc2_hsotg_disconnect() is already called from Reset path. What
> you're doing here is that you could, potentially, call
> driver->disconnect() twice.
>
> The real problem could be your implementation for ->pullup() which
> duplicates part of what ->udc_start() does:
>
> static int dwc2_hsotg_pullup(struct usb_gadget *gadget, int is_on)
> {
> struct dwc2_hsotg *hsotg = to_hsotg(gadget);
> unsigned long flags = 0;
>
> dev_dbg(hsotg->dev, "%s: is_on: %d op_state: %d\n", __func__, is_on,
> hsotg->op_state);
>
> /* Don't modify pullup state while in host mode */
> if (hsotg->op_state != OTG_STATE_B_PERIPHERAL) {
> hsotg->enabled = is_on;
> return 0;
> }
>
> spin_lock_irqsave(&hsotg->lock, flags);
> if (is_on) {
> hsotg->enabled = 1;
> dwc2_hsotg_core_init_disconnected(hsotg, false);
> dwc2_hsotg_core_connect(hsotg);
> } else {
> dwc2_hsotg_core_disconnect(hsotg);
> dwc2_hsotg_disconnect(hsotg);
> hsotg->enabled = 0;
> }
>
> hsotg->gadget.speed = USB_SPEED_UNKNOWN;
> spin_unlock_irqrestore(&hsotg->lock, flags);
>
> return 0;
> }
>
> Here's what I think dwc2_hsotg_pullup() and dwc2_hsotg_udc_start()
> should contain:
>
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 9dc6c482b89e..dbe28947d3a9 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3388,6 +3388,7 @@ static void dwc2_hsotg_init(struct dwc2_hsotg *hsotg)
> dwc2_writel(0, hsotg->regs + DAINTMSK);
>
> /* Be in disconnected state until gadget is registered */
> + /* REVISIT!!!! This should be done in probe()!!!! */
> __orr32(hsotg->regs + DCTL, DCTL_SFTDISCON);
>
> /* setup fifos */
> @@ -3428,26 +3429,6 @@ static int dwc2_hsotg_udc_start(struct usb_gadget *gadget,
> unsigned long flags;
> int ret;
>
> - if (!hsotg) {
> - pr_err("%s: called with no device\n", __func__);
> - return -ENODEV;
> - }
> -
> - if (!driver) {
> - dev_err(hsotg->dev, "%s: no driver\n", __func__);
> - return -EINVAL;
> - }
> -
> - if (driver->max_speed < USB_SPEED_FULL)
> - dev_err(hsotg->dev, "%s: bad speed\n", __func__);
> -
> - if (!driver->setup) {
> - dev_err(hsotg->dev, "%s: missing entry points\n", __func__);
> - return -EINVAL;
> - }
> -
> - WARN_ON(hsotg->driver);
> -
> driver->driver.bus = NULL;
> hsotg->driver = driver;
> hsotg->gadget.dev.of_node = hsotg->dev->of_node;
> @@ -3550,17 +3531,15 @@ static int dwc2_hsotg_pullup(struct usb_gadget *gadget, int is_on)
> /* Don't modify pullup state while in host mode */
> if (hsotg->op_state != OTG_STATE_B_PERIPHERAL) {
> hsotg->enabled = is_on;
> - return 0;
> + return -EINVAL;
> }
>
> spin_lock_irqsave(&hsotg->lock, flags);
> if (is_on) {
> hsotg->enabled = 1;
> - dwc2_hsotg_core_init_disconnected(hsotg, false);
> dwc2_hsotg_core_connect(hsotg);
> } else {
> dwc2_hsotg_core_disconnect(hsotg);
> - dwc2_hsotg_disconnect(hsotg);
> hsotg->enabled = 0;
> }
So I reverted my proposed change and gave the above patch a try on my
HiKey device, but this didn't change the issue I'm seeing. I still
get WARN_ONs in dwc2_hsotg_init_fifo() about the fifo_map not being
clear. Adding my proposed change still helps this issue for me.
Though I do see the potential for calling dwc2_hsotg_disconnect()
twice as its already called in the dwc2_hsotg_irq() path before
calling dwc2_hsotg_core_init_disconnected.
But if its of more help, the error I'm seeing seems to be triggered
from the dwc2_conn_id_status_change() code path calling
dwc2_hsotg_core_init_disconnected().
Would the proper fix to be calling dwc2_hsotg_disconnect() from
dwc2_conn_id_status_change() instead?
thanks
-john
Powered by blists - more mailing lists