xhci-mtk has 64 slots for periodic bandwidth calculations and each
slot represents byte budgets on a microframe. When an endpoint's
allocation sits on the boundary of the table, byte budgets' slot
should be rolled over but the current implementation doesn't.
This patch applies a 6 bits mask to the microframe index to handle
its rollover 64 slots and prevent out-of-bounds array access.
Signed-off-by: Ikjoon Jang <[email protected]>
---
drivers/usb/host/xhci-mtk-sch.c | 79 +++++++++------------------------
drivers/usb/host/xhci-mtk.h | 1 +
2 files changed, 23 insertions(+), 57 deletions(-)
diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
index 46cbf5d54f4f..ef16cd124343 100644
--- a/drivers/usb/host/xhci-mtk-sch.c
+++ b/drivers/usb/host/xhci-mtk-sch.c
@@ -411,18 +411,13 @@ static void setup_sch_info(struct xhci_ep_ctx *ep_ctx,
static u32 get_max_bw(struct mu3h_sch_bw_info *sch_bw,
struct mu3h_sch_ep_info *sch_ep, u32 offset)
{
- u32 num_esit;
- u32 max_bw = 0;
- u32 bw;
- int i;
- int j;
-
- num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
- for (i = 0; i < num_esit; i++) {
- u32 base = offset + i * sch_ep->esit;
+ u32 bw, max_bw = 0;
+ int i, j, idx;
+ for (i = 0; i < XHCI_MTK_MAX_ESIT; i += sch_ep->esit) {
for (j = 0; j < sch_ep->num_budget_microframes; j++) {
- bw = sch_bw->bus_bw[base + j] +
+ idx = XHCI_MTK_BW_IDX(i + offset + j);
+ bw = sch_bw->bus_bw[idx] +
sch_ep->bw_budget_table[j];
if (bw > max_bw)
max_bw = bw;
@@ -434,20 +429,16 @@ static u32 get_max_bw(struct mu3h_sch_bw_info *sch_bw,
static void update_bus_bw(struct mu3h_sch_bw_info *sch_bw,
struct mu3h_sch_ep_info *sch_ep, bool used)
{
- u32 num_esit;
- u32 base;
- int i;
- int j;
+ int i, j, idx;
- num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
- for (i = 0; i < num_esit; i++) {
- base = sch_ep->offset + i * sch_ep->esit;
+ for (i = 0; i < XHCI_MTK_MAX_ESIT; i += sch_ep->esit) {
for (j = 0; j < sch_ep->num_budget_microframes; j++) {
+ idx = XHCI_MTK_BW_IDX(i + sch_ep->offset + j);
if (used)
- sch_bw->bus_bw[base + j] +=
+ sch_bw->bus_bw[idx] +=
sch_ep->bw_budget_table[j];
else
- sch_bw->bus_bw[base + j] -=
+ sch_bw->bus_bw[idx] -=
sch_ep->bw_budget_table[j];
}
}
@@ -456,22 +447,18 @@ static void update_bus_bw(struct mu3h_sch_bw_info *sch_bw,
static int check_fs_bus_bw(struct mu3h_sch_ep_info *sch_ep, int offset)
{
struct mu3h_sch_tt *tt = sch_ep->sch_tt;
- u32 num_esit, tmp;
- int base;
- int i, j;
+ u32 bw;
+ int i, j, idx;
u8 uframes = DIV_ROUND_UP(sch_ep->maxpkt, FS_PAYLOAD_MAX);
- num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
-
if (sch_ep->ep_type == INT_IN_EP || sch_ep->ep_type == ISOC_IN_EP)
offset++;
- for (i = 0; i < num_esit; i++) {
- base = offset + i * sch_ep->esit;
-
+ for (i = 0; i < XHCI_MTK_MAX_ESIT; i += sch_ep->esit) {
for (j = 0; j < uframes; j++) {
- tmp = tt->fs_bus_bw[base + j] + sch_ep->bw_cost_per_microframe;
- if (tmp > FS_PAYLOAD_MAX)
+ idx = XHCI_MTK_BW_IDX(i + offset + j);
+ bw = tt->fs_bus_bw[idx] + sch_ep->bw_cost_per_microframe;
+ if (bw > FS_PAYLOAD_MAX)
return -ESCH_BW_OVERFLOW;
}
}
@@ -544,14 +531,11 @@ static int check_sch_tt(struct mu3h_sch_ep_info *sch_ep, u32 offset)
static void update_sch_tt(struct mu3h_sch_ep_info *sch_ep, bool used)
{
struct mu3h_sch_tt *tt = sch_ep->sch_tt;
- u32 base, num_esit;
int bw_updated;
- int i, j;
+ int i, j, idx;
int offset = sch_ep->offset;
u8 uframes = DIV_ROUND_UP(sch_ep->maxpkt, FS_PAYLOAD_MAX);
- num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
-
if (used)
bw_updated = sch_ep->bw_cost_per_microframe;
else
@@ -560,11 +544,11 @@ static void update_sch_tt(struct mu3h_sch_ep_info *sch_ep, bool used)
if (sch_ep->ep_type == INT_IN_EP || sch_ep->ep_type == ISOC_IN_EP)
offset++;
- for (i = 0; i < num_esit; i++) {
- base = offset + i * sch_ep->esit;
-
- for (j = 0; j < uframes; j++)
- tt->fs_bus_bw[base + j] += bw_updated;
+ for (i = 0; i < XHCI_MTK_MAX_ESIT; i += sch_ep->esit) {
+ for (j = 0; j < uframes; j++) {
+ idx = XHCI_MTK_BW_IDX(i + offset + j);
+ tt->fs_bus_bw[idx] += bw_updated;
+ }
}
if (used)
@@ -586,25 +570,9 @@ static int load_ep_bw(struct mu3h_sch_bw_info *sch_bw,
return 0;
}
-static u32 get_esit_boundary(struct mu3h_sch_ep_info *sch_ep)
-{
- u32 boundary = sch_ep->esit;
-
- if (sch_ep->sch_tt) { /* LS/FS with TT */
- /* tune for CS */
- if (sch_ep->ep_type != ISOC_OUT_EP)
- boundary++;
- else if (boundary > 1) /* normally esit >= 8 for FS/LS */
- boundary--;
- }
-
- return boundary;
-}
-
static int check_sch_bw(struct mu3h_sch_ep_info *sch_ep)
{
struct mu3h_sch_bw_info *sch_bw = sch_ep->bw_info;
- const u32 esit_boundary = get_esit_boundary(sch_ep);
const u32 bw_boundary = get_bw_boundary(sch_ep->speed);
u32 offset;
u32 worst_bw;
@@ -621,9 +589,6 @@ static int check_sch_bw(struct mu3h_sch_ep_info *sch_ep)
if (ret)
continue;
- if ((offset + sch_ep->num_budget_microframes) > esit_boundary)
- break;
-
worst_bw = get_max_bw(sch_bw, sch_ep, offset);
if (worst_bw > bw_boundary)
continue;
diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
index ddcf25524f67..f627941c4860 100644
--- a/drivers/usb/host/xhci-mtk.h
+++ b/drivers/usb/host/xhci-mtk.h
@@ -23,6 +23,7 @@
* bandwidth to it.
*/
#define XHCI_MTK_MAX_ESIT 64
+#define XHCI_MTK_BW_IDX(idx) ((idx) & 63)
/**
* @fs_bus_bw: array to keep track of bandwidth already used for FS
--
2.32.0.605.g8dce9f2422-goog
oops sorry I sent a prior mail in HTML.
Resend this mail in plain text.
On Thu, Aug 12, 2021 at 7:49 PM Chunfeng Yun (云春峰)
<[email protected]> wrote:
>
> On Thu, 2021-08-12 at 17:31 +0800, Ikjoon Jang wrote:
> > HI,
> >
> > On Wed, Aug 11, 2021 at 5:02 PM Chunfeng Yun (云春峰)
> > <[email protected]> wrote:
> > >
> > > On Mon, 2021-08-09 at 17:42 +0800, Ikjoon Jang wrote:
> > > > On Mon, Aug 9, 2021 at 5:11 PM Greg Kroah-Hartman
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Mon, Aug 09, 2021 at 04:59:29PM +0800, Ikjoon Jang wrote:
> > > > > > xhci-mtk has 64 slots for periodic bandwidth calculations and
> > > > > > each
> > > > > > slot represents byte budgets on a microframe. When an
> > > > > > endpoint's
> > > > > > allocation sits on the boundary of the table, byte budgets'
> > > > > > slot
> > > > > > should be rolled over but the current implementation doesn't.
> > > > > >
> > > > > > This patch applies a 6 bits mask to the microframe index to
> > > > > > handle
> > > > > > its rollover 64 slots and prevent out-of-bounds array access.
> > > > > >
> > > > > > Signed-off-by: Ikjoon Jang <[email protected]>
> > > > > > ---
> > > > > >
> > > > > > drivers/usb/host/xhci-mtk-sch.c | 79 +++++++++------------
> > > > > > ----
> > > > > > --------
> > > > > > drivers/usb/host/xhci-mtk.h | 1 +
> > > > > > 2 files changed, 23 insertions(+), 57 deletions(-)
> > > > >
> > > > > Why is this "RFC"? What needs to be addressed in this change
> > > > > before it
> > > > > can be accepted?
> > > >
> > > > sorry, I had to mention why this is RFC:
> > > >
> > > > I simply don't know about the details of the xhci-mtk internals.
> > > > It was okay from my tests with mt8173 and I think this will be
> > > > harmless
> > > > as this is "better than before".
> > > >
> > > > But when I removed get_esit_boundary(), I really have no idea why
> > > > it was there. I'm wondering if there was another reason of that
> > > > function
> > > > other than just preventing out-of-bounds. Maybe chunfeng can
> > > > answer
> > > > this?
> > >
> > > We use @esit to prevent out-of-bounds array access. it's not a
> > > ring,
> > > can't insert out-of-bounds value into head slot.
> >
> > Thanks, so that function was only for out-of-bounds array access.
> > then I think we just can remove that function and use it as a ring.
> > Can you tell me _why_ it can't be used as a ring?
> Treat it as a period, roll over slot equals to put it into the next
> period.
>
> >
> > I think a transaction (e.g. esit_boundary = 7) can start its first
> > SSPLIT
> > from Y_7 (offset = 7). But will that allocation be matched with this?
> >
> > - if ((offset + sch_ep->num_budget_microframes) >
> > esit_boundary)
> > - break;
> >
> > I mean I'm not sure why this is needed.
> Prevent out-of-bounds.
>
> >
> > Until now, I couldn't find a way to accept the USB audio headset
> > with a configuration of { INT-IN 64 + ISOC-OUT 384 + ISOC-IN 192 }
> > without this patch.
> what is the interval value of each endpoint?
interrupt ep is 2ms and others are 1ms
Thanks.
>
> >
> > >
> > > >
> > > > Thanks!
> > > >
> > > > >
> > > > > thanks,
> > > > >
> > > > > greg k-h