* [PATCH next v2 2/6] Revert "usb: xhci-mtk: Do not use xhci's virt_dev in drop_endpoint"
2021-08-26 2:51 [PATCH next v2 1/6] Revert "usb: xhci-mtk: relax TT periodic bandwidth allocation" Chunfeng Yun
@ 2021-08-26 2:51 ` Chunfeng Yun
2021-08-26 2:51 ` [PATCH next v2 3/6] usb: xhci-mtk: update fs bus bandwidth by bw_budget_table Chunfeng Yun
` (4 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Chunfeng Yun @ 2021-08-26 2:51 UTC (permalink / raw)
To: Mathias Nyman, Greg Kroah-Hartman
Cc: Chunfeng Yun, Matthias Brugger, linux-usb, linux-arm-kernel,
linux-mediatek, linux-kernel, Ikjoon Jang, Eddie Hung, Yaqii wu
I find the patch introduce some issues, e.g.
1. oops happens when xhci_gen_setup() failed, and hash is not init
but try to destroy it;
2. memory leakage happens when fail to insert ep, need free sch_ep,
or insert ep after insert int list;
3. memory leakage happens when fail to allocate sch_array, need destroy
rhashtable;
4. it's better to check ep->hcpriv when drop ep;
so prefer to revert this patch, and resend it after the issues are fixed.
This reverts commit b8731209958a1dffccc2888121f4c0280c990550.
Cc: Ikjoon Jang <ikjn@chromium.org>
Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v2: append new version patch of this one [5/6] suggested by Ikjoon
---
drivers/usb/host/xhci-mtk-sch.c | 140 ++++++++++++++------------------
drivers/usb/host/xhci-mtk.h | 15 ++--
2 files changed, 69 insertions(+), 86 deletions(-)
diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
index f9b4d27ce449..cffcaf4dfa9f 100644
--- a/drivers/usb/host/xhci-mtk-sch.c
+++ b/drivers/usb/host/xhci-mtk-sch.c
@@ -80,7 +80,7 @@ decode_ep(struct usb_host_endpoint *ep, enum usb_device_speed speed)
interval /= 1000;
}
- snprintf(buf, DBG_BUF_EN, "%s ep%d%s %s, mpkt:%d, interval:%d/%d%s",
+ snprintf(buf, DBG_BUF_EN, "%s ep%d%s %s, mpkt:%d, interval:%d/%d%s\n",
usb_speed_string(speed), usb_endpoint_num(epd),
usb_endpoint_dir_in(epd) ? "in" : "out",
usb_ep_type_string(usb_endpoint_type(epd)),
@@ -129,12 +129,6 @@ get_bw_info(struct xhci_hcd_mtk *mtk, struct usb_device *udev,
int bw_index;
virt_dev = xhci->devs[udev->slot_id];
- if (WARN_ONCE(!virt_dev, "xhci-mtk: usb %s has no virt_dev\n",
- dev_name(&udev->dev)))
- return NULL;
- if (WARN_ONCE(!virt_dev->real_port, "xhci-mtk: usb %s has invalid port number\n",
- dev_name(&udev->dev)))
- return NULL;
if (udev->speed >= USB_SPEED_SUPER) {
if (usb_endpoint_dir_out(&ep->desc))
@@ -200,6 +194,7 @@ static struct mu3h_sch_tt *find_tt(struct usb_device *udev)
}
return ERR_PTR(-ENOMEM);
}
+ INIT_LIST_HEAD(&tt->ep_list);
*ptt = tt;
}
@@ -229,7 +224,7 @@ static void drop_tt(struct usb_device *udev)
}
tt = *ptt;
- if (!tt || tt->nr_eps > 0)
+ if (!tt || !list_empty(&tt->ep_list))
return; /* never allocated , or still in use*/
*ptt = NULL;
@@ -241,19 +236,13 @@ static void drop_tt(struct usb_device *udev)
}
}
-static struct mu3h_sch_ep_info *create_sch_ep(struct xhci_hcd_mtk *mtk,
- struct usb_device *udev, struct usb_host_endpoint *ep,
- struct xhci_ep_ctx *ep_ctx)
+static struct mu3h_sch_ep_info *create_sch_ep(struct usb_device *udev,
+ struct usb_host_endpoint *ep, struct xhci_ep_ctx *ep_ctx)
{
struct mu3h_sch_ep_info *sch_ep;
struct mu3h_sch_tt *tt = NULL;
u32 len_bw_budget_table;
size_t mem_size;
- struct mu3h_sch_bw_info *bw_info;
-
- bw_info = get_bw_info(mtk, udev, ep);
- if (!bw_info)
- return ERR_PTR(-ENODEV);
if (is_fs_or_ls(udev->speed))
len_bw_budget_table = TT_MICROFRAMES_MAX;
@@ -277,10 +266,11 @@ static struct mu3h_sch_ep_info *create_sch_ep(struct xhci_hcd_mtk *mtk,
}
}
- sch_ep->bw_info = bw_info;
sch_ep->sch_tt = tt;
sch_ep->ep = ep;
sch_ep->speed = udev->speed;
+ INIT_LIST_HEAD(&sch_ep->endpoint);
+ INIT_LIST_HEAD(&sch_ep->tt_endpoint);
return sch_ep;
}
@@ -562,9 +552,9 @@ static void update_sch_tt(struct mu3h_sch_ep_info *sch_ep, bool used)
}
if (used)
- tt->nr_eps++;
- else if (!WARN_ONCE(tt->nr_eps < 1, "unbalanced sch_tt's ep count"))
- tt->nr_eps--;
+ list_add_tail(&sch_ep->tt_endpoint, &tt->ep_list);
+ else
+ list_del(&sch_ep->tt_endpoint);
}
static int load_ep_bw(struct mu3h_sch_bw_info *sch_bw,
@@ -595,9 +585,9 @@ static u32 get_esit_boundary(struct mu3h_sch_ep_info *sch_ep)
return boundary;
}
-static int check_sch_bw(struct mu3h_sch_ep_info *sch_ep)
+static int check_sch_bw(struct mu3h_sch_bw_info *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;
@@ -643,33 +633,23 @@ static int check_sch_bw(struct mu3h_sch_ep_info *sch_ep)
return load_ep_bw(sch_bw, sch_ep, true);
}
-static const struct rhashtable_params sch_ep_table_param = {
- .key_len = sizeof(struct usb_host_endpoint *),
- .key_offset = offsetof(struct mu3h_sch_ep_info, ep),
- .head_offset = offsetof(struct mu3h_sch_ep_info, ep_link),
-};
-
-static void destroy_sch_ep(struct xhci_hcd_mtk *mtk, struct usb_device *udev,
- struct mu3h_sch_ep_info *sch_ep)
+static void destroy_sch_ep(struct usb_device *udev,
+ struct mu3h_sch_bw_info *sch_bw, struct mu3h_sch_ep_info *sch_ep)
{
/* only release ep bw check passed by check_sch_bw() */
if (sch_ep->allocated)
- load_ep_bw(sch_ep->bw_info, sch_ep, false);
+ load_ep_bw(sch_bw, sch_ep, false);
if (sch_ep->sch_tt)
drop_tt(udev);
list_del(&sch_ep->endpoint);
- rhashtable_remove_fast(&mtk->sch_ep_table, &sch_ep->ep_link,
- sch_ep_table_param);
kfree(sch_ep);
}
-static bool need_bw_sch(struct usb_device *udev,
- struct usb_host_endpoint *ep)
+static bool need_bw_sch(struct usb_host_endpoint *ep,
+ enum usb_device_speed speed, int has_tt)
{
- bool has_tt = udev->tt && udev->tt->hub->parent;
-
/* only for periodic endpoints */
if (usb_endpoint_xfer_control(&ep->desc)
|| usb_endpoint_xfer_bulk(&ep->desc))
@@ -680,7 +660,7 @@ static bool need_bw_sch(struct usb_device *udev,
* a TT are also ignored, root-hub will schedule them directly,
* but need set @bpkts field of endpoint context to 1.
*/
- if (is_fs_or_ls(udev->speed) && !has_tt)
+ if (is_fs_or_ls(speed) && !has_tt)
return false;
/* skip endpoint with zero maxpkt */
@@ -695,12 +675,7 @@ int xhci_mtk_sch_init(struct xhci_hcd_mtk *mtk)
struct xhci_hcd *xhci = hcd_to_xhci(mtk->hcd);
struct mu3h_sch_bw_info *sch_array;
int num_usb_bus;
- int ret;
-
- /* mu3h_sch_ep_info table, 'usb_host_endpoint*' as a key */
- ret = rhashtable_init(&mtk->sch_ep_table, &sch_ep_table_param);
- if (ret)
- return ret;
+ int i;
/* ss IN and OUT are separated */
num_usb_bus = xhci->usb3_rhub.num_ports * 2 + xhci->usb2_rhub.num_ports;
@@ -709,6 +684,9 @@ int xhci_mtk_sch_init(struct xhci_hcd_mtk *mtk)
if (sch_array == NULL)
return -ENOMEM;
+ for (i = 0; i < num_usb_bus; i++)
+ INIT_LIST_HEAD(&sch_array[i].bw_ep_list);
+
mtk->sch_array = sch_array;
INIT_LIST_HEAD(&mtk->bw_ep_chk_list);
@@ -718,7 +696,6 @@ int xhci_mtk_sch_init(struct xhci_hcd_mtk *mtk)
void xhci_mtk_sch_exit(struct xhci_hcd_mtk *mtk)
{
- rhashtable_destroy(&mtk->sch_ep_table);
kfree(mtk->sch_array);
}
@@ -736,7 +713,9 @@ static int add_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
ep_index = xhci_get_endpoint_index(&ep->desc);
ep_ctx = xhci_get_ep_ctx(xhci, virt_dev->in_ctx, ep_index);
- if (!need_bw_sch(udev, ep)) {
+ xhci_dbg(xhci, "%s %s\n", __func__, decode_ep(ep, udev->speed));
+
+ if (!need_bw_sch(ep, udev->speed, !!virt_dev->tt_info)) {
/*
* set @bpkts to 1 if it is LS or FS periodic endpoint, and its
* device does not connected through an external HS hub
@@ -748,22 +727,14 @@ static int add_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
return 0;
}
- xhci_dbg(xhci, "%s %s\n", __func__, decode_ep(ep, udev->speed));
-
- sch_ep = create_sch_ep(mtk, udev, ep, ep_ctx);
+ sch_ep = create_sch_ep(udev, ep, ep_ctx);
if (IS_ERR_OR_NULL(sch_ep))
return -ENOMEM;
- if (rhashtable_insert_fast(&mtk->sch_ep_table, &sch_ep->ep_link,
- sch_ep_table_param))
- return -EEXIST;
-
setup_sch_info(ep_ctx, sch_ep);
list_add_tail(&sch_ep->endpoint, &mtk->bw_ep_chk_list);
- xhci_dbg(xhci, "added sch_ep %p : %p\n", sch_ep, ep);
-
return 0;
}
@@ -772,20 +743,25 @@ static void drop_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
{
struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
- struct mu3h_sch_ep_info *sch_ep;
- void *key = ep;
+ struct xhci_virt_device *virt_dev;
+ struct mu3h_sch_bw_info *sch_bw;
+ struct mu3h_sch_ep_info *sch_ep, *tmp;
- if (!need_bw_sch(udev, ep))
- return;
+ virt_dev = xhci->devs[udev->slot_id];
xhci_dbg(xhci, "%s %s\n", __func__, decode_ep(ep, udev->speed));
- sch_ep = rhashtable_lookup_fast(&mtk->sch_ep_table, &key,
- sch_ep_table_param);
- if (sch_ep)
- destroy_sch_ep(mtk, udev, sch_ep);
- else
- xhci_dbg(xhci, "ep %p is not on the bw table\n", ep);
+ if (!need_bw_sch(ep, udev->speed, !!virt_dev->tt_info))
+ return;
+
+ sch_bw = get_bw_info(mtk, udev, ep);
+
+ list_for_each_entry_safe(sch_ep, tmp, &sch_bw->bw_ep_list, endpoint) {
+ if (sch_ep->ep == ep) {
+ destroy_sch_ep(udev, sch_bw, sch_ep);
+ break;
+ }
+ }
}
int xhci_mtk_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
@@ -793,22 +769,30 @@ int xhci_mtk_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
struct xhci_virt_device *virt_dev = xhci->devs[udev->slot_id];
- struct mu3h_sch_ep_info *sch_ep;
+ struct mu3h_sch_bw_info *sch_bw;
+ struct mu3h_sch_ep_info *sch_ep, *tmp;
int ret;
xhci_dbg(xhci, "%s() udev %s\n", __func__, dev_name(&udev->dev));
list_for_each_entry(sch_ep, &mtk->bw_ep_chk_list, endpoint) {
- struct xhci_ep_ctx *ep_ctx;
- struct usb_host_endpoint *ep = sch_ep->ep;
- unsigned int ep_index = xhci_get_endpoint_index(&ep->desc);
+ sch_bw = get_bw_info(mtk, udev, sch_ep->ep);
- ret = check_sch_bw(sch_ep);
+ ret = check_sch_bw(sch_bw, sch_ep);
if (ret) {
xhci_err(xhci, "Not enough bandwidth! (%s)\n",
sch_error_string(-ret));
return -ENOSPC;
}
+ }
+
+ list_for_each_entry_safe(sch_ep, tmp, &mtk->bw_ep_chk_list, endpoint) {
+ struct xhci_ep_ctx *ep_ctx;
+ struct usb_host_endpoint *ep = sch_ep->ep;
+ unsigned int ep_index = xhci_get_endpoint_index(&ep->desc);
+
+ sch_bw = get_bw_info(mtk, udev, ep);
+ list_move_tail(&sch_ep->endpoint, &sch_bw->bw_ep_list);
ep_ctx = xhci_get_ep_ctx(xhci, virt_dev->in_ctx, ep_index);
ep_ctx->reserved[0] = cpu_to_le32(EP_BPKTS(sch_ep->pkts)
@@ -822,23 +806,22 @@ int xhci_mtk_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
sch_ep->offset, sch_ep->repeat);
}
- ret = xhci_check_bandwidth(hcd, udev);
- if (!ret)
- INIT_LIST_HEAD(&mtk->bw_ep_chk_list);
-
- return ret;
+ return xhci_check_bandwidth(hcd, udev);
}
void xhci_mtk_reset_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
{
struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+ struct mu3h_sch_bw_info *sch_bw;
struct mu3h_sch_ep_info *sch_ep, *tmp;
xhci_dbg(xhci, "%s() udev %s\n", __func__, dev_name(&udev->dev));
- list_for_each_entry_safe(sch_ep, tmp, &mtk->bw_ep_chk_list, endpoint)
- destroy_sch_ep(mtk, udev, sch_ep);
+ list_for_each_entry_safe(sch_ep, tmp, &mtk->bw_ep_chk_list, endpoint) {
+ sch_bw = get_bw_info(mtk, udev, sch_ep->ep);
+ destroy_sch_ep(udev, sch_bw, sch_ep);
+ }
xhci_reset_bandwidth(hcd, udev);
}
@@ -867,7 +850,8 @@ int xhci_mtk_drop_ep(struct usb_hcd *hcd, struct usb_device *udev,
if (ret)
return ret;
- drop_ep_quirk(hcd, udev, ep);
+ if (ep->hcpriv)
+ drop_ep_quirk(hcd, udev, ep);
return 0;
}
diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
index ddcf25524f67..ace432356c41 100644
--- a/drivers/usb/host/xhci-mtk.h
+++ b/drivers/usb/host/xhci-mtk.h
@@ -10,7 +10,6 @@
#define _XHCI_MTK_H_
#include <linux/clk.h>
-#include <linux/rhashtable.h>
#include "xhci.h"
@@ -26,34 +25,36 @@
/**
* @fs_bus_bw: array to keep track of bandwidth already used for FS
- * @nr_eps: number of endpoints using this TT
+ * @ep_list: Endpoints using this TT
*/
struct mu3h_sch_tt {
u32 fs_bus_bw[XHCI_MTK_MAX_ESIT];
- int nr_eps;
+ struct list_head ep_list;
};
/**
* struct mu3h_sch_bw_info: schedule information for bandwidth domain
*
* @bus_bw: array to keep track of bandwidth already used at each uframes
+ * @bw_ep_list: eps in the bandwidth domain
*
* treat a HS root port as a bandwidth domain, but treat a SS root port as
* two bandwidth domains, one for IN eps and another for OUT eps.
*/
struct mu3h_sch_bw_info {
u32 bus_bw[XHCI_MTK_MAX_ESIT];
+ struct list_head bw_ep_list;
};
/**
* struct mu3h_sch_ep_info: schedule information for endpoint
*
- * @bw_info: bandwidth domain which this endpoint belongs
* @esit: unit is 125us, equal to 2 << Interval field in ep-context
* @num_budget_microframes: number of continuous uframes
* (@repeat==1) scheduled within the interval
* @bw_cost_per_microframe: bandwidth cost per microframe
- * @endpoint: linked into bw_ep_chk_list, used by check_bandwidth hook
+ * @endpoint: linked into bandwidth domain which it belongs to
+ * @tt_endpoint: linked into mu3h_sch_tt's list which it belongs to
* @sch_tt: mu3h_sch_tt linked into
* @ep_type: endpoint type
* @maxpkt: max packet size of endpoint
@@ -81,12 +82,11 @@ struct mu3h_sch_ep_info {
u32 num_budget_microframes;
u32 bw_cost_per_microframe;
struct list_head endpoint;
- struct mu3h_sch_bw_info *bw_info;
+ struct list_head tt_endpoint;
struct mu3h_sch_tt *sch_tt;
u32 ep_type;
u32 maxpkt;
struct usb_host_endpoint *ep;
- struct rhash_head ep_link;
enum usb_device_speed speed;
bool allocated;
/*
@@ -134,7 +134,6 @@ struct xhci_hcd_mtk {
struct device *dev;
struct usb_hcd *hcd;
struct mu3h_sch_bw_info *sch_array;
- struct rhashtable sch_ep_table;
struct list_head bw_ep_chk_list;
struct mu3c_ippc_regs __iomem *ippc_regs;
int num_u2_ports;
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH next v2 3/6] usb: xhci-mtk: update fs bus bandwidth by bw_budget_table
2021-08-26 2:51 [PATCH next v2 1/6] Revert "usb: xhci-mtk: relax TT periodic bandwidth allocation" Chunfeng Yun
2021-08-26 2:51 ` [PATCH next v2 2/6] Revert "usb: xhci-mtk: Do not use xhci's virt_dev in drop_endpoint" Chunfeng Yun
@ 2021-08-26 2:51 ` Chunfeng Yun
2021-08-26 11:54 ` Ikjoon Jang
2021-08-26 2:51 ` [PATCH next v2 4/6] usb: xhci-mtk: add a member of num_esit Chunfeng Yun
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Chunfeng Yun @ 2021-08-26 2:51 UTC (permalink / raw)
To: Mathias Nyman, Greg Kroah-Hartman
Cc: Chunfeng Yun, Matthias Brugger, linux-usb, linux-arm-kernel,
linux-mediatek, linux-kernel, Ikjoon Jang, Eddie Hung, Yaqii wu
Use @bw_budget_table[] to update fs bus bandwidth due to
not all microframes consume @bw_cost_per_microframe, see
setup_sch_info().
Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v2: new patch, move from another series
---
drivers/usb/host/xhci-mtk-sch.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
index cffcaf4dfa9f..83abd28269ca 100644
--- a/drivers/usb/host/xhci-mtk-sch.c
+++ b/drivers/usb/host/xhci-mtk-sch.c
@@ -458,8 +458,8 @@ static int check_fs_bus_bw(struct mu3h_sch_ep_info *sch_ep, int offset)
* Compared with hs bus, no matter what ep type,
* the hub will always delay one uframe to send data
*/
- for (j = 0; j < sch_ep->cs_count; j++) {
- tmp = tt->fs_bus_bw[base + j] + sch_ep->bw_cost_per_microframe;
+ for (j = 0; j < sch_ep->num_budget_microframes; j++) {
+ tmp = tt->fs_bus_bw[base + j] + sch_ep->bw_budget_table[j];
if (tmp > FS_PAYLOAD_MAX)
return -ESCH_BW_OVERFLOW;
}
@@ -534,21 +534,18 @@ 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;
num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
- if (used)
- bw_updated = sch_ep->bw_cost_per_microframe;
- else
- bw_updated = -sch_ep->bw_cost_per_microframe;
-
for (i = 0; i < num_esit; i++) {
base = sch_ep->offset + i * sch_ep->esit;
- for (j = 0; j < sch_ep->cs_count; j++)
- tt->fs_bus_bw[base + j] += bw_updated;
+ for (j = 0; j < sch_ep->num_budget_microframes; j++)
+ if (used)
+ tt->fs_bus_bw[base + j] += sch_ep->bw_budget_table[j];
+ else
+ tt->fs_bus_bw[base + j] -= sch_ep->bw_budget_table[j];
}
if (used)
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH next v2 3/6] usb: xhci-mtk: update fs bus bandwidth by bw_budget_table
2021-08-26 2:51 ` [PATCH next v2 3/6] usb: xhci-mtk: update fs bus bandwidth by bw_budget_table Chunfeng Yun
@ 2021-08-26 11:54 ` Ikjoon Jang
2021-08-27 6:49 ` Chunfeng Yun (云春峰)
0 siblings, 1 reply; 15+ messages in thread
From: Ikjoon Jang @ 2021-08-26 11:54 UTC (permalink / raw)
To: Chunfeng Yun
Cc: Mathias Nyman, Greg Kroah-Hartman, Matthias Brugger,
open list:USB XHCI DRIVER,
moderated list:ARM/Mediatek SoC support,
moderated list:ARM/Mediatek SoC support, open list, Eddie Hung,
Yaqii wu
Hi Chunfeng,
On Thu, Aug 26, 2021 at 10:52 AM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
>
> Use @bw_budget_table[] to update fs bus bandwidth due to
> not all microframes consume @bw_cost_per_microframe, see
> setup_sch_info().
>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
> v2: new patch, move from another series
> ---
> drivers/usb/host/xhci-mtk-sch.c | 17 +++++++----------
> 1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
> index cffcaf4dfa9f..83abd28269ca 100644
> --- a/drivers/usb/host/xhci-mtk-sch.c
> +++ b/drivers/usb/host/xhci-mtk-sch.c
> @@ -458,8 +458,8 @@ static int check_fs_bus_bw(struct mu3h_sch_ep_info *sch_ep, int offset)
> * Compared with hs bus, no matter what ep type,
> * the hub will always delay one uframe to send data
> */
> - for (j = 0; j < sch_ep->cs_count; j++) {
> - tmp = tt->fs_bus_bw[base + j] + sch_ep->bw_cost_per_microframe;
> + for (j = 0; j < sch_ep->num_budget_microframes; j++) {
> + tmp = tt->fs_bus_bw[base + j] + sch_ep->bw_budget_table[j];
I'm worrying about this case with two endpoints,
* EP1OUT: isochronous, maxpacket=192: bw_budget_table[] = { 188, 188, 0, ... }
* EP2IN: interrupt, maxpacket=64: bw_budget_table[] = { 0, 0, 64, 64, ... }
(Is this correct bw_budget_table contents for those eps?)
I'm not sure if it's okay for those two endpoints to be allocated
on the same u-frame slot.
Can you please check if this is okay for xhci-mtk?
(I feel like I already asked the same questions many times.)
> if (tmp > FS_PAYLOAD_MAX)
> return -ESCH_BW_OVERFLOW;
> }
> @@ -534,21 +534,18 @@ 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;
>
> num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
>
> - if (used)
> - bw_updated = sch_ep->bw_cost_per_microframe;
> - else
> - bw_updated = -sch_ep->bw_cost_per_microframe;
> -
> for (i = 0; i < num_esit; i++) {
> base = sch_ep->offset + i * sch_ep->esit;
>
> - for (j = 0; j < sch_ep->cs_count; j++)
> - tt->fs_bus_bw[base + j] += bw_updated;
> + for (j = 0; j < sch_ep->num_budget_microframes; j++)
> + if (used)
> + tt->fs_bus_bw[base + j] += sch_ep->bw_budget_table[j];
> + else
> + tt->fs_bus_bw[base + j] -= sch_ep->bw_budget_table[j];
> }
>
> if (used)
> --
> 2.18.0
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH next v2 3/6] usb: xhci-mtk: update fs bus bandwidth by bw_budget_table
2021-08-26 11:54 ` Ikjoon Jang
@ 2021-08-27 6:49 ` Chunfeng Yun (云春峰)
2021-08-27 9:14 ` Ikjoon Jang
0 siblings, 1 reply; 15+ messages in thread
From: Chunfeng Yun (云春峰) @ 2021-08-27 6:49 UTC (permalink / raw)
To: ikjn
Cc: linux-mediatek, linux-kernel, linux-usb,
Yaqii Wu (武亚奇),
mathias.nyman, Eddie Hung (洪正鑫),
linux-arm-kernel, gregkh, matthias.bgg
On Thu, 2021-08-26 at 19:54 +0800, Ikjoon Jang wrote:
> Hi Chunfeng,
>
> On Thu, Aug 26, 2021 at 10:52 AM Chunfeng Yun <
> chunfeng.yun@mediatek.com> wrote:
> >
> > Use @bw_budget_table[] to update fs bus bandwidth due to
> > not all microframes consume @bw_cost_per_microframe, see
> > setup_sch_info().
> >
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> > v2: new patch, move from another series
> > ---
> > drivers/usb/host/xhci-mtk-sch.c | 17 +++++++----------
> > 1 file changed, 7 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-mtk-sch.c
> > b/drivers/usb/host/xhci-mtk-sch.c
> > index cffcaf4dfa9f..83abd28269ca 100644
> > --- a/drivers/usb/host/xhci-mtk-sch.c
> > +++ b/drivers/usb/host/xhci-mtk-sch.c
> > @@ -458,8 +458,8 @@ static int check_fs_bus_bw(struct
> > mu3h_sch_ep_info *sch_ep, int offset)
> > * Compared with hs bus, no matter what ep type,
> > * the hub will always delay one uframe to send
> > data
> > */
> > - for (j = 0; j < sch_ep->cs_count; j++) {
> > - tmp = tt->fs_bus_bw[base + j] + sch_ep-
> > >bw_cost_per_microframe;
> > + for (j = 0; j < sch_ep->num_budget_microframes;
> > j++) {
> > + tmp = tt->fs_bus_bw[base + j] + sch_ep-
> > >bw_budget_table[j];
>
> I'm worrying about this case with two endpoints,
> * EP1OUT: isochronous, maxpacket=192: bw_budget_table[] = { 188, 188,
> 0, ... }
> * EP2IN: interrupt, maxpacket=64: bw_budget_table[] = { 0, 0, 64, 64,
> ... }
> (Is this correct bw_budget_table contents for those eps?)
Yes, ep1out isoc use two uframe, ep2in intr use a extra cs;
>
> I'm not sure if it's okay for those two endpoints to be allocated
> on the same u-frame slot.
> Can you please check if this is okay for xhci-mtk?
Already test it this afternoon, can transfer data rightly on our dvt
env.
> (I feel like I already asked the same questions many times.)
Yes, as said before, prefer to use bw_budget_table[], if there is
issue, we can fix it by building this table.
Thanks
>
>
> > if (tmp > FS_PAYLOAD_MAX)
> > return -ESCH_BW_OVERFLOW;
> > }
> > @@ -534,21 +534,18 @@ 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;
> >
> > num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
> >
> > - if (used)
> > - bw_updated = sch_ep->bw_cost_per_microframe;
> > - else
> > - bw_updated = -sch_ep->bw_cost_per_microframe;
> > -
> > for (i = 0; i < num_esit; i++) {
> > base = sch_ep->offset + i * sch_ep->esit;
> >
> > - for (j = 0; j < sch_ep->cs_count; j++)
> > - tt->fs_bus_bw[base + j] += bw_updated;
> > + for (j = 0; j < sch_ep->num_budget_microframes;
> > j++)
> > + if (used)
> > + tt->fs_bus_bw[base + j] += sch_ep-
> > >bw_budget_table[j];
> > + else
> > + tt->fs_bus_bw[base + j] -= sch_ep-
> > >bw_budget_table[j];
> > }
> >
> > if (used)
> > --
> > 2.18.0
> >
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH next v2 3/6] usb: xhci-mtk: update fs bus bandwidth by bw_budget_table
2021-08-27 6:49 ` Chunfeng Yun (云春峰)
@ 2021-08-27 9:14 ` Ikjoon Jang
2021-08-27 9:48 ` Chunfeng Yun (云春峰)
0 siblings, 1 reply; 15+ messages in thread
From: Ikjoon Jang @ 2021-08-27 9:14 UTC (permalink / raw)
To: Chunfeng Yun (云春峰)
Cc: linux-mediatek, linux-kernel, linux-usb,
Yaqii Wu (武亚奇),
mathias.nyman, Eddie Hung (洪正鑫),
linux-arm-kernel, gregkh, matthias.bgg
On Fri, Aug 27, 2021 at 2:49 PM Chunfeng Yun (云春峰)
<Chunfeng.Yun@mediatek.com> wrote:
>
> On Thu, 2021-08-26 at 19:54 +0800, Ikjoon Jang wrote:
> > Hi Chunfeng,
> >
> > On Thu, Aug 26, 2021 at 10:52 AM Chunfeng Yun <
> > chunfeng.yun@mediatek.com> wrote:
> > >
> > > Use @bw_budget_table[] to update fs bus bandwidth due to
> > > not all microframes consume @bw_cost_per_microframe, see
> > > setup_sch_info().
> > >
> > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > ---
> > > v2: new patch, move from another series
> > > ---
> > > drivers/usb/host/xhci-mtk-sch.c | 17 +++++++----------
> > > 1 file changed, 7 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/usb/host/xhci-mtk-sch.c
> > > b/drivers/usb/host/xhci-mtk-sch.c
> > > index cffcaf4dfa9f..83abd28269ca 100644
> > > --- a/drivers/usb/host/xhci-mtk-sch.c
> > > +++ b/drivers/usb/host/xhci-mtk-sch.c
> > > @@ -458,8 +458,8 @@ static int check_fs_bus_bw(struct
> > > mu3h_sch_ep_info *sch_ep, int offset)
> > > * Compared with hs bus, no matter what ep type,
> > > * the hub will always delay one uframe to send
> > > data
> > > */
> > > - for (j = 0; j < sch_ep->cs_count; j++) {
> > > - tmp = tt->fs_bus_bw[base + j] + sch_ep-
> > > >bw_cost_per_microframe;
> > > + for (j = 0; j < sch_ep->num_budget_microframes;
> > > j++) {
> > > + tmp = tt->fs_bus_bw[base + j] + sch_ep-
> > > >bw_budget_table[j];
> >
> > I'm worrying about this case with two endpoints,
> > * EP1OUT: isochronous, maxpacket=192: bw_budget_table[] = { 188, 188,
> > 0, ... }
> > * EP2IN: interrupt, maxpacket=64: bw_budget_table[] = { 0, 0, 64, 64,
> > ... }
> > (Is this correct bw_budget_table contents for those eps?)
> Yes, ep1out isoc use two uframe, ep2in intr use a extra cs;
> >
> > I'm not sure if it's okay for those two endpoints to be allocated
> > on the same u-frame slot.
> > Can you please check if this is okay for xhci-mtk?
> Already test it this afternoon, can transfer data rightly on our dvt
> env.
>
> > (I feel like I already asked the same questions many times.)
> Yes, as said before, prefer to use bw_budget_table[], if there is
> issue, we can fix it by building this table.
So do you mean such an allocation shouldn't be a problem by IP design?
This patch starts to allow such an allocation (again).
But i remember my earlier tests showed that when those two eps
in the above example are allocated on the same u-frame slot,
xhci-mtk puts "SSPLIT for EP2" between
"SSPLIT-start and SSPLIT-end for EP1OUT transaction",
which is a spec violation. Hub will generate bit stuffing errors on the
full-speed bus.
>
> Thanks
> >
> >
> > > if (tmp > FS_PAYLOAD_MAX)
> > > return -ESCH_BW_OVERFLOW;
> > > }
> > > @@ -534,21 +534,18 @@ 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;
> > >
> > > num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
> > >
> > > - if (used)
> > > - bw_updated = sch_ep->bw_cost_per_microframe;
> > > - else
> > > - bw_updated = -sch_ep->bw_cost_per_microframe;
> > > -
> > > for (i = 0; i < num_esit; i++) {
> > > base = sch_ep->offset + i * sch_ep->esit;
> > >
> > > - for (j = 0; j < sch_ep->cs_count; j++)
> > > - tt->fs_bus_bw[base + j] += bw_updated;
> > > + for (j = 0; j < sch_ep->num_budget_microframes;
> > > j++)
> > > + if (used)
> > > + tt->fs_bus_bw[base + j] += sch_ep-
> > > >bw_budget_table[j];
> > > + else
> > > + tt->fs_bus_bw[base + j] -= sch_ep-
> > > >bw_budget_table[j];
> > > }
> > >
> > > if (used)
> > > --
> > > 2.18.0
> > >
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH next v2 3/6] usb: xhci-mtk: update fs bus bandwidth by bw_budget_table
2021-08-27 9:14 ` Ikjoon Jang
@ 2021-08-27 9:48 ` Chunfeng Yun (云春峰)
2021-08-30 3:49 ` Ikjoon Jang
0 siblings, 1 reply; 15+ messages in thread
From: Chunfeng Yun (云春峰) @ 2021-08-27 9:48 UTC (permalink / raw)
To: ikjn
Cc: linux-kernel, linux-mediatek, linux-usb,
Yaqii Wu (武亚奇),
mathias.nyman, Eddie Hung (洪正鑫),
linux-arm-kernel, gregkh, matthias.bgg
On Fri, 2021-08-27 at 17:14 +0800, Ikjoon Jang wrote:
> On Fri, Aug 27, 2021 at 2:49 PM Chunfeng Yun (云春峰)
> <Chunfeng.Yun@mediatek.com> wrote:
> >
> > On Thu, 2021-08-26 at 19:54 +0800, Ikjoon Jang wrote:
> > > Hi Chunfeng,
> > >
> > > On Thu, Aug 26, 2021 at 10:52 AM Chunfeng Yun <
> > > chunfeng.yun@mediatek.com> wrote:
> > > >
> > > > Use @bw_budget_table[] to update fs bus bandwidth due to
> > > > not all microframes consume @bw_cost_per_microframe, see
> > > > setup_sch_info().
> > > >
> > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > ---
> > > > v2: new patch, move from another series
> > > > ---
> > > > drivers/usb/host/xhci-mtk-sch.c | 17 +++++++----------
> > > > 1 file changed, 7 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/host/xhci-mtk-sch.c
> > > > b/drivers/usb/host/xhci-mtk-sch.c
> > > > index cffcaf4dfa9f..83abd28269ca 100644
> > > > --- a/drivers/usb/host/xhci-mtk-sch.c
> > > > +++ b/drivers/usb/host/xhci-mtk-sch.c
> > > > @@ -458,8 +458,8 @@ static int check_fs_bus_bw(struct
> > > > mu3h_sch_ep_info *sch_ep, int offset)
> > > > * Compared with hs bus, no matter what ep
> > > > type,
> > > > * the hub will always delay one uframe to send
> > > > data
> > > > */
> > > > - for (j = 0; j < sch_ep->cs_count; j++) {
> > > > - tmp = tt->fs_bus_bw[base + j] + sch_ep-
> > > > > bw_cost_per_microframe;
> > > >
> > > > + for (j = 0; j < sch_ep->num_budget_microframes;
> > > > j++) {
> > > > + tmp = tt->fs_bus_bw[base + j] + sch_ep-
> > > > > bw_budget_table[j];
> > >
> > > I'm worrying about this case with two endpoints,
> > > * EP1OUT: isochronous, maxpacket=192: bw_budget_table[] = { 188,
> > > 188,
> > > 0, ... }
> > > * EP2IN: interrupt, maxpacket=64: bw_budget_table[] = { 0, 0, 64,
> > > 64,
> > > ... }
> > > (Is this correct bw_budget_table contents for those eps?)
> >
> > Yes, ep1out isoc use two uframe, ep2in intr use a extra cs;
> > >
> > > I'm not sure if it's okay for those two endpoints to be allocated
> > > on the same u-frame slot.
> > > Can you please check if this is okay for xhci-mtk?
> >
> > Already test it this afternoon, can transfer data rightly on our
> > dvt
> > env.
> >
> > > (I feel like I already asked the same questions many times.)
> >
> > Yes, as said before, prefer to use bw_budget_table[], if there is
> > issue, we can fix it by building this table.
>
> So do you mean such an allocation shouldn't be a problem by IP
> design?
Yes, at least on our dvt platform
>
> This patch starts to allow such an allocation (again).
> But i remember my earlier tests showed that when those two eps
> in the above example are allocated on the same u-frame slot,
> xhci-mtk puts "SSPLIT for EP2" between
> "SSPLIT-start and SSPLIT-end for EP1OUT transaction",
> which is a spec violation.
Which section in usb2.0 spec?
> Hub will generate bit stuffing errors on the
> full-speed bus.
which platform?
>
>
> >
> > Thanks
> > >
> > >
> > > > if (tmp > FS_PAYLOAD_MAX)
> > > > return -ESCH_BW_OVERFLOW;
> > > > }
> > > > @@ -534,21 +534,18 @@ 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;
> > > >
> > > > num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
> > > >
> > > > - if (used)
> > > > - bw_updated = sch_ep->bw_cost_per_microframe;
> > > > - else
> > > > - bw_updated = -sch_ep->bw_cost_per_microframe;
> > > > -
> > > > for (i = 0; i < num_esit; i++) {
> > > > base = sch_ep->offset + i * sch_ep->esit;
> > > >
> > > > - for (j = 0; j < sch_ep->cs_count; j++)
> > > > - tt->fs_bus_bw[base + j] += bw_updated;
> > > > + for (j = 0; j < sch_ep->num_budget_microframes;
> > > > j++)
> > > > + if (used)
> > > > + tt->fs_bus_bw[base + j] +=
> > > > sch_ep-
> > > > > bw_budget_table[j];
> > > >
> > > > + else
> > > > + tt->fs_bus_bw[base + j] -=
> > > > sch_ep-
> > > > > bw_budget_table[j];
> > > >
> > > > }
> > > >
> > > > if (used)
> > > > --
> > > > 2.18.0
> > > >
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH next v2 3/6] usb: xhci-mtk: update fs bus bandwidth by bw_budget_table
2021-08-27 9:48 ` Chunfeng Yun (云春峰)
@ 2021-08-30 3:49 ` Ikjoon Jang
2021-08-31 6:50 ` Chunfeng Yun (云春峰)
0 siblings, 1 reply; 15+ messages in thread
From: Ikjoon Jang @ 2021-08-30 3:49 UTC (permalink / raw)
To: Chunfeng Yun (云春峰)
Cc: linux-kernel, linux-mediatek, linux-usb,
Yaqii Wu (武亚奇),
mathias.nyman, Eddie Hung (洪正鑫),
linux-arm-kernel, gregkh, matthias.bgg
On Fri, Aug 27, 2021 at 5:49 PM Chunfeng Yun (云春峰)
<Chunfeng.Yun@mediatek.com> wrote:
>
> On Fri, 2021-08-27 at 17:14 +0800, Ikjoon Jang wrote:
> > On Fri, Aug 27, 2021 at 2:49 PM Chunfeng Yun (云春峰)
> > <Chunfeng.Yun@mediatek.com> wrote:
> > >
> > > On Thu, 2021-08-26 at 19:54 +0800, Ikjoon Jang wrote:
> > > > Hi Chunfeng,
> > > >
> > > > On Thu, Aug 26, 2021 at 10:52 AM Chunfeng Yun <
> > > > chunfeng.yun@mediatek.com> wrote:
> > > > >
> > > > > Use @bw_budget_table[] to update fs bus bandwidth due to
> > > > > not all microframes consume @bw_cost_per_microframe, see
> > > > > setup_sch_info().
> > > > >
> > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > > ---
> > > > > v2: new patch, move from another series
> > > > > ---
> > > > > drivers/usb/host/xhci-mtk-sch.c | 17 +++++++----------
> > > > > 1 file changed, 7 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/drivers/usb/host/xhci-mtk-sch.c
> > > > > b/drivers/usb/host/xhci-mtk-sch.c
> > > > > index cffcaf4dfa9f..83abd28269ca 100644
> > > > > --- a/drivers/usb/host/xhci-mtk-sch.c
> > > > > +++ b/drivers/usb/host/xhci-mtk-sch.c
> > > > > @@ -458,8 +458,8 @@ static int check_fs_bus_bw(struct
> > > > > mu3h_sch_ep_info *sch_ep, int offset)
> > > > > * Compared with hs bus, no matter what ep
> > > > > type,
> > > > > * the hub will always delay one uframe to send
> > > > > data
> > > > > */
> > > > > - for (j = 0; j < sch_ep->cs_count; j++) {
> > > > > - tmp = tt->fs_bus_bw[base + j] + sch_ep-
> > > > > > bw_cost_per_microframe;
> > > > >
> > > > > + for (j = 0; j < sch_ep->num_budget_microframes;
> > > > > j++) {
> > > > > + tmp = tt->fs_bus_bw[base + j] + sch_ep-
> > > > > > bw_budget_table[j];
> > > >
> > > > I'm worrying about this case with two endpoints,
> > > > * EP1OUT: isochronous, maxpacket=192: bw_budget_table[] = { 188,
> > > > 188,
> > > > 0, ... }
> > > > * EP2IN: interrupt, maxpacket=64: bw_budget_table[] = { 0, 0, 64,
> > > > 64,
> > > > ... }
> > > > (Is this correct bw_budget_table contents for those eps?)
> > >
> > > Yes, ep1out isoc use two uframe, ep2in intr use a extra cs;
> > > >
> > > > I'm not sure if it's okay for those two endpoints to be allocated
> > > > on the same u-frame slot.
> > > > Can you please check if this is okay for xhci-mtk?
> > >
> > > Already test it this afternoon, can transfer data rightly on our
> > > dvt
> > > env.
> > >
> > > > (I feel like I already asked the same questions many times.)
> > >
> > > Yes, as said before, prefer to use bw_budget_table[], if there is
> > > issue, we can fix it by building this table.
> >
> > So do you mean such an allocation shouldn't be a problem by IP
> > design?
> Yes, at least on our dvt platform
Did you check that your side also has a similar allocation
(SSPLIT-all sits between SSPLIT-start ~ -end for another ep)?
My audio headset doesn't work properly with this scheme.
>
> >
> > This patch starts to allow such an allocation (again).
> > But i remember my earlier tests showed that when those two eps
> > in the above example are allocated on the same u-frame slot,
> > xhci-mtk puts "SSPLIT for EP2" between
> > "SSPLIT-start and SSPLIT-end for EP1OUT transaction",
> > which is a spec violation.
>
> Which section in usb2.0 spec?
I think that's just a basic rule - if software wants to send 192 bytes
through a full-speed bus, HC should send OUT/DATA 192 bytes
continuously without inserting any other packets during that 192 bytes.
and usb2 11.14.2 mentions that TT has separated
Start-Split and Complete-Split buffers
but not tracked each transaction per endpoint basis.
>
> > Hub will generate bit stuffing errors on the
> > full-speed bus.
> which platform?
I remember it was mt8173.
And for bit stuffing errors I mentioned in the earlier mail.
when I read the spec again, 11.21 mentions that bit stuffing error
is generated when _a microframe_ should be passed without
corresponding SSPLIT-mid/end. So this is not the case and also
I'm not sure what will happen on the full-speed bus, sorry.
In my case what I can be sure of is that the audio output was
broken with those allotments.
What is the xhci-mtk's policy when there are more than two EPs
marked as the same u-frame offset like in the above example?
>
> >
> >
> > >
> > > Thanks
> > > >
> > > >
> > > > > if (tmp > FS_PAYLOAD_MAX)
> > > > > return -ESCH_BW_OVERFLOW;
> > > > > }
> > > > > @@ -534,21 +534,18 @@ 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;
> > > > >
> > > > > num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
> > > > >
> > > > > - if (used)
> > > > > - bw_updated = sch_ep->bw_cost_per_microframe;
> > > > > - else
> > > > > - bw_updated = -sch_ep->bw_cost_per_microframe;
> > > > > -
> > > > > for (i = 0; i < num_esit; i++) {
> > > > > base = sch_ep->offset + i * sch_ep->esit;
> > > > >
> > > > > - for (j = 0; j < sch_ep->cs_count; j++)
> > > > > - tt->fs_bus_bw[base + j] += bw_updated;
> > > > > + for (j = 0; j < sch_ep->num_budget_microframes;
> > > > > j++)
> > > > > + if (used)
> > > > > + tt->fs_bus_bw[base + j] +=
> > > > > sch_ep-
> > > > > > bw_budget_table[j];
> > > > >
> > > > > + else
> > > > > + tt->fs_bus_bw[base + j] -=
> > > > > sch_ep-
> > > > > > bw_budget_table[j];
> > > > >
> > > > > }
> > > > >
> > > > > if (used)
> > > > > --
> > > > > 2.18.0
> > > > >
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH next v2 3/6] usb: xhci-mtk: update fs bus bandwidth by bw_budget_table
2021-08-30 3:49 ` Ikjoon Jang
@ 2021-08-31 6:50 ` Chunfeng Yun (云春峰)
0 siblings, 0 replies; 15+ messages in thread
From: Chunfeng Yun (云春峰) @ 2021-08-31 6:50 UTC (permalink / raw)
To: ikjn
Cc: linux-kernel, linux-mediatek, linux-usb,
Yaqii Wu (武亚奇),
mathias.nyman, Eddie Hung (洪正鑫),
linux-arm-kernel, gregkh, matthias.bgg
On Mon, 2021-08-30 at 11:49 +0800, Ikjoon Jang wrote:
> On Fri, Aug 27, 2021 at 5:49 PM Chunfeng Yun (云春峰)
> <Chunfeng.Yun@mediatek.com> wrote:
> >
> > On Fri, 2021-08-27 at 17:14 +0800, Ikjoon Jang wrote:
> > > On Fri, Aug 27, 2021 at 2:49 PM Chunfeng Yun (云春峰)
> > > <Chunfeng.Yun@mediatek.com> wrote:
> > > >
> > > > On Thu, 2021-08-26 at 19:54 +0800, Ikjoon Jang wrote:
> > > > > Hi Chunfeng,
> > > > >
> > > > > On Thu, Aug 26, 2021 at 10:52 AM Chunfeng Yun <
> > > > > chunfeng.yun@mediatek.com> wrote:
> > > > > >
> > > > > > Use @bw_budget_table[] to update fs bus bandwidth due to
> > > > > > not all microframes consume @bw_cost_per_microframe, see
> > > > > > setup_sch_info().
> > > > > >
> > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > > > ---
> > > > > > v2: new patch, move from another series
> > > > > > ---
> > > > > > drivers/usb/host/xhci-mtk-sch.c | 17 +++++++----------
> > > > > > 1 file changed, 7 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/usb/host/xhci-mtk-sch.c
> > > > > > b/drivers/usb/host/xhci-mtk-sch.c
> > > > > > index cffcaf4dfa9f..83abd28269ca 100644
> > > > > > --- a/drivers/usb/host/xhci-mtk-sch.c
> > > > > > +++ b/drivers/usb/host/xhci-mtk-sch.c
> > > > > > @@ -458,8 +458,8 @@ static int check_fs_bus_bw(struct
> > > > > > mu3h_sch_ep_info *sch_ep, int offset)
> > > > > > * Compared with hs bus, no matter what ep
> > > > > > type,
> > > > > > * the hub will always delay one uframe to
> > > > > > send
> > > > > > data
> > > > > > */
> > > > > > - for (j = 0; j < sch_ep->cs_count; j++) {
> > > > > > - tmp = tt->fs_bus_bw[base + j] +
> > > > > > sch_ep-
> > > > > > > bw_cost_per_microframe;
> > > > > >
> > > > > > + for (j = 0; j < sch_ep-
> > > > > > >num_budget_microframes;
> > > > > > j++) {
> > > > > > + tmp = tt->fs_bus_bw[base + j] +
> > > > > > sch_ep-
> > > > > > > bw_budget_table[j];
> > > > >
> > > > > I'm worrying about this case with two endpoints,
> > > > > * EP1OUT: isochronous, maxpacket=192: bw_budget_table[] = {
> > > > > 188,
> > > > > 188,
> > > > > 0, ... }
> > > > > * EP2IN: interrupt, maxpacket=64: bw_budget_table[] = { 0, 0,
> > > > > 64,
> > > > > 64,
> > > > > ... }
> > > > > (Is this correct bw_budget_table contents for those eps?)
> > > >
> > > > Yes, ep1out isoc use two uframe, ep2in intr use a extra cs;
> > > > >
> > > > > I'm not sure if it's okay for those two endpoints to be
> > > > > allocated
> > > > > on the same u-frame slot.
> > > > > Can you please check if this is okay for xhci-mtk?
> > > >
> > > > Already test it this afternoon, can transfer data rightly on
> > > > our
> > > > dvt
> > > > env.
> > > >
> > > > > (I feel like I already asked the same questions many times.)
> > > >
> > > > Yes, as said before, prefer to use bw_budget_table[], if there
> > > > is
> > > > issue, we can fix it by building this table.
> > >
> > > So do you mean such an allocation shouldn't be a problem by IP
> > > design?
> >
> > Yes, at least on our dvt platform
>
> Did you check that your side also has a similar allocation
> (SSPLIT-all sits between SSPLIT-start ~ -end for another ep)?
> My audio headset doesn't work properly with this scheme.
>
> >
> > >
> > > This patch starts to allow such an allocation (again).
> > > But i remember my earlier tests showed that when those two eps
> > > in the above example are allocated on the same u-frame slot,
> > > xhci-mtk puts "SSPLIT for EP2" between
> > > "SSPLIT-start and SSPLIT-end for EP1OUT transaction",
> > > which is a spec violation.
> >
> > Which section in usb2.0 spec?
>
> I think that's just a basic rule - if software wants to send 192
> bytes
> through a full-speed bus, HC should send OUT/DATA 192 bytes
> continuously without inserting any other packets during that 192
> bytes.
> and usb2 11.14.2 mentions that TT has separated
> Start-Split and Complete-Split buffers
> but not tracked each transaction per endpoint basis.
>
> >
> > > Hub will generate bit stuffing errors on the
> > > full-speed bus.
> >
> > which platform?
>
> I remember it was mt8173.
Does it happen on mt8192?
>
> And for bit stuffing errors I mentioned in the earlier mail.
> when I read the spec again, 11.21 mentions that bit stuffing error
> is generated when _a microframe_ should be passed without
> corresponding SSPLIT-mid/end. So this is not the case and also
> I'm not sure what will happen on the full-speed bus, sorry.
> In my case what I can be sure of is that the audio output was
> broken with those allotments.
>
> What is the xhci-mtk's policy when there are more than two EPs
> marked as the same u-frame offset like in the above example?
Seems no this limitation, an EP doesn't monopolize an u-frame
>
> >
> > >
> > >
> > > >
> > > > Thanks
> > > > >
> > > > >
> > > > > > if (tmp > FS_PAYLOAD_MAX)
> > > > > > return -ESCH_BW_OVERFLOW;
> > > > > > }
> > > > > > @@ -534,21 +534,18 @@ 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;
> > > > > >
> > > > > > num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
> > > > > >
> > > > > > - if (used)
> > > > > > - bw_updated = sch_ep-
> > > > > > >bw_cost_per_microframe;
> > > > > > - else
> > > > > > - bw_updated = -sch_ep-
> > > > > > >bw_cost_per_microframe;
> > > > > > -
> > > > > > for (i = 0; i < num_esit; i++) {
> > > > > > base = sch_ep->offset + i * sch_ep->esit;
> > > > > >
> > > > > > - for (j = 0; j < sch_ep->cs_count; j++)
> > > > > > - tt->fs_bus_bw[base + j] +=
> > > > > > bw_updated;
> > > > > > + for (j = 0; j < sch_ep-
> > > > > > >num_budget_microframes;
> > > > > > j++)
> > > > > > + if (used)
> > > > > > + tt->fs_bus_bw[base + j] +=
> > > > > > sch_ep-
> > > > > > > bw_budget_table[j];
> > > > > >
> > > > > > + else
> > > > > > + tt->fs_bus_bw[base + j] -=
> > > > > > sch_ep-
> > > > > > > bw_budget_table[j];
> > > > > >
> > > > > > }
> > > > > >
> > > > > > if (used)
> > > > > > --
> > > > > > 2.18.0
> > > > > >
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH next v2 4/6] usb: xhci-mtk: add a member of num_esit
2021-08-26 2:51 [PATCH next v2 1/6] Revert "usb: xhci-mtk: relax TT periodic bandwidth allocation" Chunfeng Yun
2021-08-26 2:51 ` [PATCH next v2 2/6] Revert "usb: xhci-mtk: Do not use xhci's virt_dev in drop_endpoint" Chunfeng Yun
2021-08-26 2:51 ` [PATCH next v2 3/6] usb: xhci-mtk: update fs bus bandwidth by bw_budget_table Chunfeng Yun
@ 2021-08-26 2:51 ` Chunfeng Yun
2021-08-26 2:51 ` [PATCH next v2 5/6] usb: xhci-mtk: Do not use xhci's virt_dev in drop_endpoint Chunfeng Yun
` (2 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Chunfeng Yun @ 2021-08-26 2:51 UTC (permalink / raw)
To: Mathias Nyman, Greg Kroah-Hartman
Cc: Chunfeng Yun, Matthias Brugger, linux-usb, linux-arm-kernel,
linux-mediatek, linux-kernel, Ikjoon Jang, Eddie Hung, Yaqii wu
Add a member num_esit to save the number of esit, then no need
caculate it in some functions.
Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v2: new patch, move from another series
---
drivers/usb/host/xhci-mtk-sch.c | 20 +++++++-------------
drivers/usb/host/xhci-mtk.h | 2 ++
2 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
index 83abd28269ca..54c521524bfc 100644
--- a/drivers/usb/host/xhci-mtk-sch.c
+++ b/drivers/usb/host/xhci-mtk-sch.c
@@ -297,6 +297,7 @@ static void setup_sch_info(struct xhci_ep_ctx *ep_ctx,
CTX_TO_MAX_ESIT_PAYLOAD(le32_to_cpu(ep_ctx->tx_info));
sch_ep->esit = get_esit(ep_ctx);
+ sch_ep->num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
sch_ep->ep_type = ep_type;
sch_ep->maxpkt = maxpkt;
sch_ep->offset = 0;
@@ -401,14 +402,12 @@ 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++) {
+ for (i = 0; i < sch_ep->num_esit; i++) {
u32 base = offset + i * sch_ep->esit;
for (j = 0; j < sch_ep->num_budget_microframes; j++) {
@@ -424,13 +423,11 @@ 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;
- num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
- for (i = 0; i < num_esit; i++) {
+ for (i = 0; i < sch_ep->num_esit; i++) {
base = sch_ep->offset + i * sch_ep->esit;
for (j = 0; j < sch_ep->num_budget_microframes; j++) {
if (used)
@@ -446,12 +443,11 @@ 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;
+ u32 tmp;
int base;
int i, j;
- num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
- for (i = 0; i < num_esit; i++) {
+ for (i = 0; i < sch_ep->num_esit; i++) {
base = offset + i * sch_ep->esit;
/*
@@ -533,12 +529,10 @@ 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;
+ u32 base;
int i, j;
- num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
-
- for (i = 0; i < num_esit; i++) {
+ for (i = 0; i < sch_ep->num_esit; i++) {
base = sch_ep->offset + i * sch_ep->esit;
for (j = 0; j < sch_ep->num_budget_microframes; j++)
diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
index ace432356c41..eb05aaf6edbe 100644
--- a/drivers/usb/host/xhci-mtk.h
+++ b/drivers/usb/host/xhci-mtk.h
@@ -50,6 +50,7 @@ struct mu3h_sch_bw_info {
* struct mu3h_sch_ep_info: schedule information for endpoint
*
* @esit: unit is 125us, equal to 2 << Interval field in ep-context
+ * @num_esit: number of @esit in a period
* @num_budget_microframes: number of continuous uframes
* (@repeat==1) scheduled within the interval
* @bw_cost_per_microframe: bandwidth cost per microframe
@@ -79,6 +80,7 @@ struct mu3h_sch_bw_info {
*/
struct mu3h_sch_ep_info {
u32 esit;
+ u32 num_esit;
u32 num_budget_microframes;
u32 bw_cost_per_microframe;
struct list_head endpoint;
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH next v2 5/6] usb: xhci-mtk: Do not use xhci's virt_dev in drop_endpoint
2021-08-26 2:51 [PATCH next v2 1/6] Revert "usb: xhci-mtk: relax TT periodic bandwidth allocation" Chunfeng Yun
` (2 preceding siblings ...)
2021-08-26 2:51 ` [PATCH next v2 4/6] usb: xhci-mtk: add a member of num_esit Chunfeng Yun
@ 2021-08-26 2:51 ` Chunfeng Yun
2021-08-26 2:51 ` [PATCH next v2 6/6] usb: xhci-mtk: allow bandwidth table rollover Chunfeng Yun
2021-08-26 11:41 ` [PATCH next v2 1/6] Revert "usb: xhci-mtk: relax TT periodic bandwidth allocation" Greg Kroah-Hartman
5 siblings, 0 replies; 15+ messages in thread
From: Chunfeng Yun @ 2021-08-26 2:51 UTC (permalink / raw)
To: Mathias Nyman, Greg Kroah-Hartman
Cc: Chunfeng Yun, Matthias Brugger, linux-usb, linux-arm-kernel,
linux-mediatek, linux-kernel, Ikjoon Jang, Eddie Hung, Yaqii wu
xhci-mtk depends on xhci's internal virt_dev when it retrieves its
internal data from usb_host_endpoint both in add_endpoint and
drop_endpoint callbacks. But when setup packet was retired by
transaction errors in xhci_setup_device() path, a virt_dev for the slot
is newly created with real_port 0. This leads to xhci-mtks's NULL pointer
dereference from drop_endpoint callback as xhci-mtk assumes that virt_dev's
real_port is always started from one. The similar problems were addressed
by [1] but that can't cover the failure cases from setup_device.
This patch drops the usages of xhci's virt_dev in xhci-mtk's drop_endpoint
callback by adopting hashtable for searching mtk's schedule entity
from a given usb_host_endpoint pointer instead of searching a linked list.
So mtk's drop_endpoint callback doesn't have to rely on virt_dev at all.
[1] f351f4b63dac ("usb: xhci-mtk: fix oops when unbind driver")
Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v2: use fixed size hash table instead of rhashtable;
fix some issues as described in this series [2/6]
---
drivers/usb/host/xhci-mtk-sch.c | 100 ++++++++++++++++----------------
drivers/usb/host/xhci-mtk.h | 11 +++-
2 files changed, 60 insertions(+), 51 deletions(-)
diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
index 54c521524bfc..f3c6f078f82d 100644
--- a/drivers/usb/host/xhci-mtk-sch.c
+++ b/drivers/usb/host/xhci-mtk-sch.c
@@ -80,7 +80,7 @@ decode_ep(struct usb_host_endpoint *ep, enum usb_device_speed speed)
interval /= 1000;
}
- snprintf(buf, DBG_BUF_EN, "%s ep%d%s %s, mpkt:%d, interval:%d/%d%s\n",
+ snprintf(buf, DBG_BUF_EN, "%s ep%d%s %s, mpkt:%d, interval:%d/%d%s",
usb_speed_string(speed), usb_endpoint_num(epd),
usb_endpoint_dir_in(epd) ? "in" : "out",
usb_ep_type_string(usb_endpoint_type(epd)),
@@ -129,6 +129,10 @@ get_bw_info(struct xhci_hcd_mtk *mtk, struct usb_device *udev,
int bw_index;
virt_dev = xhci->devs[udev->slot_id];
+ if (!virt_dev->real_port) {
+ WARN_ONCE(1, "%s invalid real_port\n", dev_name(&udev->dev));
+ return NULL;
+ }
if (udev->speed >= USB_SPEED_SUPER) {
if (usb_endpoint_dir_out(&ep->desc))
@@ -236,14 +240,20 @@ static void drop_tt(struct usb_device *udev)
}
}
-static struct mu3h_sch_ep_info *create_sch_ep(struct usb_device *udev,
- struct usb_host_endpoint *ep, struct xhci_ep_ctx *ep_ctx)
+static struct mu3h_sch_ep_info *
+create_sch_ep(struct xhci_hcd_mtk *mtk, struct usb_device *udev,
+ struct usb_host_endpoint *ep, struct xhci_ep_ctx *ep_ctx)
{
struct mu3h_sch_ep_info *sch_ep;
+ struct mu3h_sch_bw_info *bw_info;
struct mu3h_sch_tt *tt = NULL;
u32 len_bw_budget_table;
size_t mem_size;
+ bw_info = get_bw_info(mtk, udev, ep);
+ if (!bw_info)
+ return ERR_PTR(-ENODEV);
+
if (is_fs_or_ls(udev->speed))
len_bw_budget_table = TT_MICROFRAMES_MAX;
else if ((udev->speed >= USB_SPEED_SUPER)
@@ -266,11 +276,13 @@ static struct mu3h_sch_ep_info *create_sch_ep(struct usb_device *udev,
}
}
+ sch_ep->bw_info = bw_info;
sch_ep->sch_tt = tt;
sch_ep->ep = ep;
sch_ep->speed = udev->speed;
INIT_LIST_HEAD(&sch_ep->endpoint);
INIT_LIST_HEAD(&sch_ep->tt_endpoint);
+ INIT_HLIST_NODE(&sch_ep->hentry);
return sch_ep;
}
@@ -576,9 +588,9 @@ static u32 get_esit_boundary(struct mu3h_sch_ep_info *sch_ep)
return boundary;
}
-static int check_sch_bw(struct mu3h_sch_bw_info *sch_bw,
- struct mu3h_sch_ep_info *sch_ep)
+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;
@@ -624,23 +636,26 @@ static int check_sch_bw(struct mu3h_sch_bw_info *sch_bw,
return load_ep_bw(sch_bw, sch_ep, true);
}
-static void destroy_sch_ep(struct usb_device *udev,
- struct mu3h_sch_bw_info *sch_bw, struct mu3h_sch_ep_info *sch_ep)
+static void destroy_sch_ep(struct xhci_hcd_mtk *mtk, struct usb_device *udev,
+ struct mu3h_sch_ep_info *sch_ep)
{
/* only release ep bw check passed by check_sch_bw() */
if (sch_ep->allocated)
- load_ep_bw(sch_bw, sch_ep, false);
+ load_ep_bw(sch_ep->bw_info, sch_ep, false);
if (sch_ep->sch_tt)
drop_tt(udev);
list_del(&sch_ep->endpoint);
+ hlist_del(&sch_ep->hentry);
kfree(sch_ep);
}
-static bool need_bw_sch(struct usb_host_endpoint *ep,
- enum usb_device_speed speed, int has_tt)
+static bool need_bw_sch(struct usb_device *udev,
+ struct usb_host_endpoint *ep)
{
+ bool has_tt = udev->tt && udev->tt->hub->parent;
+
/* only for periodic endpoints */
if (usb_endpoint_xfer_control(&ep->desc)
|| usb_endpoint_xfer_bulk(&ep->desc))
@@ -651,7 +666,7 @@ static bool need_bw_sch(struct usb_host_endpoint *ep,
* a TT are also ignored, root-hub will schedule them directly,
* but need set @bpkts field of endpoint context to 1.
*/
- if (is_fs_or_ls(speed) && !has_tt)
+ if (is_fs_or_ls(udev->speed) && !has_tt)
return false;
/* skip endpoint with zero maxpkt */
@@ -666,7 +681,6 @@ int xhci_mtk_sch_init(struct xhci_hcd_mtk *mtk)
struct xhci_hcd *xhci = hcd_to_xhci(mtk->hcd);
struct mu3h_sch_bw_info *sch_array;
int num_usb_bus;
- int i;
/* ss IN and OUT are separated */
num_usb_bus = xhci->usb3_rhub.num_ports * 2 + xhci->usb2_rhub.num_ports;
@@ -675,12 +689,10 @@ int xhci_mtk_sch_init(struct xhci_hcd_mtk *mtk)
if (sch_array == NULL)
return -ENOMEM;
- for (i = 0; i < num_usb_bus; i++)
- INIT_LIST_HEAD(&sch_array[i].bw_ep_list);
-
mtk->sch_array = sch_array;
INIT_LIST_HEAD(&mtk->bw_ep_chk_list);
+ hash_init(mtk->sch_ep_hash);
return 0;
}
@@ -704,9 +716,7 @@ static int add_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
ep_index = xhci_get_endpoint_index(&ep->desc);
ep_ctx = xhci_get_ep_ctx(xhci, virt_dev->in_ctx, ep_index);
- xhci_dbg(xhci, "%s %s\n", __func__, decode_ep(ep, udev->speed));
-
- if (!need_bw_sch(ep, udev->speed, !!virt_dev->tt_info)) {
+ if (!need_bw_sch(udev, ep)) {
/*
* set @bpkts to 1 if it is LS or FS periodic endpoint, and its
* device does not connected through an external HS hub
@@ -718,13 +728,16 @@ static int add_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
return 0;
}
- sch_ep = create_sch_ep(udev, ep, ep_ctx);
+ xhci_dbg(xhci, "%s %s\n", __func__, decode_ep(ep, udev->speed));
+
+ sch_ep = create_sch_ep(mtk, udev, ep, ep_ctx);
if (IS_ERR_OR_NULL(sch_ep))
return -ENOMEM;
setup_sch_info(ep_ctx, sch_ep);
list_add_tail(&sch_ep->endpoint, &mtk->bw_ep_chk_list);
+ hash_add(mtk->sch_ep_hash, &sch_ep->hentry, (unsigned long)ep);
return 0;
}
@@ -734,22 +747,18 @@ static void drop_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
{
struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
- struct xhci_virt_device *virt_dev;
- struct mu3h_sch_bw_info *sch_bw;
- struct mu3h_sch_ep_info *sch_ep, *tmp;
-
- virt_dev = xhci->devs[udev->slot_id];
-
- xhci_dbg(xhci, "%s %s\n", __func__, decode_ep(ep, udev->speed));
+ struct mu3h_sch_ep_info *sch_ep;
+ struct hlist_node *hn;
- if (!need_bw_sch(ep, udev->speed, !!virt_dev->tt_info))
+ if (!need_bw_sch(udev, ep))
return;
- sch_bw = get_bw_info(mtk, udev, ep);
+ xhci_err(xhci, "%s %s\n", __func__, decode_ep(ep, udev->speed));
- list_for_each_entry_safe(sch_ep, tmp, &sch_bw->bw_ep_list, endpoint) {
+ hash_for_each_possible_safe(mtk->sch_ep_hash, sch_ep,
+ hn, hentry, (unsigned long)ep) {
if (sch_ep->ep == ep) {
- destroy_sch_ep(udev, sch_bw, sch_ep);
+ destroy_sch_ep(mtk, udev, sch_ep);
break;
}
}
@@ -760,30 +769,22 @@ int xhci_mtk_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
struct xhci_virt_device *virt_dev = xhci->devs[udev->slot_id];
- struct mu3h_sch_bw_info *sch_bw;
- struct mu3h_sch_ep_info *sch_ep, *tmp;
+ struct mu3h_sch_ep_info *sch_ep;
int ret;
xhci_dbg(xhci, "%s() udev %s\n", __func__, dev_name(&udev->dev));
list_for_each_entry(sch_ep, &mtk->bw_ep_chk_list, endpoint) {
- sch_bw = get_bw_info(mtk, udev, sch_ep->ep);
+ struct xhci_ep_ctx *ep_ctx;
+ struct usb_host_endpoint *ep = sch_ep->ep;
+ unsigned int ep_index = xhci_get_endpoint_index(&ep->desc);
- ret = check_sch_bw(sch_bw, sch_ep);
+ ret = check_sch_bw(sch_ep);
if (ret) {
xhci_err(xhci, "Not enough bandwidth! (%s)\n",
sch_error_string(-ret));
return -ENOSPC;
}
- }
-
- list_for_each_entry_safe(sch_ep, tmp, &mtk->bw_ep_chk_list, endpoint) {
- struct xhci_ep_ctx *ep_ctx;
- struct usb_host_endpoint *ep = sch_ep->ep;
- unsigned int ep_index = xhci_get_endpoint_index(&ep->desc);
-
- sch_bw = get_bw_info(mtk, udev, ep);
- list_move_tail(&sch_ep->endpoint, &sch_bw->bw_ep_list);
ep_ctx = xhci_get_ep_ctx(xhci, virt_dev->in_ctx, ep_index);
ep_ctx->reserved[0] = cpu_to_le32(EP_BPKTS(sch_ep->pkts)
@@ -797,22 +798,23 @@ int xhci_mtk_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
sch_ep->offset, sch_ep->repeat);
}
- return xhci_check_bandwidth(hcd, udev);
+ ret = xhci_check_bandwidth(hcd, udev);
+ if (!ret)
+ INIT_LIST_HEAD(&mtk->bw_ep_chk_list);
+
+ return ret;
}
void xhci_mtk_reset_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
{
struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
- struct mu3h_sch_bw_info *sch_bw;
struct mu3h_sch_ep_info *sch_ep, *tmp;
xhci_dbg(xhci, "%s() udev %s\n", __func__, dev_name(&udev->dev));
- list_for_each_entry_safe(sch_ep, tmp, &mtk->bw_ep_chk_list, endpoint) {
- sch_bw = get_bw_info(mtk, udev, sch_ep->ep);
- destroy_sch_ep(udev, sch_bw, sch_ep);
- }
+ list_for_each_entry_safe(sch_ep, tmp, &mtk->bw_ep_chk_list, endpoint)
+ destroy_sch_ep(mtk, udev, sch_ep);
xhci_reset_bandwidth(hcd, udev);
}
diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
index eb05aaf6edbe..dc5e83f5c5cc 100644
--- a/drivers/usb/host/xhci-mtk.h
+++ b/drivers/usb/host/xhci-mtk.h
@@ -10,11 +10,15 @@
#define _XHCI_MTK_H_
#include <linux/clk.h>
+#include <linux/hashtable.h>
#include "xhci.h"
#define BULK_CLKS_NUM 5
+/* support at most 64 ep, use 32 size hash table */
+#define SCH_EP_HASH_BITS 5
+
/**
* To simplify scheduler algorithm, set a upper limit for ESIT,
* if a synchromous ep's ESIT is larger than @XHCI_MTK_MAX_ESIT,
@@ -36,14 +40,12 @@ struct mu3h_sch_tt {
* struct mu3h_sch_bw_info: schedule information for bandwidth domain
*
* @bus_bw: array to keep track of bandwidth already used at each uframes
- * @bw_ep_list: eps in the bandwidth domain
*
* treat a HS root port as a bandwidth domain, but treat a SS root port as
* two bandwidth domains, one for IN eps and another for OUT eps.
*/
struct mu3h_sch_bw_info {
u32 bus_bw[XHCI_MTK_MAX_ESIT];
- struct list_head bw_ep_list;
};
/**
@@ -54,8 +56,10 @@ struct mu3h_sch_bw_info {
* @num_budget_microframes: number of continuous uframes
* (@repeat==1) scheduled within the interval
* @bw_cost_per_microframe: bandwidth cost per microframe
+ * @hentry: hash table entry
* @endpoint: linked into bandwidth domain which it belongs to
* @tt_endpoint: linked into mu3h_sch_tt's list which it belongs to
+ * @bw_info: bandwidth domain which this endpoint belongs
* @sch_tt: mu3h_sch_tt linked into
* @ep_type: endpoint type
* @maxpkt: max packet size of endpoint
@@ -84,7 +88,9 @@ struct mu3h_sch_ep_info {
u32 num_budget_microframes;
u32 bw_cost_per_microframe;
struct list_head endpoint;
+ struct hlist_node hentry;
struct list_head tt_endpoint;
+ struct mu3h_sch_bw_info *bw_info;
struct mu3h_sch_tt *sch_tt;
u32 ep_type;
u32 maxpkt;
@@ -137,6 +143,7 @@ struct xhci_hcd_mtk {
struct usb_hcd *hcd;
struct mu3h_sch_bw_info *sch_array;
struct list_head bw_ep_chk_list;
+ DECLARE_HASHTABLE(sch_ep_hash, SCH_EP_HASH_BITS);
struct mu3c_ippc_regs __iomem *ippc_regs;
int num_u2_ports;
int num_u3_ports;
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH next v2 6/6] usb: xhci-mtk: allow bandwidth table rollover
2021-08-26 2:51 [PATCH next v2 1/6] Revert "usb: xhci-mtk: relax TT periodic bandwidth allocation" Chunfeng Yun
` (3 preceding siblings ...)
2021-08-26 2:51 ` [PATCH next v2 5/6] usb: xhci-mtk: Do not use xhci's virt_dev in drop_endpoint Chunfeng Yun
@ 2021-08-26 2:51 ` Chunfeng Yun
2021-08-26 11:41 ` [PATCH next v2 1/6] Revert "usb: xhci-mtk: relax TT periodic bandwidth allocation" Greg Kroah-Hartman
5 siblings, 0 replies; 15+ messages in thread
From: Chunfeng Yun @ 2021-08-26 2:51 UTC (permalink / raw)
To: Mathias Nyman, Greg Kroah-Hartman
Cc: Chunfeng Yun, Matthias Brugger, linux-usb, linux-arm-kernel,
linux-mediatek, linux-kernel, Ikjoon Jang, Eddie Hung, Yaqii wu
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
can be rolled over but the current implementation doesn't.
This patch allows the microframe index rollover and prevent
out-of-bounds array access.
Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v2: new patch
---
drivers/usb/host/xhci-mtk-sch.c | 51 +++++++++++----------------------
drivers/usb/host/xhci-mtk.h | 3 +-
2 files changed, 18 insertions(+), 36 deletions(-)
diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
index f3c6f078f82d..134f4789bd89 100644
--- a/drivers/usb/host/xhci-mtk-sch.c
+++ b/drivers/usb/host/xhci-mtk-sch.c
@@ -416,15 +416,14 @@ static u32 get_max_bw(struct mu3h_sch_bw_info *sch_bw,
{
u32 max_bw = 0;
u32 bw;
- int i;
- int j;
+ int i, j, k;
for (i = 0; i < sch_ep->num_esit; i++) {
u32 base = offset + i * sch_ep->esit;
for (j = 0; j < sch_ep->num_budget_microframes; j++) {
- bw = sch_bw->bus_bw[base + j] +
- sch_ep->bw_budget_table[j];
+ k = XHCI_MTK_BW_INDEX(base + j);
+ bw = sch_bw->bus_bw[k] + sch_ep->bw_budget_table[j];
if (bw > max_bw)
max_bw = bw;
}
@@ -436,18 +435,16 @@ static void update_bus_bw(struct mu3h_sch_bw_info *sch_bw,
struct mu3h_sch_ep_info *sch_ep, bool used)
{
u32 base;
- int i;
- int j;
+ int i, j, k;
for (i = 0; i < sch_ep->num_esit; i++) {
base = sch_ep->offset + i * sch_ep->esit;
for (j = 0; j < sch_ep->num_budget_microframes; j++) {
+ k = XHCI_MTK_BW_INDEX(base + j);
if (used)
- sch_bw->bus_bw[base + j] +=
- sch_ep->bw_budget_table[j];
+ sch_bw->bus_bw[k] += sch_ep->bw_budget_table[j];
else
- sch_bw->bus_bw[base + j] -=
- sch_ep->bw_budget_table[j];
+ sch_bw->bus_bw[k] -= sch_ep->bw_budget_table[j];
}
}
}
@@ -457,7 +454,7 @@ 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 tmp;
int base;
- int i, j;
+ int i, j, k;
for (i = 0; i < sch_ep->num_esit; i++) {
base = offset + i * sch_ep->esit;
@@ -467,7 +464,8 @@ static int check_fs_bus_bw(struct mu3h_sch_ep_info *sch_ep, int offset)
* the hub will always delay one uframe to send data
*/
for (j = 0; j < sch_ep->num_budget_microframes; j++) {
- tmp = tt->fs_bus_bw[base + j] + sch_ep->bw_budget_table[j];
+ k = XHCI_MTK_BW_INDEX(base + j);
+ tmp = tt->fs_bus_bw[k] + sch_ep->bw_budget_table[j];
if (tmp > FS_PAYLOAD_MAX)
return -ESCH_BW_OVERFLOW;
}
@@ -542,16 +540,18 @@ 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;
- int i, j;
+ int i, j, k;
for (i = 0; i < sch_ep->num_esit; i++) {
base = sch_ep->offset + i * sch_ep->esit;
- for (j = 0; j < sch_ep->num_budget_microframes; j++)
+ for (j = 0; j < sch_ep->num_budget_microframes; j++) {
+ k = XHCI_MTK_BW_INDEX(base + j);
if (used)
- tt->fs_bus_bw[base + j] += sch_ep->bw_budget_table[j];
+ tt->fs_bus_bw[k] += sch_ep->bw_budget_table[j];
else
- tt->fs_bus_bw[base + j] -= sch_ep->bw_budget_table[j];
+ tt->fs_bus_bw[k] -= sch_ep->bw_budget_table[j];
+ }
}
if (used)
@@ -573,25 +573,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;
@@ -608,9 +592,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 dc5e83f5c5cc..7cc0123eada0 100644
--- a/drivers/usb/host/xhci-mtk.h
+++ b/drivers/usb/host/xhci-mtk.h
@@ -25,7 +25,8 @@
* round down to the limit value, that means allocating more
* bandwidth to it.
*/
-#define XHCI_MTK_MAX_ESIT 64
+#define XHCI_MTK_MAX_ESIT (1 << 6)
+#define XHCI_MTK_BW_INDEX(x) ((x) & (XHCI_MTK_MAX_ESIT - 1))
/**
* @fs_bus_bw: array to keep track of bandwidth already used for FS
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH next v2 1/6] Revert "usb: xhci-mtk: relax TT periodic bandwidth allocation"
2021-08-26 2:51 [PATCH next v2 1/6] Revert "usb: xhci-mtk: relax TT periodic bandwidth allocation" Chunfeng Yun
` (4 preceding siblings ...)
2021-08-26 2:51 ` [PATCH next v2 6/6] usb: xhci-mtk: allow bandwidth table rollover Chunfeng Yun
@ 2021-08-26 11:41 ` Greg Kroah-Hartman
2021-08-27 3:23 ` Chunfeng Yun (云春峰)
2021-08-27 9:46 ` Chunfeng Yun (云春峰)
5 siblings, 2 replies; 15+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-26 11:41 UTC (permalink / raw)
To: Chunfeng Yun
Cc: Mathias Nyman, Matthias Brugger, linux-usb, linux-arm-kernel,
linux-mediatek, linux-kernel, Ikjoon Jang, Eddie Hung, Yaqii wu
On Thu, Aug 26, 2021 at 10:51:39AM +0800, Chunfeng Yun wrote:
> As discussed in following patch:
> https://patchwork.kernel.org/patch/12420339
>
> No need calculate number of uframes again, but should use value
> form check_sch_tt(), if we plan to remove extra CS, also can do
> it in check_sch_tt(). So revert this patch, and prepare to send
> new patch for it.
>
> This reverts commit 548011957d1d72e0b662300c8b32b81d593b796e.
>
> Cc: Ikjoon Jang <ikjn@chromium.org>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
> v2: no changes
This series does not apply to my tree at all now, can you please rebase
and resend?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH next v2 1/6] Revert "usb: xhci-mtk: relax TT periodic bandwidth allocation"
2021-08-26 11:41 ` [PATCH next v2 1/6] Revert "usb: xhci-mtk: relax TT periodic bandwidth allocation" Greg Kroah-Hartman
@ 2021-08-27 3:23 ` Chunfeng Yun (云春峰)
2021-08-27 9:46 ` Chunfeng Yun (云春峰)
1 sibling, 0 replies; 15+ messages in thread
From: Chunfeng Yun (云春峰) @ 2021-08-27 3:23 UTC (permalink / raw)
To: gregkh
Cc: linux-mediatek, linux-kernel, linux-usb,
Yaqii Wu (武亚奇),
mathias.nyman, Eddie Hung (洪正鑫),
linux-arm-kernel, matthias.bgg, ikjn
Hi Greg,
On Thu, 2021-08-26 at 13:41 +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 26, 2021 at 10:51:39AM +0800, Chunfeng Yun wrote:
> > As discussed in following patch:
> > https://patchwork.kernel.org/patch/12420339
> >
> > No need calculate number of uframes again, but should use value
> > form check_sch_tt(), if we plan to remove extra CS, also can do
> > it in check_sch_tt(). So revert this patch, and prepare to send
> > new patch for it.
> >
> > This reverts commit 548011957d1d72e0b662300c8b32b81d593b796e.
> >
> > Cc: Ikjoon Jang <ikjn@chromium.org>
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> > v2: no changes
>
> This series does not apply to my tree at all now, can you please
> rebase
> and resend?
Very sorry, I send out two series [1][2] for xhci-mtk, but don't take
care of conflicts, suppose that series [1] will be applied firstly, due
to one binding patch [3] of series [2] is not acked/reviewed by Rob (I
think only need modify some misleading commit message).
anyway, I find that only one patch [4] of series [1] is not applied, so
I'll fix conficts and resend it based on usb-testing branch.
Sorry again.
[1]: Series = [next,v2,1/6] Revert "usb: xhci-mtk: relax TT periodic
bandwidth allocation"
https://patchwork.kernel.org/project/linux-mediatek/list/?series=537471
[2]: Series = [RESEND,1/9] dt-bindings: usb: mtk-xhci: add optional
property to disable usb2 ports
https://patchwork.kernel.org/project/linux-mediatek/list/?series=532595
[3]:
https://patchwork.kernel.org/project/linux-mediatek/patch/1629189389-18779-2-git-send-email-chunfeng.yun@mediatek.com/
[RESEND,2/9] dt-bindings: usb: mtk-xhci: add compatible for mt8195
[4]:
https://patchwork.kernel.org/project/linux-mediatek/patch/20210826025144.51992-6-chunfeng.yun@mediatek.com/
[next,v2,6/6] usb: xhci-mtk: allow bandwidth table rollover
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH next v2 1/6] Revert "usb: xhci-mtk: relax TT periodic bandwidth allocation"
2021-08-26 11:41 ` [PATCH next v2 1/6] Revert "usb: xhci-mtk: relax TT periodic bandwidth allocation" Greg Kroah-Hartman
2021-08-27 3:23 ` Chunfeng Yun (云春峰)
@ 2021-08-27 9:46 ` Chunfeng Yun (云春峰)
1 sibling, 0 replies; 15+ messages in thread
From: Chunfeng Yun (云春峰) @ 2021-08-27 9:46 UTC (permalink / raw)
To: gregkh
Cc: linux-mediatek, linux-kernel, linux-usb,
Yaqii Wu (武亚奇),
mathias.nyman, Eddie Hung (洪正鑫),
linux-arm-kernel, matthias.bgg, ikjn
Hi Greg,
On Thu, 2021-08-26 at 13:41 +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 26, 2021 at 10:51:39AM +0800, Chunfeng Yun wrote:
> > As discussed in following patch:
> > https://patchwork.kernel.org/patch/12420339
> >
> > No need calculate number of uframes again, but should use value
> > form check_sch_tt(), if we plan to remove extra CS, also can do
> > it in check_sch_tt(). So revert this patch, and prepare to send
> > new patch for it.
> >
> > This reverts commit 548011957d1d72e0b662300c8b32b81d593b796e.
> >
> > Cc: Ikjoon Jang <ikjn@chromium.org>
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> > v2: no changes
>
> This series does not apply to my tree at all now, can you please
> rebase
> and resend?
Very sorry, I send two series [1][2] for xhci-mtk, but don't take care
of conficts:
I suppose you will apply [1] firstly, due to [3] is not acked or
reviewed by Rob (I think, only need modify commit message).
Anyway, only one patch [4] is not in usb-testing branch due to
conflicts, I'll fix it and send out, sorry again.
[1]. Series = [next,v2,1/6] Revert "usb: xhci-mtk: relax TT periodic
bandwidth allocation"
https://patchwork.kernel.org/project/linux-mediatek/list/?series=537471
[2]. Series = [RESEND,1/9] dt-bindings: usb: mtk-xhci: add optional
property to disable usb2 ports
https://patchwork.kernel.org/project/linux-mediatek/list/?series=532595
[3]:
https://patchwork.kernel.org/project/linux-mediatek/patch/1629189389-18779-2-git-send-email-chunfeng.yun@mediatek.com/
[RESEND,2/9] dt-bindings: usb: mtk-xhci: add compatible for mt8195
[4]:
https://patchwork.kernel.org/project/linux-mediatek/patch/20210826025144.51992-6-chunfeng.yun@mediatek.com/
[next,v2,6/6] usb: xhci-mtk: allow bandwidth table rollover
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 15+ messages in thread