| 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: Thu, 13 Oct 2016 19:16:44 +0800
From: Baolin Wang <baolin.wang@...aro.org>
To: Felipe Balbi <balbi@...nel.org>
Cc: Greg KH <gregkh@...uxfoundation.org>,
Mark Brown <broonie@...nel.org>,
USB <linux-usb@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] usb: dwc3: gadget: Wait for end transfer complete
before free irq
On 13 October 2016 at 18:56, Felipe Balbi <balbi@...nel.org> wrote:
>
> Hi,
>
> Felipe Balbi <balbi@...nel.org> writes:
>> Hi,
>>
>> Baolin Wang <baolin.wang@...aro.org> writes:
>>>>>> I'm thinking this is a bug in configfs interface of Gadget API, not
>>>>>> dwc3. The only reason for this to happen would be if we get to
>>>>>> ->udc_stop() with endpoints still enabled.
>>>>>>
>>>>>> Can you check if that's the case? i.e. can you check if any endpoints
>>>>>> are still enabled when we get here?
>>>>>
>>>>> No, it is not any endpoints are still enabled. Like I said in commit
>>>>> message, we will start end transfer command when disable the endpoint,
>>>>> if the end transfer command complete event comes after we have freed
>>>>> the gadget irq, it will trigger the interrupt line all the time to
>>>>> crash the system.
>>>>
>>>> I see what the problem is. Databook tells us we *must* set CMDIOC when
>>>> issuing EndTransfer command and we should always wait for Command
>>>> Complete IRQ. However, we took a shortcut and just delayed for 100us
>>>> after issuing End Transfer.
>>>
>>> Yes, but 100us delay is not enough in some scenarios, like changing
>>> function with configfs frequently, we will met problems.
>>
>> heh, 100us *is* enough. However we still get an IRQ because we requested
>> for it. If you want to fix this, then you need to find a way to get rid
>> of that 100us udelay() and add a proper wait_for_completion() to delay
>> execution until command complete IRQ fires up.
>
> I haven't tested this yet, but it shows the idea (I think we might still
> have a race with ep_queue if we're still waiting for End Transfer, but
Yes, maybe we need check DWC3_EP_END_TRANSFER_PENDING flag when
queuing one request.
> that's easy to sort out by checking for DWC3_EP_END_TRANSFER_PENDING
> before calling kick_transfer). Could you have a look and, perhaps, run a
> test?
Sure. I will test it and send out the result tomorrow.
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index e878366ead00..24a77e9f9bba 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -26,6 +26,7 @@
> #include <linux/dma-mapping.h>
> #include <linux/mm.h>
> #include <linux/debugfs.h>
> +#include <linux/wait.h>
>
> #include <linux/usb/ch9.h>
> #include <linux/usb/gadget.h>
> @@ -504,6 +505,7 @@ struct dwc3_event_buffer {
> * @endpoint: usb endpoint
> * @pending_list: list of pending requests for this endpoint
> * @started_list: list of started requests on this endpoint
> + * @wait_end_transfer: wait_queue_head_t for waiting on End Transfer complete
> * @lock: spinlock for endpoint request queue traversal
> * @regs: pointer to first endpoint register
> * @trb_pool: array of transaction buffers
> @@ -529,6 +531,8 @@ struct dwc3_ep {
> struct list_head pending_list;
> struct list_head started_list;
>
> + wait_queue_head_t wait_end_transfer;
> +
> spinlock_t lock;
> void __iomem *regs;
>
> @@ -545,7 +549,7 @@ struct dwc3_ep {
> #define DWC3_EP_BUSY (1 << 4)
> #define DWC3_EP_PENDING_REQUEST (1 << 5)
> #define DWC3_EP_MISSED_ISOC (1 << 6)
> -
> +#define DWC3_EP_END_TRANSFER_PENDING (1 << 7)
> /* This last one is specific to EP0 */
> #define DWC3_EP0_DIR_IN (1 << 31)
>
> @@ -1047,6 +1051,9 @@ struct dwc3_event_depevt {
> #define DEPEVT_TRANSFER_BUS_EXPIRY 2
>
> u32 parameters:16;
> +
> +/* For Command Complete Events */
> +#define DEPEVT_PARAMETER_CMD(n) (((n) & (0xf << 24)) >> 24)
> } __packed;
>
> /**
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3ba05b12e49a..8037bff43485 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -598,6 +598,8 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
> reg |= DWC3_DALEPENA_EP(dep->number);
> dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
>
> + init_waitqueue_head(&dep->wait_end_transfer);
> +
> if (usb_endpoint_xfer_control(desc))
> return 0;
>
> @@ -2156,6 +2158,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
> {
> struct dwc3_ep *dep;
> u8 epnum = event->endpoint_number;
> + u8 cmd;
>
> dep = dwc->eps[epnum];
>
> @@ -2200,8 +2203,13 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
> return;
> }
> break;
> - case DWC3_DEPEVT_RXTXFIFOEVT:
> case DWC3_DEPEVT_EPCMDCMPLT:
> + cmd = DEPEVT_PARAMETER_CMD(event->parameters);
> +
> + if (cmd == DWC3_DEPCMD_ENDTRANSFER)
> + dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
> + break;
> + case DWC3_DEPEVT_RXTXFIFOEVT:
> break;
> }
> }
> @@ -2246,6 +2254,7 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
> }
>
> static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force)
> +__releases(&dwc->lock) __acquires(&dwc->lock)
> {
> struct dwc3_ep *dep;
> struct dwc3_gadget_ep_cmd_params params;
> @@ -2295,6 +2304,12 @@ static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force)
> memset(¶ms, 0, sizeof(params));
> ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
> WARN_ON_ONCE(ret);
> +
> + dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
> + wait_event_cmd(dep->wait_end_transfer,
> + !(dep->flags & DWC3_EP_END_TRANSFER_PENDING),
> + spin_unlock_irq(&dwc->lock), spin_lock_irq(&dwc->lock));
> +
> dep->resource_index = 0;
> dep->flags &= ~DWC3_EP_BUSY;
>
>
>
>
> --
> balbi
--
Baolin.wang
Best Regards
Powered by blists - more mailing lists