linux-kernel - Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 7 Dec 2018 09:06:33 +0000
From:   Minas Harutyunyan <minas.harutyunyan@...opsys.com>
To:     Marek Szyprowski <m.szyprowski@...sung.com>,
        Minas Harutyunyan <minas.harutyunyan@...opsys.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Maynard CABIENTE <maynard.cabiente@...itan.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Subject: Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on
 disconnect"

Hi Marek,

On 12/6/2018 7:04 PM, Marek Szyprowski wrote:
> Dear Minas,
> 
> On 2018-12-04 13:34, Minas Harutyunyan wrote:
>> On 11/23/2018 6:43 PM, Dan Carpenter wrote:
>>> Ugh...  We also had a long thread about the v2 patch but it turns out
>>> the list was not CC'd.  I should have checked for that.
>>>
>>> You have to pass a flag to say if the caller holds the lock or not...
>>>
>>> regards,
>>> dan carpenter
>>>
>> Hi Dan, Marek, Maynard,
>>
>> Could you please apply bellow patch over follow patches:
>>
>> dccf1bad4be7 usb: dwc2: Disable all EP's on disconnect
>> 6f774b501928 usb: dwc2: Fix ep disable spinlock flow.
>>
>> Please review and test. Feedback is appreciated :-)
> 
> Okay, I finally managed to find some time to check this. Your diff is
> mangled, so I had to manually apply it. Frankly, it is very similar to
> the revert I proposed. I've checked it on my test machines and it fixes
> the issues. I'm not very happy about the unlock/lock design, but it
> should be safe in this case and doesn't make the code even more complex.
> Feel free to add a following tag to the final patch:
> 
> Tested-by: Marek Szyprowski <m.szyprowski@...sung.com>
Thanks for testing.

> 
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 8eb25e3597b0..db438d13c36a 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -3165,8 +3165,6 @@ static void kill_all_requests(struct dwc2_hsotg
>> *hsotg,
>>                   dwc2_hsotg_txfifo_flush(hsotg, ep->fifo_index);
>>    }
>>
>> -static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>> -
>>    /**
>>     * dwc2_hsotg_disconnect - disconnect service
>>     * @hsotg: The device state.
>> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
>>           hsotg->connected = 0;
>>           hsotg->test_mode = 0;
>>
>> -       /* all endpoints should be shutdown */
>>           for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>>                   if (hsotg->eps_in[ep])
>> -                       dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>> +                       kill_all_requests(hsotg, hsotg->eps_in[ep],
>> +                                                         -ESHUTDOWN);
>>                   if (hsotg->eps_out[ep])
>> -                       dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>> +                       kill_all_requests(hsotg, hsotg->eps_out[ep],
>> +                                                         -ESHUTDOWN);
>>           }
>>
>>           call_gadget(hsotg, disconnect);
>> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
>> dwc2_hsotg *hsotg, bool periodic)
>>                           GINTSTS_PTXFEMP |  \
>>                           GINTSTS_RXFLVL)
>>
>> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>> +
>>    /**
>>     * dwc2_hsotg_core_init - issue softreset to the core
>>     * @hsotg: The device state
>> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
>> dwc2_hsotg *hsotg,
>>                           return;
>>           } else {
>>                   /* all endpoints should be shutdown */
>> +               spin_unlock(&hsotg->lock);
>>                   for (ep = 1; ep < hsotg->num_of_eps; ep++) {
>>                           if (hsotg->eps_in[ep])
>>   
>> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>                           if (hsotg->eps_out[ep])
>>   
>> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>                   }
>> +               spin_lock(&hsotg->lock);
>>           }
>>
>>           /*
>> @@ -4072,7 +4075,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>>           unsigned long flags;
>>           u32 epctrl_reg;
>>           u32 ctrl;
>> -       int locked;
>>
>>           dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
>>
>> @@ -4088,7 +4090,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>>
>>           epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>>
>> -       locked = spin_trylock_irqsave(&hsotg->lock, flags);
>> +       spin_lock_irqsave(&hsotg->lock, flags);
>>
>>           ctrl = dwc2_readl(hsotg, epctrl_reg);
>>
>> @@ -4112,8 +4114,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>>           hs_ep->fifo_index = 0;
>>           hs_ep->fifo_size = 0;
>>
>> -       if (locked)
>> -               spin_unlock_irqrestore(&hsotg->lock, flags);
>> +       spin_unlock_irqrestore(&hsotg->lock, flags);
>>
>>           return 0;
>>    }
>>
>> Thanks,
>> Minas
>>
>>
> Best regards
> 

Thanks,
Minas