Hi,
John Youn <johnyoun@xxxxxxxxxxxx> writes:
> This patch fixes up some issues related to the trb_left calculation.
>
> This calculation sometimes included the link trb slot in the trbs_left
> and sometimes didn't.
good catch. But this patch seems like it can be broken into smaller
pieces. See below
> In the case where the dequeue < enqueue, this number does not include
> the link trb and should be used as-is. Otherwise it will include the
> link_trb and should be decremented to reflect the actual amount of free
> trb slots. The fixed calculation will be the proper amount of free TRB
> slots left, taking into account whether a link TRB takes up one of the
> slots.
this is one fix :-)
> When checking for full or empty where enqueue == dequeue, the pointer
> may be 0. In that case previous TRB is the one before the link TRB. Use
> that to check the HWO bit to see if we are full or empty.
if enqueue == dequeue, we could be anywhere in the ring. So previous TRB
might not be the one before the link. Care to further explain this case?
It's also a separate fix, btw.
> In case DWC3_TRB_NUM is ever set lower than 256, mod trbs_left result by
> DWC3_TRB_NUM.
I don't get this. Where did we have % 256? I can only see % DWC3_TRB_NUM.
> In dwc3_prepare_trbs, if trbs_left is 0, do nothing and return early.
another fix.
> When an EP is enabled, initialized the TRB pool to clean up any stale
> data. When the previous TRB was not properly cleaned up on disconnect,
> the empty/full checking logic sometimes failed causing the EP to stop
> processing requests.
another fix.
> Signed-off-by: John Youn <johnyoun@xxxxxxxxxxxx>
> ---
> drivers/usb/dwc3/gadget.c | 37 ++++++++++++++++++++++++++++++-------
> 1 file changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3eaef22..a76634b 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -561,6 +561,12 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
> if (usb_endpoint_xfer_control(desc))
> goto out;
>
> + /* Initialize the TRB pool */
> + dep->trb_dequeue = 0;
> + dep->trb_enqueue = 0;
> + memset(dep->trb_pool, 0,
> + sizeof(struct dwc3_trb) * DWC3_TRB_NUM);
this is a separate fix. A very good one, actually ;-)
> /* Link TRB. The HWO bit is never reset */
> trb_st_hw = &dep->trb_pool[0];
>
> @@ -849,6 +855,8 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
> static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
> {
> struct dwc3_trb *tmp;
> + u8 tmp_index;
> + u8 trbs_left;
>
> /*
> * If enqueue & dequeue are equal than it is either full or empty.
> @@ -858,17 +866,29 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
> * more transfers in our ring.
> */
> if (dep->trb_enqueue == dep->trb_dequeue) {
> - /* If we're full, enqueue/dequeue are > 0 */
> - if (dep->trb_enqueue) {
> - tmp = &dep->trb_pool[dep->trb_enqueue - 1];
> - if (tmp->ctrl & DWC3_TRB_CTRL_HWO)
> - return 0;
> - }
> + if (!dep->trb_enqueue)
> + /*
> + * The TRB just before the zeroth one is the
> + * one just before the LINK TRB.
> + */
> + tmp_index = DWC3_TRB_NUM - 2;
this seems wrong. If both enqueue and dequeue are 0, it means our entire
ring is empty, and we can use (by default) 255 TRBs, with one space left
for the link. This DWC3_TRB_NUM - 2 will give me 254, instead of 255.
> + else
> + tmp_index = dep->trb_enqueue - 1;
If enqueue and dequeue are equal, but not 0, then we're anywhere in the
middle of the ring, and this is where we're either full or empty. I'm
using HWO of the previous TRB to figure out if we're full or not.
> + tmp = &dep->trb_pool[tmp_index];
> + if (tmp->ctrl & DWC3_TRB_CTRL_HWO)
> + return 0;
>
> return DWC3_TRB_NUM - 1;
> }
>
> - return dep->trb_dequeue - dep->trb_enqueue;
> + trbs_left = dep->trb_dequeue - dep->trb_enqueue;
> + trbs_left %= DWC3_TRB_NUM;
> +
> + if (dep->trb_dequeue < dep->trb_enqueue)
> + trbs_left--;
this branch seems correct, but part of a separate patch.
> +
> + return trbs_left;
> }
>
> static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
> @@ -949,6 +969,9 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep, int starting)
>
> trbs_left = dwc3_calc_trbs_left(dep);
>
> + if (!trbs_left)
> + return;
also a separate patch.
--
balbi
Attachment:
signature.asc
Description: PGP signature
