[RFC PATCH 1/6] usb: xhci-mtk: update fs bus bandwidth by bw_budget_table

[RFC PATCH 1/6] usb: xhci-mtk: update fs bus bandwidth by bw_budget_table

[Chunfeng Yun]

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>
---
 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
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[RFC PATCH 2/6] usb: xhci-mtk: add a member of num_esit

[Chunfeng Yun]

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>
---
 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
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[RFC PATCH 3/6] usb: xhci-mtk: Do not use xhci's virt_dev in drop_endpoint

[Chunfeng Yun]

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>
---
 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
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RFC PATCH 3/6] usb: xhci-mtk: Do not use xhci's virt_dev in drop_endpoint

[Ikjoon Jang]

HI Chunfeng,

On Mon, Aug 23, 2021 at 8:54 PM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
>
> 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>
> ---
>  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];

Yeah, previous code had unnecessary NULL checking here.

> +       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);

so tt_endpoint was revived, is this related to bugs?

>
>         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;

I think a simple hashtable is well fit but also rhashtable
had no problems to use.

Overall, I suggest you merge this patch with reverting
b8731209958a "usb: xhci-mtk: Do not use xhci's virt_dev in drop_endpoint"
with some notes about the bugs I made.

IIUC, this patch looks like reviving mu3h_sch_tt::ep_list and
changing rhash_table to hashtable. The incremental patch will be
much shorter sized and clearer to understand.
I'm not so clear what you are trying to fix.

Besides that, I don't have any objections to these changes. :)
Thanks.

>         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
>

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RFC PATCH 3/6] usb: xhci-mtk: Do not use xhci's virt_dev in drop_endpoint

[Chunfeng Yun (云春峰)]

On Tue, 2021-08-24 at 11:53 +0800, Ikjoon Jang wrote:
> HI Chunfeng,
> 
> On Mon, Aug 23, 2021 at 8:54 PM Chunfeng Yun <
> chunfeng.yun@mediatek.com> wrote:
> > 
> > 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>
> > ---
> >  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];
> 
> Yeah, previous code had unnecessary NULL checking here.

yes, xhci_add/drop_endpoint() -> xhci_check_args() helps to check it,
it will return -EINVAL.

> 
> > +       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);
> 
> so tt_endpoint was revived, is this related to bugs?
No, it's no related with drop_endpoint issue, so keep it unchanged.
It helps to add debug info when encounter issue, we may want to find
all eps attached to a specific TT.

> 
> > 
> >         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);
> >  }
[skip]
> > 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;
> 
> I think a simple hashtable is well fit but also rhashtable
> had no problems to use.
Yes, rhashtable is also fine, but seems more complex, may introduce new
issues easier.

> 
> Overall, I suggest you merge this patch with reverting
> b8731209958a "usb: xhci-mtk: Do not use xhci's virt_dev in
> drop_endpoint"
> with some notes about the bugs I made.

Ok, they are all related with error handling:
1. when usb_add_hcd() -> xhci_mtk_setup() -> xhci_mtk_ssusb_config() or
xhci_gen_setup() failed, xhci_mtk_sch_init() will not be called, but
probe() will call xhci_mtk_sch_exit(), then NULL dereference oops
happens, because rhashtable is not init but try to destroy it.
2. in xhci_mtk_sch_init(), when alloc sch_array failed, need destroy
rhashtable;
3. in add_ep_quirk(), when rhashtable_insert_fast() failed, need
destroy sch_ep(), it's not added in bw_ep_chk_list.

Thanks a lot

> 
> IIUC, this patch looks like reviving mu3h_sch_tt::ep_list and
> changing rhash_table to hashtable. The incremental patch will be
> much shorter sized and clearer to understand.
> I'm not so clear what you are trying to fix.
> 
> Besides that, I don't have any objections to these changes. :)
> Thanks.
> 
> >         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
> > 
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[RFC PATCH 4/6] usb: xhci-mtk: allow bandwidth table rollover

[Chunfeng Yun]

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>
---
 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
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[RFC PATCH 5/6] dt-bindings: usb: mtk-xhci: support drop extra CS

[Chunfeng Yun]

Add optional property to drop extra CS for FS isoc/intr with TT,
this helps to release some bandwidth.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 .../devicetree/bindings/usb/mediatek,mtk-xhci.yaml          | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml b/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml
index 49729aab6d1a..963d4dedd4f2 100644
--- a/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml
+++ b/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml
@@ -123,7 +123,11 @@ properties:
       defined in the xHCI spec on MTK's controller.
     default: 5000
 
-  # the following properties are only used for case 1
+  mediatek,no-extra-cs:
+    description: do not add extra CS for FS intr/isoc eps with TT
+    type: boolean
+
+  # the foullowing properties are only used for case 1
   wakeup-source:
     description: enable USB remote wakeup, see power/wakeup-source.txt
     type: boolean
-- 
2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[RFC PATCH 6/6] usb: xhci-mtk: Add support drop extra CS

[Chunfeng Yun]

Support option to drop extra CS for FS isoc/intr whith TT.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/host/xhci-mtk-sch.c | 17 ++++++++++-------
 drivers/usb/host/xhci-mtk.c     |  1 +
 drivers/usb/host/xhci-mtk.h     |  2 ++
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
index 134f4789bd89..e5e9d930981f 100644
--- a/drivers/usb/host/xhci-mtk-sch.c
+++ b/drivers/usb/host/xhci-mtk-sch.c
@@ -280,6 +280,7 @@ create_sch_ep(struct xhci_hcd_mtk *mtk, struct usb_device *udev,
 	sch_ep->sch_tt = tt;
 	sch_ep->ep = ep;
 	sch_ep->speed = udev->speed;
+	sch_ep->no_extra_cs = mtk->no_extra_cs;
 	INIT_LIST_HEAD(&sch_ep->endpoint);
 	INIT_LIST_HEAD(&sch_ep->tt_endpoint);
 	INIT_HLIST_NODE(&sch_ep->hentry);
@@ -512,14 +513,16 @@ static int check_sch_tt(struct mu3h_sch_ep_info *sch_ep, u32 offset)
 		if (last_cs > 7)
 			return -ESCH_CS_OVERFLOW;
 
-		if (sch_ep->ep_type == ISOC_IN_EP)
-			extra_cs_count = (last_cs == 7) ? 1 : 2;
-		else /*  ep_type : INTR IN / INTR OUT */
-			extra_cs_count = 1;
+		if (!sch_ep->no_extra_cs) {
+			if (sch_ep->ep_type == ISOC_IN_EP)
+				extra_cs_count = (last_cs == 7) ? 1 : 2;
+			else /*  ep_type : INTR IN / INTR OUT */
+				extra_cs_count = 1;
 
-		cs_count += extra_cs_count;
-		if (cs_count > 7)
-			cs_count = 7; /* HW limit */
+			cs_count += extra_cs_count;
+			if (cs_count > 7)
+				cs_count = 7; /* HW limit */
+		}
 
 		sch_ep->cs_count = cs_count;
 		/* one for ss, the other for idle */
diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
index d66b6a8b59ba..da559d1db813 100644
--- a/drivers/usb/host/xhci-mtk.c
+++ b/drivers/usb/host/xhci-mtk.c
@@ -442,6 +442,7 @@ static int xhci_mtk_probe(struct platform_device *pdev)
 
 	mtk->lpm_support = of_property_read_bool(node, "usb3-lpm-capable");
 	mtk->u2_lpm_disable = of_property_read_bool(node, "usb2-lpm-disable");
+	mtk->no_extra_cs = of_property_read_bool(node, "mediatek,no-extra-cs");
 	/* optional property, ignore the error if it does not exist */
 	of_property_read_u32(node, "mediatek,u3p-dis-msk",
 			     &mtk->u3p_dis_msk);
diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
index 7cc0123eada0..bd8f7f7160f6 100644
--- a/drivers/usb/host/xhci-mtk.h
+++ b/drivers/usb/host/xhci-mtk.h
@@ -98,6 +98,7 @@ struct mu3h_sch_ep_info {
 	struct usb_host_endpoint *ep;
 	enum usb_device_speed speed;
 	bool allocated;
+	bool no_extra_cs;
 	/*
 	 * mtk xHCI scheduling information put into reserved DWs
 	 * in ep context
@@ -155,6 +156,7 @@ struct xhci_hcd_mtk {
 	unsigned int has_ippc:1;
 	unsigned int lpm_support:1;
 	unsigned int u2_lpm_disable:1;
+	unsigned int no_extra_cs:1;
 	/* usb remote wakeup */
 	unsigned int uwk_en:1;
 	struct regmap *uwk;
-- 
2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek