We can't depend on the TRB's HWO bit to determine if the TRB ring is "full". A TRB is only available when the driver had processed it, not when the controller consumed and relinquished the TRB's ownership to the driver. Otherwise, the driver may overwrite unprocessed TRBs. This can happen when many transfer events accumulate and the system is slow to process them and/or when there are too many small requests.
If a request is in the started_list, that means there is one or more unprocessed TRBs remained. Check this instead of the TRB's HWO bit whether the TRB ring is full.
Cc: stable@vger.kernel.org Fixes: c4233573f6ee ("usb: dwc3: gadget: prepare TRBs on update transfers too") Signed-off-by: Thinh Nguyen Thinh.Nguyen@synopsys.com --- drivers/usb/dwc3/gadget.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 84fe57ef5a49..1e6ddbc986ba 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -940,19 +940,19 @@ static struct dwc3_trb *dwc3_ep_prev_trb(struct dwc3_ep *dep, u8 index)
static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep) { - struct dwc3_trb *tmp; u8 trbs_left;
/* - * If enqueue & dequeue are equal than it is either full or empty. - * - * One way to know for sure is if the TRB right before us has HWO bit - * set or not. If it has, then we're definitely full and can't fit any - * more transfers in our ring. + * If the enqueue & dequeue are equal then the TRB ring is either full + * or empty. It's considered full when there are DWC3_TRB_NUM-1 of TRBs + * pending to be processed by the driver. */ if (dep->trb_enqueue == dep->trb_dequeue) { - tmp = dwc3_ep_prev_trb(dep, dep->trb_enqueue); - if (tmp->ctrl & DWC3_TRB_CTRL_HWO) + /* + * If there is any request remained in the started_list at + * this point, that means there is no TRB available. + */ + if (!list_empty(&dep->started_list)) return 0;
return DWC3_TRB_NUM - 1;
base-commit: 5571ea3117ca22849072adb58074fb5a2fd12c00
Hi,
Thinh Nguyen Thinh.Nguyen@synopsys.com writes:
We can't depend on the TRB's HWO bit to determine if the TRB ring is "full". A TRB is only available when the driver had processed it, not when the controller consumed and relinquished the TRB's ownership to the driver. Otherwise, the driver may overwrite unprocessed TRBs. This can happen when many transfer events accumulate and the system is slow to process them and/or when there are too many small requests.
If a request is in the started_list, that means there is one or more unprocessed TRBs remained. Check this instead of the TRB's HWO bit whether the TRB ring is full.
Cc: stable@vger.kernel.org Fixes: c4233573f6ee ("usb: dwc3: gadget: prepare TRBs on update transfers too") Signed-off-by: Thinh Nguyen Thinh.Nguyen@synopsys.com
drivers/usb/dwc3/gadget.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 84fe57ef5a49..1e6ddbc986ba 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -940,19 +940,19 @@ static struct dwc3_trb *dwc3_ep_prev_trb(struct dwc3_ep *dep, u8 index) static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep) {
- struct dwc3_trb *tmp; u8 trbs_left;
/*
* If enqueue & dequeue are equal than it is either full or empty.** One way to know for sure is if the TRB right before us has HWO bit* set or not. If it has, then we're definitely full and can't fit any* more transfers in our ring.
* If the enqueue & dequeue are equal then the TRB ring is either full* or empty. It's considered full when there are DWC3_TRB_NUM-1 of TRBs* pending to be processed by the driver.
tmp = dwc3_ep_prev_trb(dep, dep->trb_enqueue);if (tmp->ctrl & DWC3_TRB_CTRL_HWO)
/** If there is any request remained in the started_list at* this point, that means there is no TRB available.*/if (!list_empty(&dep->started_list)) return 0;
we could also do away with calc_trbs_left() completely if we just add an actual counter that gets incremented decremented together with the enqueue/dequeue pointers. Since we have 256 TRBs per endpoint and only 255 are usable, this means we can do away with a single u8 per endpoint. Perhaps that could be done as a second step after this fix is merged?
Felipe Balbi wrote:
Hi,
Thinh Nguyen Thinh.Nguyen@synopsys.com writes:
We can't depend on the TRB's HWO bit to determine if the TRB ring is "full". A TRB is only available when the driver had processed it, not when the controller consumed and relinquished the TRB's ownership to the driver. Otherwise, the driver may overwrite unprocessed TRBs. This can happen when many transfer events accumulate and the system is slow to process them and/or when there are too many small requests.
If a request is in the started_list, that means there is one or more unprocessed TRBs remained. Check this instead of the TRB's HWO bit whether the TRB ring is full.
Cc: stable@vger.kernel.org Fixes: c4233573f6ee ("usb: dwc3: gadget: prepare TRBs on update transfers too") Signed-off-by: Thinh Nguyen Thinh.Nguyen@synopsys.com
drivers/usb/dwc3/gadget.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 84fe57ef5a49..1e6ddbc986ba 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -940,19 +940,19 @@ static struct dwc3_trb *dwc3_ep_prev_trb(struct dwc3_ep *dep, u8 index) static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep) {
- struct dwc3_trb *tmp; u8 trbs_left;
/*
* If enqueue & dequeue are equal than it is either full or empty.** One way to know for sure is if the TRB right before us has HWO bit* set or not. If it has, then we're definitely full and can't fit any* more transfers in our ring.
* If the enqueue & dequeue are equal then the TRB ring is either full* or empty. It's considered full when there are DWC3_TRB_NUM-1 of TRBs* pending to be processed by the driver.
tmp = dwc3_ep_prev_trb(dep, dep->trb_enqueue);if (tmp->ctrl & DWC3_TRB_CTRL_HWO)
/** If there is any request remained in the started_list at* this point, that means there is no TRB available.*/if (!list_empty(&dep->started_list)) return 0;we could also do away with calc_trbs_left() completely if we just add an actual counter that gets incremented decremented together with the enqueue/dequeue pointers. Since we have 256 TRBs per endpoint and only 255 are usable, this means we can do away with a single u8 per endpoint. Perhaps that could be done as a second step after this fix is merged?
Yes, that would simplify the logic. We can do that after merging this fix.
Thanks, Thinh
Thinh Nguyen Thinh.Nguyen@synopsys.com writes:
Felipe Balbi wrote:
Hi,
Thinh Nguyen Thinh.Nguyen@synopsys.com writes:
We can't depend on the TRB's HWO bit to determine if the TRB ring is "full". A TRB is only available when the driver had processed it, not when the controller consumed and relinquished the TRB's ownership to the driver. Otherwise, the driver may overwrite unprocessed TRBs. This can happen when many transfer events accumulate and the system is slow to process them and/or when there are too many small requests.
If a request is in the started_list, that means there is one or more unprocessed TRBs remained. Check this instead of the TRB's HWO bit whether the TRB ring is full.
Cc: stable@vger.kernel.org Fixes: c4233573f6ee ("usb: dwc3: gadget: prepare TRBs on update transfers too") Signed-off-by: Thinh Nguyen Thinh.Nguyen@synopsys.com
drivers/usb/dwc3/gadget.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 84fe57ef5a49..1e6ddbc986ba 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -940,19 +940,19 @@ static struct dwc3_trb *dwc3_ep_prev_trb(struct dwc3_ep *dep, u8 index) static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep) {
- struct dwc3_trb *tmp; u8 trbs_left;
/*
* If enqueue & dequeue are equal than it is either full or empty.** One way to know for sure is if the TRB right before us has HWO bit* set or not. If it has, then we're definitely full and can't fit any* more transfers in our ring.
* If the enqueue & dequeue are equal then the TRB ring is either full* or empty. It's considered full when there are DWC3_TRB_NUM-1 of TRBs* pending to be processed by the driver.
tmp = dwc3_ep_prev_trb(dep, dep->trb_enqueue);if (tmp->ctrl & DWC3_TRB_CTRL_HWO)
/** If there is any request remained in the started_list at* this point, that means there is no TRB available.*/if (!list_empty(&dep->started_list)) return 0;we could also do away with calc_trbs_left() completely if we just add an actual counter that gets incremented decremented together with the enqueue/dequeue pointers. Since we have 256 TRBs per endpoint and only 255 are usable, this means we can do away with a single u8 per endpoint. Perhaps that could be done as a second step after this fix is merged?
Yes, that would simplify the logic. We can do that after merging this fix.
sounds good:
Acked-by: Felipe Balbi balbi@kernel.org
linux-stable-mirror@lists.linaro.org
-
Felipe Balbi -
Thinh Nguyen