linux-mediatek.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
@ 2020-12-29  6:24 Ikjoon Jang
  2021-01-07 11:09 ` Mathias Nyman
  2021-01-08  6:34 ` Chunfeng Yun
  0 siblings, 2 replies; 12+ messages in thread
From: Ikjoon Jang @ 2020-12-29  6:24 UTC (permalink / raw)
  To: linux-mediatek, linux-usb
  Cc: Zhanyong Wang, Mathias Nyman, Greg Kroah-Hartman, linux-kernel,
	Tianping Fang, Matthias Brugger, Chunfeng Yun, Ikjoon Jang,
	linux-arm-kernel

xhci-mtk has hooks on add_endpoint() and drop_endpoint() from xhci
to handle its own sw bandwidth managements and stores bandwidth data
into internal table every time add_endpoint() is called,
so when bandwidth allocation fails at one endpoint, all earlier
allocation from the same interface could still remain at the table.

This patch adds two more hooks from check_bandwidth() and
reset_bandwidth(), and make mtk-xhci to releases all failed endpoints
from reset_bandwidth().

Fixes: 08e469de87a2 ("usb: xhci-mtk: supports bandwidth scheduling with multi-TT")
Signed-off-by: Ikjoon Jang <ikjn@chromium.org>

---

Changes in v5:
- Fix a wrong commit id in Fixes tag

Changes in v4:
- bugfix in v3, check_bandwidth() return uninitialized value
  when no new endpoints were added.
- change Fixes tag to keep dependency

Changes in v3:
- drop unrelated code cleanups
- change Fixes tag to keep dependency

Changes in v2:
- fix a 0-day warning from unused variable
- split one big patch into three patches
- fix wrong offset in mediatek hw flags

 drivers/usb/host/xhci-mtk-sch.c | 124 ++++++++++++++++++++++----------
 drivers/usb/host/xhci-mtk.h     |  13 ++++
 drivers/usb/host/xhci.c         |   9 +++
 3 files changed, 109 insertions(+), 37 deletions(-)

diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
index 45c54d56ecbd..95d20de9fd1f 100644
--- a/drivers/usb/host/xhci-mtk-sch.c
+++ b/drivers/usb/host/xhci-mtk-sch.c
@@ -200,6 +200,7 @@ static struct mu3h_sch_ep_info *create_sch_ep(struct usb_device *udev,
 
 	sch_ep->sch_tt = tt;
 	sch_ep->ep = ep;
+	INIT_LIST_HEAD(&sch_ep->tt_endpoint);
 
 	return sch_ep;
 }
@@ -583,6 +584,8 @@ int xhci_mtk_sch_init(struct xhci_hcd_mtk *mtk)
 
 	mtk->sch_array = sch_array;
 
+	INIT_LIST_HEAD(&mtk->bw_ep_list_new);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(xhci_mtk_sch_init);
@@ -601,19 +604,14 @@ int xhci_mtk_add_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
 	struct xhci_ep_ctx *ep_ctx;
 	struct xhci_slot_ctx *slot_ctx;
 	struct xhci_virt_device *virt_dev;
-	struct mu3h_sch_bw_info *sch_bw;
 	struct mu3h_sch_ep_info *sch_ep;
-	struct mu3h_sch_bw_info *sch_array;
 	unsigned int ep_index;
-	int bw_index;
-	int ret = 0;
 
 	xhci = hcd_to_xhci(hcd);
 	virt_dev = xhci->devs[udev->slot_id];
 	ep_index = xhci_get_endpoint_index(&ep->desc);
 	slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->in_ctx);
 	ep_ctx = xhci_get_ep_ctx(xhci, virt_dev->in_ctx, ep_index);
-	sch_array = mtk->sch_array;
 
 	xhci_dbg(xhci, "%s() type:%d, speed:%d, mpkt:%d, dir:%d, ep:%p\n",
 		__func__, usb_endpoint_type(&ep->desc), udev->speed,
@@ -632,39 +630,34 @@ int xhci_mtk_add_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
 		return 0;
 	}
 
-	bw_index = get_bw_index(xhci, udev, ep);
-	sch_bw = &sch_array[bw_index];
-
 	sch_ep = create_sch_ep(udev, ep, ep_ctx);
 	if (IS_ERR_OR_NULL(sch_ep))
 		return -ENOMEM;
 
 	setup_sch_info(udev, ep_ctx, sch_ep);
 
-	ret = check_sch_bw(udev, sch_bw, sch_ep);
-	if (ret) {
-		xhci_err(xhci, "Not enough bandwidth!\n");
-		if (is_fs_or_ls(udev->speed))
-			drop_tt(udev);
-
-		kfree(sch_ep);
-		return -ENOSPC;
-	}
+	list_add_tail(&sch_ep->endpoint, &mtk->bw_ep_list_new);
 
-	list_add_tail(&sch_ep->endpoint, &sch_bw->bw_ep_list);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xhci_mtk_add_ep_quirk);
 
-	ep_ctx->reserved[0] |= cpu_to_le32(EP_BPKTS(sch_ep->pkts)
-		| EP_BCSCOUNT(sch_ep->cs_count) | EP_BBM(sch_ep->burst_mode));
-	ep_ctx->reserved[1] |= cpu_to_le32(EP_BOFFSET(sch_ep->offset)
-		| EP_BREPEAT(sch_ep->repeat));
+static void xhci_mtk_drop_ep(struct xhci_hcd_mtk *mtk, struct usb_device *udev,
+			     struct mu3h_sch_ep_info *sch_ep)
+{
+	struct xhci_hcd *xhci = hcd_to_xhci(mtk->hcd);
+	int bw_index = get_bw_index(xhci, udev, sch_ep->ep);
+	struct mu3h_sch_bw_info *sch_bw = &mtk->sch_array[bw_index];
 
-	xhci_dbg(xhci, " PKTS:%x, CSCOUNT:%x, BM:%x, OFFSET:%x, REPEAT:%x\n",
-			sch_ep->pkts, sch_ep->cs_count, sch_ep->burst_mode,
-			sch_ep->offset, sch_ep->repeat);
+	update_bus_bw(sch_bw, sch_ep, 0);
+	list_del(&sch_ep->endpoint);
 
-	return 0;
+	if (sch_ep->sch_tt) {
+		list_del(&sch_ep->tt_endpoint);
+		drop_tt(udev);
+	}
+	kfree(sch_ep);
 }
-EXPORT_SYMBOL_GPL(xhci_mtk_add_ep_quirk);
 
 void xhci_mtk_drop_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
 		struct usb_host_endpoint *ep)
@@ -675,7 +668,7 @@ void xhci_mtk_drop_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
 	struct xhci_virt_device *virt_dev;
 	struct mu3h_sch_bw_info *sch_array;
 	struct mu3h_sch_bw_info *sch_bw;
-	struct mu3h_sch_ep_info *sch_ep;
+	struct mu3h_sch_ep_info *sch_ep, *tmp;
 	int bw_index;
 
 	xhci = hcd_to_xhci(hcd);
@@ -694,17 +687,74 @@ void xhci_mtk_drop_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
 	bw_index = get_bw_index(xhci, udev, ep);
 	sch_bw = &sch_array[bw_index];
 
-	list_for_each_entry(sch_ep, &sch_bw->bw_ep_list, endpoint) {
+	list_for_each_entry_safe(sch_ep, tmp, &sch_bw->bw_ep_list, endpoint) {
 		if (sch_ep->ep == ep) {
-			update_bus_bw(sch_bw, sch_ep, 0);
-			list_del(&sch_ep->endpoint);
-			if (is_fs_or_ls(udev->speed)) {
-				list_del(&sch_ep->tt_endpoint);
-				drop_tt(udev);
-			}
-			kfree(sch_ep);
-			break;
+			xhci_mtk_drop_ep(mtk, udev, sch_ep);
 		}
 	}
 }
 EXPORT_SYMBOL_GPL(xhci_mtk_drop_ep_quirk);
+
+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;
+	int bw_index, ret;
+
+	dev_dbg(&udev->dev, "%s\n", __func__);
+
+	if (list_empty(&mtk->bw_ep_list_new))
+		return 0;
+
+	list_for_each_entry(sch_ep, &mtk->bw_ep_list_new, endpoint) {
+		bw_index = get_bw_index(xhci, udev, sch_ep->ep);
+		sch_bw = &mtk->sch_array[bw_index];
+
+		ret = check_sch_bw(udev, sch_bw, sch_ep);
+		if (ret) {
+			xhci_err(xhci, "Not enough bandwidth!\n");
+			return -ENOSPC;
+		}
+	}
+
+	list_for_each_entry_safe(sch_ep, tmp, &mtk->bw_ep_list_new, 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);
+
+		bw_index = get_bw_index(xhci, udev, ep);
+		sch_bw = &mtk->sch_array[bw_index];
+
+		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)
+			| EP_BCSCOUNT(sch_ep->cs_count)
+			| EP_BBM(sch_ep->burst_mode));
+		ep_ctx->reserved[1] |= cpu_to_le32(EP_BOFFSET(sch_ep->offset)
+			| EP_BREPEAT(sch_ep->repeat));
+
+		xhci_dbg(xhci, " PKTS:%x, CSCOUNT:%x, BM:%x, OFFSET:%x, REPEAT:%x\n",
+			sch_ep->pkts, sch_ep->cs_count, sch_ep->burst_mode,
+			sch_ep->offset, sch_ep->repeat);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xhci_mtk_check_bandwidth);
+
+void xhci_mtk_reset_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
+{
+	struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
+	struct mu3h_sch_ep_info *sch_ep, *tmp;
+
+	dev_dbg(&udev->dev, "%s\n", __func__);
+
+	list_for_each_entry_safe(sch_ep, tmp, &mtk->bw_ep_list_new, endpoint) {
+		xhci_mtk_drop_ep(mtk, udev, sch_ep);
+	}
+}
+EXPORT_SYMBOL_GPL(xhci_mtk_reset_bandwidth);
diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
index a93cfe817904..577f431c5c93 100644
--- a/drivers/usb/host/xhci-mtk.h
+++ b/drivers/usb/host/xhci-mtk.h
@@ -130,6 +130,7 @@ struct mu3c_ippc_regs {
 struct xhci_hcd_mtk {
 	struct device *dev;
 	struct usb_hcd *hcd;
+	struct list_head bw_ep_list_new;
 	struct mu3h_sch_bw_info *sch_array;
 	struct mu3c_ippc_regs __iomem *ippc_regs;
 	bool has_ippc;
@@ -166,6 +167,8 @@ int xhci_mtk_add_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
 		struct usb_host_endpoint *ep);
 void xhci_mtk_drop_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
 		struct usb_host_endpoint *ep);
+int xhci_mtk_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev);
+void xhci_mtk_reset_bandwidth(struct usb_hcd *hcd, struct usb_device *udev);
 
 #else
 static inline int xhci_mtk_add_ep_quirk(struct usb_hcd *hcd,
@@ -179,6 +182,16 @@ static inline void xhci_mtk_drop_ep_quirk(struct usb_hcd *hcd,
 {
 }
 
+static inline int xhci_mtk_check_bandwidth(struct usb_hcd *hcd,
+		struct usb_device *udev)
+{
+	return 0;
+}
+
+static inline void xhci_mtk_reset_bandwidth(struct usb_hcd *hcd,
+		struct usb_device *udev)
+{
+}
 #endif
 
 #endif		/* _XHCI_MTK_H_ */
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index d4a8d0efbbc4..e1fcd3cf723f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -2882,6 +2882,12 @@ static int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
 	xhci_dbg(xhci, "%s called for udev %p\n", __func__, udev);
 	virt_dev = xhci->devs[udev->slot_id];
 
+	if (xhci->quirks & XHCI_MTK_HOST) {
+		ret = xhci_mtk_check_bandwidth(hcd, udev);
+		if (ret < 0)
+			return ret;
+	}
+
 	command = xhci_alloc_command(xhci, true, GFP_KERNEL);
 	if (!command)
 		return -ENOMEM;
@@ -2970,6 +2976,9 @@ static void xhci_reset_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
 		return;
 	xhci = hcd_to_xhci(hcd);
 
+	if (xhci->quirks & XHCI_MTK_HOST)
+		xhci_mtk_reset_bandwidth(hcd, udev);
+
 	xhci_dbg(xhci, "%s called for udev %p\n", __func__, udev);
 	virt_dev = xhci->devs[udev->slot_id];
 	/* Free any rings allocated for added endpoints */
-- 
2.29.2.729.g45daf8777d-goog


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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
  2020-12-29  6:24 [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data Ikjoon Jang
@ 2021-01-07 11:09 ` Mathias Nyman
  2021-01-08  2:56   ` Ikjoon Jang
  2021-01-08  6:11   ` Chunfeng Yun
  2021-01-08  6:34 ` Chunfeng Yun
  1 sibling, 2 replies; 12+ messages in thread
From: Mathias Nyman @ 2021-01-07 11:09 UTC (permalink / raw)
  To: Ikjoon Jang, linux-mediatek, linux-usb
  Cc: Zhanyong Wang, Mathias Nyman, Greg Kroah-Hartman, linux-kernel,
	Tianping Fang, Matthias Brugger, Chunfeng Yun, linux-arm-kernel

On 29.12.2020 8.24, Ikjoon Jang wrote:
> xhci-mtk has hooks on add_endpoint() and drop_endpoint() from xhci
> to handle its own sw bandwidth managements and stores bandwidth data
> into internal table every time add_endpoint() is called,
> so when bandwidth allocation fails at one endpoint, all earlier
> allocation from the same interface could still remain at the table.
> 
> This patch adds two more hooks from check_bandwidth() and
> reset_bandwidth(), and make mtk-xhci to releases all failed endpoints
> from reset_bandwidth().
> 
> Fixes: 08e469de87a2 ("usb: xhci-mtk: supports bandwidth scheduling with multi-TT")
> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> 

...

> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index d4a8d0efbbc4..e1fcd3cf723f 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -2882,6 +2882,12 @@ static int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
>  	xhci_dbg(xhci, "%s called for udev %p\n", __func__, udev);
>  	virt_dev = xhci->devs[udev->slot_id];
>  
> +	if (xhci->quirks & XHCI_MTK_HOST) {
> +		ret = xhci_mtk_check_bandwidth(hcd, udev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +

Just noticed that XHCI_MTK_HOST quirk is only set in xhci-mtk.c.
xhci-mtk.c calls xhci_init_driver(..., xhci_mtk_overrides) with a .reset override function.

why not add override functions for .check_bandwidth and .reset_bandwidth to xhci_mtk_overrides instead?

Another patch to add similar overrides for .add_endpoint and .drop_endpoint should probably be
done so that we can get rid of the xhci_mtk_add/drop_ep_quirk() calls in xhci.c as well

Thanks
-Mathias


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
  2021-01-07 11:09 ` Mathias Nyman
@ 2021-01-08  2:56   ` Ikjoon Jang
  2021-01-08  6:11   ` Chunfeng Yun
  1 sibling, 0 replies; 12+ messages in thread
From: Ikjoon Jang @ 2021-01-08  2:56 UTC (permalink / raw)
  To: Mathias Nyman
  Cc: Zhanyong Wang, Mathias Nyman, Greg Kroah-Hartman, linux-usb,
	open list, Tianping Fang, Matthias Brugger,
	moderated list:ARM/Mediatek SoC support, Chunfeng Yun,
	moderated list:ARM/Mediatek SoC support

On Thu, Jan 7, 2021 at 7:07 PM Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
>
> On 29.12.2020 8.24, Ikjoon Jang wrote:
> > xhci-mtk has hooks on add_endpoint() and drop_endpoint() from xhci
> > to handle its own sw bandwidth managements and stores bandwidth data
> > into internal table every time add_endpoint() is called,
> > so when bandwidth allocation fails at one endpoint, all earlier
> > allocation from the same interface could still remain at the table.
> >
> > This patch adds two more hooks from check_bandwidth() and
> > reset_bandwidth(), and make mtk-xhci to releases all failed endpoints
> > from reset_bandwidth().
> >
> > Fixes: 08e469de87a2 ("usb: xhci-mtk: supports bandwidth scheduling with multi-TT")
> > Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> >
>
> ...
>
> >
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index d4a8d0efbbc4..e1fcd3cf723f 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -2882,6 +2882,12 @@ static int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
> >       xhci_dbg(xhci, "%s called for udev %p\n", __func__, udev);
> >       virt_dev = xhci->devs[udev->slot_id];
> >
> > +     if (xhci->quirks & XHCI_MTK_HOST) {
> > +             ret = xhci_mtk_check_bandwidth(hcd, udev);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +
>
> Just noticed that XHCI_MTK_HOST quirk is only set in xhci-mtk.c.
> xhci-mtk.c calls xhci_init_driver(..., xhci_mtk_overrides) with a .reset override function.
>
> why not add override functions for .check_bandwidth and .reset_bandwidth to xhci_mtk_overrides instead?
>
> Another patch to add similar overrides for .add_endpoint and .drop_endpoint should probably be
> done so that we can get rid of the xhci_mtk_add/drop_ep_quirk() calls in xhci.c as well

Yes, I agree.
Let me submit another patch adding more overridables to xhci_driver_overrides.
Thanks.

>
> Thanks
> -Mathias
>

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
  2021-01-07 11:09 ` Mathias Nyman
  2021-01-08  2:56   ` Ikjoon Jang
@ 2021-01-08  6:11   ` Chunfeng Yun
  2021-01-08 14:46     ` Mathias Nyman
  1 sibling, 1 reply; 12+ messages in thread
From: Chunfeng Yun @ 2021-01-08  6:11 UTC (permalink / raw)
  To: Mathias Nyman
  Cc: Zhanyong Wang, Mathias Nyman, Greg Kroah-Hartman, linux-usb,
	linux-kernel, Tianping Fang, linux-mediatek, Matthias Brugger,
	Ikjoon Jang, linux-arm-kernel

On Thu, 2021-01-07 at 13:09 +0200, Mathias Nyman wrote:
> On 29.12.2020 8.24, Ikjoon Jang wrote:
> > xhci-mtk has hooks on add_endpoint() and drop_endpoint() from xhci
> > to handle its own sw bandwidth managements and stores bandwidth data
> > into internal table every time add_endpoint() is called,
> > so when bandwidth allocation fails at one endpoint, all earlier
> > allocation from the same interface could still remain at the table.
> > 
> > This patch adds two more hooks from check_bandwidth() and
> > reset_bandwidth(), and make mtk-xhci to releases all failed endpoints
> > from reset_bandwidth().
> > 
> > Fixes: 08e469de87a2 ("usb: xhci-mtk: supports bandwidth scheduling with multi-TT")
> > Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> > 
> 
> ...
> 
> > 
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index d4a8d0efbbc4..e1fcd3cf723f 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -2882,6 +2882,12 @@ static int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
> >  	xhci_dbg(xhci, "%s called for udev %p\n", __func__, udev);
> >  	virt_dev = xhci->devs[udev->slot_id];
> >  
> > +	if (xhci->quirks & XHCI_MTK_HOST) {
> > +		ret = xhci_mtk_check_bandwidth(hcd, udev);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> 
> Just noticed that XHCI_MTK_HOST quirk is only set in xhci-mtk.c.
> xhci-mtk.c calls xhci_init_driver(..., xhci_mtk_overrides) with a .reset override function.
> 
> why not add override functions for .check_bandwidth and .reset_bandwidth to xhci_mtk_overrides instead?
> 
> Another patch to add similar overrides for .add_endpoint and .drop_endpoint should probably be
> done so that we can get rid of the xhci_mtk_add/drop_ep_quirk() calls in xhci.c as well
You mean, we can export xhci_add/drop_endpoint()?

> 
> Thanks
> -Mathias
> 

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
  2020-12-29  6:24 [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data Ikjoon Jang
  2021-01-07 11:09 ` Mathias Nyman
@ 2021-01-08  6:34 ` Chunfeng Yun
  2021-01-08 10:50   ` Ikjoon Jang
  1 sibling, 1 reply; 12+ messages in thread
From: Chunfeng Yun @ 2021-01-08  6:34 UTC (permalink / raw)
  To: Ikjoon Jang
  Cc: Zhanyong Wang, Mathias Nyman, Greg Kroah-Hartman, linux-usb,
	linux-kernel, Tianping Fang, linux-mediatek, Matthias Brugger,
	linux-arm-kernel

On Tue, 2020-12-29 at 14:24 +0800, Ikjoon Jang wrote:
> xhci-mtk has hooks on add_endpoint() and drop_endpoint() from xhci
> to handle its own sw bandwidth managements and stores bandwidth data
> into internal table every time add_endpoint() is called,
> so when bandwidth allocation fails at one endpoint, all earlier
> allocation from the same interface could still remain at the table.
If failed to add an endpoint, will cause failure of its interface
config, then the other endpoints in the same interface will be dropped
later? you mean some endpoints in an interface may fail but without
affecting its function?

> 
> This patch adds two more hooks from check_bandwidth() and
> reset_bandwidth(), and make mtk-xhci to releases all failed endpoints
> from reset_bandwidth().
> 
> Fixes: 08e469de87a2 ("usb: xhci-mtk: supports bandwidth scheduling with multi-TT")
> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
  2021-01-08  6:34 ` Chunfeng Yun
@ 2021-01-08 10:50   ` Ikjoon Jang
  0 siblings, 0 replies; 12+ messages in thread
From: Ikjoon Jang @ 2021-01-08 10:50 UTC (permalink / raw)
  To: Chunfeng Yun
  Cc: Zhanyong Wang, Mathias Nyman, Greg Kroah-Hartman, linux-usb,
	open list, Tianping Fang,
	moderated list:ARM/Mediatek SoC support, Matthias Brugger,
	moderated list:ARM/Mediatek SoC support

On Fri, Jan 8, 2021 at 2:34 PM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
>
> On Tue, 2020-12-29 at 14:24 +0800, Ikjoon Jang wrote:
> > xhci-mtk has hooks on add_endpoint() and drop_endpoint() from xhci
> > to handle its own sw bandwidth managements and stores bandwidth data
> > into internal table every time add_endpoint() is called,
> > so when bandwidth allocation fails at one endpoint, all earlier
> > allocation from the same interface could still remain at the table.
> If failed to add an endpoint, will cause failure of its interface
> config, then the other endpoints in the same interface will be dropped
> later? you mean some endpoints in an interface may fail but without
> affecting its function?

Yes, drop_endpoint() is called for a failed interface when set_interface()
fails to switch alt settings, but set_configuration() does not call
drop_endpoint().
TT data seems to remain allocated until a device gets removed.

>
> >
> > This patch adds two more hooks from check_bandwidth() and
> > reset_bandwidth(), and make mtk-xhci to releases all failed endpoints
> > from reset_bandwidth().
> >
> > Fixes: 08e469de87a2 ("usb: xhci-mtk: supports bandwidth scheduling with multi-TT")
> > Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
>

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
  2021-01-08  6:11   ` Chunfeng Yun
@ 2021-01-08 14:46     ` Mathias Nyman
  2021-01-12  5:48       ` Ikjoon Jang
  0 siblings, 1 reply; 12+ messages in thread
From: Mathias Nyman @ 2021-01-08 14:46 UTC (permalink / raw)
  To: Chunfeng Yun
  Cc: Zhanyong Wang, Mathias Nyman, Greg Kroah-Hartman, linux-usb,
	linux-kernel, Tianping Fang, linux-mediatek, Matthias Brugger,
	Ikjoon Jang, linux-arm-kernel

On 8.1.2021 8.11, Chunfeng Yun wrote:
> On Thu, 2021-01-07 at 13:09 +0200, Mathias Nyman wrote:
>> On 29.12.2020 8.24, Ikjoon Jang wrote:
>>> xhci-mtk has hooks on add_endpoint() and drop_endpoint() from xhci
>>> to handle its own sw bandwidth managements and stores bandwidth data
>>> into internal table every time add_endpoint() is called,
>>> so when bandwidth allocation fails at one endpoint, all earlier
>>> allocation from the same interface could still remain at the table.
>>>
>>> This patch adds two more hooks from check_bandwidth() and
>>> reset_bandwidth(), and make mtk-xhci to releases all failed endpoints
>>> from reset_bandwidth().
>>>
>>> Fixes: 08e469de87a2 ("usb: xhci-mtk: supports bandwidth scheduling with multi-TT")
>>> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
>>>
>>
>> ...
>>
>>>
>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>> index d4a8d0efbbc4..e1fcd3cf723f 100644
>>> --- a/drivers/usb/host/xhci.c
>>> +++ b/drivers/usb/host/xhci.c
>>> @@ -2882,6 +2882,12 @@ static int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
>>>  	xhci_dbg(xhci, "%s called for udev %p\n", __func__, udev);
>>>  	virt_dev = xhci->devs[udev->slot_id];
>>>  
>>> +	if (xhci->quirks & XHCI_MTK_HOST) {
>>> +		ret = xhci_mtk_check_bandwidth(hcd, udev);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +	}
>>> +
>>
>> Just noticed that XHCI_MTK_HOST quirk is only set in xhci-mtk.c.
>> xhci-mtk.c calls xhci_init_driver(..., xhci_mtk_overrides) with a .reset override function.
>>
>> why not add override functions for .check_bandwidth and .reset_bandwidth to xhci_mtk_overrides instead?
>>
>> Another patch to add similar overrides for .add_endpoint and .drop_endpoint should probably be
>> done so that we can get rid of the xhci_mtk_add/drop_ep_quirk() calls in xhci.c as well
> You mean, we can export xhci_add/drop_endpoint()?

I think so, unless you have a better idea.
I prefer exporting the generic add/drop_endpoint functions rather than the vendor specific quirk functions.

-Mathias
  

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
  2021-01-08 14:46     ` Mathias Nyman
@ 2021-01-12  5:48       ` Ikjoon Jang
  2021-01-14  2:23         ` Chunfeng Yun
  2021-01-14  8:29         ` Chunfeng Yun
  0 siblings, 2 replies; 12+ messages in thread
From: Ikjoon Jang @ 2021-01-12  5:48 UTC (permalink / raw)
  To: Mathias Nyman
  Cc: Zhanyong Wang, Mathias Nyman, Greg Kroah-Hartman, linux-usb,
	open list, Tianping Fang, Matthias Brugger,
	moderated list:ARM/Mediatek SoC support, Chunfeng Yun,
	moderated list:ARM/Mediatek SoC support

On Fri, Jan 8, 2021 at 10:44 PM Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
>
> On 8.1.2021 8.11, Chunfeng Yun wrote:
> > On Thu, 2021-01-07 at 13:09 +0200, Mathias Nyman wrote:
> >> On 29.12.2020 8.24, Ikjoon Jang wrote:
> >>> xhci-mtk has hooks on add_endpoint() and drop_endpoint() from xhci
> >>> to handle its own sw bandwidth managements and stores bandwidth data
> >>> into internal table every time add_endpoint() is called,
> >>> so when bandwidth allocation fails at one endpoint, all earlier
> >>> allocation from the same interface could still remain at the table.
> >>>
> >>> This patch adds two more hooks from check_bandwidth() and
> >>> reset_bandwidth(), and make mtk-xhci to releases all failed endpoints
> >>> from reset_bandwidth().
> >>>
> >>> Fixes: 08e469de87a2 ("usb: xhci-mtk: supports bandwidth scheduling with multi-TT")
> >>> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> >>>
> >>
> >> ...
> >>
> >>>
> >>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> >>> index d4a8d0efbbc4..e1fcd3cf723f 100644
> >>> --- a/drivers/usb/host/xhci.c
> >>> +++ b/drivers/usb/host/xhci.c
> >>> @@ -2882,6 +2882,12 @@ static int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
> >>>     xhci_dbg(xhci, "%s called for udev %p\n", __func__, udev);
> >>>     virt_dev = xhci->devs[udev->slot_id];
> >>>
> >>> +   if (xhci->quirks & XHCI_MTK_HOST) {
> >>> +           ret = xhci_mtk_check_bandwidth(hcd, udev);
> >>> +           if (ret < 0)
> >>> +                   return ret;
> >>> +   }
> >>> +
> >>
> >> Just noticed that XHCI_MTK_HOST quirk is only set in xhci-mtk.c.
> >> xhci-mtk.c calls xhci_init_driver(..., xhci_mtk_overrides) with a .reset override function.
> >>
> >> why not add override functions for .check_bandwidth and .reset_bandwidth to xhci_mtk_overrides instead?
> >>
> >> Another patch to add similar overrides for .add_endpoint and .drop_endpoint should probably be
> >> done so that we can get rid of the xhci_mtk_add/drop_ep_quirk() calls in xhci.c as well
> > You mean, we can export xhci_add/drop_endpoint()?
>
> I think so, unless you have a better idea.
> I prefer exporting the generic add/drop_endpoint functions rather than the vendor specific quirk functions.
>

When moving out all MTK_HOST quirks and unlink xhci-mtk-sch from xhci,
xhci-mtk-sch still needs to touch the xhci internals, at least struct
xhci_ep_ctx.

My naive idea is just let xhci export one more function to expose xhci_ep_ctx.
But I'm not sure whether this is acceptable:

+struct xhci_ep_ctx* xhci_get_ep_contex(struct xhci_hcd *xhci, struct
usb_host_endpoint *ep)
+{ ... }
+EXPORT_SYMBOL(xhci_get_ep_context);

But for v6, I'm going to submit a patch with {check|reset}_bandwidth()
quirk function
 switched into xhci_driver_overrides first. (and preserve existing
MTK_HOST quirk functions).

Thanks!

> -Mathias
>

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
  2021-01-12  5:48       ` Ikjoon Jang
@ 2021-01-14  2:23         ` Chunfeng Yun
  2021-01-14  8:29         ` Chunfeng Yun
  1 sibling, 0 replies; 12+ messages in thread
From: Chunfeng Yun @ 2021-01-14  2:23 UTC (permalink / raw)
  To: Ikjoon Jang
  Cc: Zhanyong Wang, Mathias Nyman, Greg Kroah-Hartman, linux-usb,
	open list, Tianping Fang,
	moderated list:ARM/Mediatek SoC support, Matthias Brugger,
	Mathias Nyman, moderated list:ARM/Mediatek SoC support

On Tue, 2021-01-12 at 13:48 +0800, Ikjoon Jang wrote:
> On Fri, Jan 8, 2021 at 10:44 PM Mathias Nyman
> <mathias.nyman@linux.intel.com> wrote:
> >
> > On 8.1.2021 8.11, Chunfeng Yun wrote:
> > > On Thu, 2021-01-07 at 13:09 +0200, Mathias Nyman wrote:
> > >> On 29.12.2020 8.24, Ikjoon Jang wrote:
> > >>> xhci-mtk has hooks on add_endpoint() and drop_endpoint() from xhci
> > >>> to handle its own sw bandwidth managements and stores bandwidth data
> > >>> into internal table every time add_endpoint() is called,
> > >>> so when bandwidth allocation fails at one endpoint, all earlier
> > >>> allocation from the same interface could still remain at the table.
> > >>>
> > >>> This patch adds two more hooks from check_bandwidth() and
> > >>> reset_bandwidth(), and make mtk-xhci to releases all failed endpoints
> > >>> from reset_bandwidth().
> > >>>
> > >>> Fixes: 08e469de87a2 ("usb: xhci-mtk: supports bandwidth scheduling with multi-TT")
> > >>> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> > >>>
> > >>
> > >> ...
> > >>
> > >>>
> > >>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > >>> index d4a8d0efbbc4..e1fcd3cf723f 100644
> > >>> --- a/drivers/usb/host/xhci.c
> > >>> +++ b/drivers/usb/host/xhci.c
> > >>> @@ -2882,6 +2882,12 @@ static int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
> > >>>     xhci_dbg(xhci, "%s called for udev %p\n", __func__, udev);
> > >>>     virt_dev = xhci->devs[udev->slot_id];
> > >>>
> > >>> +   if (xhci->quirks & XHCI_MTK_HOST) {
> > >>> +           ret = xhci_mtk_check_bandwidth(hcd, udev);
> > >>> +           if (ret < 0)
> > >>> +                   return ret;
> > >>> +   }
> > >>> +
> > >>
> > >> Just noticed that XHCI_MTK_HOST quirk is only set in xhci-mtk.c.
> > >> xhci-mtk.c calls xhci_init_driver(..., xhci_mtk_overrides) with a .reset override function.
> > >>
> > >> why not add override functions for .check_bandwidth and .reset_bandwidth to xhci_mtk_overrides instead?
> > >>
> > >> Another patch to add similar overrides for .add_endpoint and .drop_endpoint should probably be
> > >> done so that we can get rid of the xhci_mtk_add/drop_ep_quirk() calls in xhci.c as well
> > > You mean, we can export xhci_add/drop_endpoint()?
> >
> > I think so, unless you have a better idea.
> > I prefer exporting the generic add/drop_endpoint functions rather than the vendor specific quirk functions.
> >
> 
> When moving out all MTK_HOST quirks and unlink xhci-mtk-sch from xhci,
> xhci-mtk-sch still needs to touch the xhci internals, at least struct
> xhci_ep_ctx.
> 
> My naive idea is just let xhci export one more function to expose xhci_ep_ctx.
> But I'm not sure whether this is acceptable:
Mathias suggest export xhci_add/drop_endpoint(), then we can add two new
functions as following in xhci-mtk.c:

xhci_mtk_add_endpoint()
{
    xhci_add_endpoint();
    xhci_mtk_add_ep_quirk();
}

xhci_mtk_drop_endpoint()
{
    xhci_mtk_drop_ep_quirk();
    xhci_drop_endpoint();
}

I'll try to test it

Thanks

> 
> +struct xhci_ep_ctx* xhci_get_ep_contex(struct xhci_hcd *xhci, struct
> usb_host_endpoint *ep)
> +{ ... }
> +EXPORT_SYMBOL(xhci_get_ep_context);
> 
> But for v6, I'm going to submit a patch with {check|reset}_bandwidth()
> quirk function
>  switched into xhci_driver_overrides first. (and preserve existing
> MTK_HOST quirk functions).
> 
> Thanks!
> 
> > -Mathias
> >

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
  2021-01-12  5:48       ` Ikjoon Jang
  2021-01-14  2:23         ` Chunfeng Yun
@ 2021-01-14  8:29         ` Chunfeng Yun
  2021-01-15  2:51           ` Ikjoon Jang
  1 sibling, 1 reply; 12+ messages in thread
From: Chunfeng Yun @ 2021-01-14  8:29 UTC (permalink / raw)
  To: Ikjoon Jang
  Cc: Zhanyong Wang, Mathias Nyman, Greg Kroah-Hartman, linux-usb,
	open list, Tianping Fang,
	moderated list:ARM/Mediatek SoC support, Matthias Brugger,
	Mathias Nyman, moderated list:ARM/Mediatek SoC support

Hi Ikjoon,

On Tue, 2021-01-12 at 13:48 +0800, Ikjoon Jang wrote:
> On Fri, Jan 8, 2021 at 10:44 PM Mathias Nyman
> <mathias.nyman@linux.intel.com> wrote:
> >
> > On 8.1.2021 8.11, Chunfeng Yun wrote:
> > > On Thu, 2021-01-07 at 13:09 +0200, Mathias Nyman wrote:
> > >> On 29.12.2020 8.24, Ikjoon Jang wrote:
> > >>> xhci-mtk has hooks on add_endpoint() and drop_endpoint() from xhci
> > >>> to handle its own sw bandwidth managements and stores bandwidth data
> > >>> into internal table every time add_endpoint() is called,
> > >>> so when bandwidth allocation fails at one endpoint, all earlier
> > >>> allocation from the same interface could still remain at the table.
> > >>>
> > >>> This patch adds two more hooks from check_bandwidth() and
> > >>> reset_bandwidth(), and make mtk-xhci to releases all failed endpoints
> > >>> from reset_bandwidth().
> > >>>
> > >>> Fixes: 08e469de87a2 ("usb: xhci-mtk: supports bandwidth scheduling with multi-TT")
> > >>> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> > >>>
> > >>
> > >> ...
> > >>
> > >>>
> > >>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > >>> index d4a8d0efbbc4..e1fcd3cf723f 100644
> > >>> --- a/drivers/usb/host/xhci.c
> > >>> +++ b/drivers/usb/host/xhci.c
> > >>> @@ -2882,6 +2882,12 @@ static int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
> > >>>     xhci_dbg(xhci, "%s called for udev %p\n", __func__, udev);
> > >>>     virt_dev = xhci->devs[udev->slot_id];
> > >>>
> > >>> +   if (xhci->quirks & XHCI_MTK_HOST) {
> > >>> +           ret = xhci_mtk_check_bandwidth(hcd, udev);
> > >>> +           if (ret < 0)
> > >>> +                   return ret;
> > >>> +   }
> > >>> +
> > >>
> > >> Just noticed that XHCI_MTK_HOST quirk is only set in xhci-mtk.c.
> > >> xhci-mtk.c calls xhci_init_driver(..., xhci_mtk_overrides) with a .reset override function.
> > >>
> > >> why not add override functions for .check_bandwidth and .reset_bandwidth to xhci_mtk_overrides instead?
> > >>
> > >> Another patch to add similar overrides for .add_endpoint and .drop_endpoint should probably be
> > >> done so that we can get rid of the xhci_mtk_add/drop_ep_quirk() calls in xhci.c as well
> > > You mean, we can export xhci_add/drop_endpoint()?
> >
> > I think so, unless you have a better idea.
> > I prefer exporting the generic add/drop_endpoint functions rather than the vendor specific quirk functions.
> >
> 
> When moving out all MTK_HOST quirks and unlink xhci-mtk-sch from xhci,
> xhci-mtk-sch still needs to touch the xhci internals, at least struct
> xhci_ep_ctx.
> 
> My naive idea is just let xhci export one more function to expose xhci_ep_ctx.
> But I'm not sure whether this is acceptable:
I find that xhci_add_endpoint() ignores some errors with return 0, for
these cases we needn't call xhci_mtk_add_ep-quirk(), so may be not a
good way to just export xhci_add_endpoint().

> 
> +struct xhci_ep_ctx* xhci_get_ep_contex(struct xhci_hcd *xhci, struct
> usb_host_endpoint *ep)
> +{ ... }
> +EXPORT_SYMBOL(xhci_get_ep_context);
> 
> But for v6, I'm going to submit a patch with {check|reset}_bandwidth()
> quirk function
>  switched into xhci_driver_overrides first. (and preserve existing
> MTK_HOST quirk functions).
> 
> Thanks!
> 
> > -Mathias
> >

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
  2021-01-14  8:29         ` Chunfeng Yun
@ 2021-01-15  2:51           ` Ikjoon Jang
  2021-01-19  2:33             ` Chunfeng Yun
  0 siblings, 1 reply; 12+ messages in thread
From: Ikjoon Jang @ 2021-01-15  2:51 UTC (permalink / raw)
  To: Chunfeng Yun
  Cc: Zhanyong Wang, Mathias Nyman, Greg Kroah-Hartman, linux-usb,
	open list, Tianping Fang,
	moderated list:ARM/Mediatek SoC support, Matthias Brugger,
	Mathias Nyman, moderated list:ARM/Mediatek SoC support

On Thu, Jan 14, 2021 at 4:30 PM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
>
> Hi Ikjoon,
>
> On Tue, 2021-01-12 at 13:48 +0800, Ikjoon Jang wrote:
> > On Fri, Jan 8, 2021 at 10:44 PM Mathias Nyman
> > <mathias.nyman@linux.intel.com> wrote:
> > >
> > > On 8.1.2021 8.11, Chunfeng Yun wrote:
> > > > On Thu, 2021-01-07 at 13:09 +0200, Mathias Nyman wrote:
> > > >> On 29.12.2020 8.24, Ikjoon Jang wrote:
> > > >>> xhci-mtk has hooks on add_endpoint() and drop_endpoint() from xhci
> > > >>> to handle its own sw bandwidth managements and stores bandwidth data
> > > >>> into internal table every time add_endpoint() is called,
> > > >>> so when bandwidth allocation fails at one endpoint, all earlier
> > > >>> allocation from the same interface could still remain at the table.
> > > >>>
> > > >>> This patch adds two more hooks from check_bandwidth() and
> > > >>> reset_bandwidth(), and make mtk-xhci to releases all failed endpoints
> > > >>> from reset_bandwidth().
> > > >>>
> > > >>> Fixes: 08e469de87a2 ("usb: xhci-mtk: supports bandwidth scheduling with multi-TT")
> > > >>> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> > > >>>
> > > >>
> > > >> ...
> > > >>
> > > >>>
> > > >>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > > >>> index d4a8d0efbbc4..e1fcd3cf723f 100644
> > > >>> --- a/drivers/usb/host/xhci.c
> > > >>> +++ b/drivers/usb/host/xhci.c
> > > >>> @@ -2882,6 +2882,12 @@ static int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
> > > >>>     xhci_dbg(xhci, "%s called for udev %p\n", __func__, udev);
> > > >>>     virt_dev = xhci->devs[udev->slot_id];
> > > >>>
> > > >>> +   if (xhci->quirks & XHCI_MTK_HOST) {
> > > >>> +           ret = xhci_mtk_check_bandwidth(hcd, udev);
> > > >>> +           if (ret < 0)
> > > >>> +                   return ret;
> > > >>> +   }
> > > >>> +
> > > >>
> > > >> Just noticed that XHCI_MTK_HOST quirk is only set in xhci-mtk.c.
> > > >> xhci-mtk.c calls xhci_init_driver(..., xhci_mtk_overrides) with a .reset override function.
> > > >>
> > > >> why not add override functions for .check_bandwidth and .reset_bandwidth to xhci_mtk_overrides instead?
> > > >>
> > > >> Another patch to add similar overrides for .add_endpoint and .drop_endpoint should probably be
> > > >> done so that we can get rid of the xhci_mtk_add/drop_ep_quirk() calls in xhci.c as well
> > > > You mean, we can export xhci_add/drop_endpoint()?
> > >
> > > I think so, unless you have a better idea.
> > > I prefer exporting the generic add/drop_endpoint functions rather than the vendor specific quirk functions.
> > >
> >
> > When moving out all MTK_HOST quirks and unlink xhci-mtk-sch from xhci,
> > xhci-mtk-sch still needs to touch the xhci internals, at least struct
> > xhci_ep_ctx.
> >
> > My naive idea is just let xhci export one more function to expose xhci_ep_ctx.
> > But I'm not sure whether this is acceptable:
> I find that xhci_add_endpoint() ignores some errors with return 0, for
> these cases we needn't call xhci_mtk_add_ep-quirk(), so may be not a
> good way to just export xhci_add_endpoint().

yeah, maybe that's from ep0 case?

And I've thought that we could also unlink xhci-mtk-sch from the xhci module
if MTK_HOST quirk functions are moved out to mtk platform driver's overrides.
I guess I've gone too far.

If we keep xhci-mtk-sch being built with the xhci module,
xhci-mtk-sch can directly access input control context and its drop/add flags,
so I think we can simply remove {add|drop}_endpoint() quirks and just handle
them all in {check|reset}_bandwidth() overrides.

>
> >
> > +struct xhci_ep_ctx* xhci_get_ep_contex(struct xhci_hcd *xhci, struct
> > usb_host_endpoint *ep)
> > +{ ... }
> > +EXPORT_SYMBOL(xhci_get_ep_context);
> >
> > But for v6, I'm going to submit a patch with {check|reset}_bandwidth()
> > quirk function
> >  switched into xhci_driver_overrides first. (and preserve existing
> > MTK_HOST quirk functions).
> >
> > Thanks!
> >
> > > -Mathias
> > >
>

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
  2021-01-15  2:51           ` Ikjoon Jang
@ 2021-01-19  2:33             ` Chunfeng Yun
  0 siblings, 0 replies; 12+ messages in thread
From: Chunfeng Yun @ 2021-01-19  2:33 UTC (permalink / raw)
  To: Ikjoon Jang
  Cc: Zhanyong Wang, Mathias Nyman, Greg Kroah-Hartman, linux-usb,
	open list, Tianping Fang,
	moderated list:ARM/Mediatek SoC support, Matthias Brugger,
	Mathias Nyman, moderated list:ARM/Mediatek SoC support

On Fri, 2021-01-15 at 10:51 +0800, Ikjoon Jang wrote:
> On Thu, Jan 14, 2021 at 4:30 PM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> >
> > Hi Ikjoon,
> >
> > On Tue, 2021-01-12 at 13:48 +0800, Ikjoon Jang wrote:
> > > On Fri, Jan 8, 2021 at 10:44 PM Mathias Nyman
> > > <mathias.nyman@linux.intel.com> wrote:
> > > >
> > > > On 8.1.2021 8.11, Chunfeng Yun wrote:
> > > > > On Thu, 2021-01-07 at 13:09 +0200, Mathias Nyman wrote:
> > > > >> On 29.12.2020 8.24, Ikjoon Jang wrote:
> > > > >>> xhci-mtk has hooks on add_endpoint() and drop_endpoint() from xhci
> > > > >>> to handle its own sw bandwidth managements and stores bandwidth data
> > > > >>> into internal table every time add_endpoint() is called,
> > > > >>> so when bandwidth allocation fails at one endpoint, all earlier
> > > > >>> allocation from the same interface could still remain at the table.
> > > > >>>
> > > > >>> This patch adds two more hooks from check_bandwidth() and
> > > > >>> reset_bandwidth(), and make mtk-xhci to releases all failed endpoints
> > > > >>> from reset_bandwidth().
> > > > >>>
> > > > >>> Fixes: 08e469de87a2 ("usb: xhci-mtk: supports bandwidth scheduling with multi-TT")
> > > > >>> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> > > > >>>
> > > > >>
> > > > >> ...
> > > > >>
> > > > >>>
> > > > >>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > > > >>> index d4a8d0efbbc4..e1fcd3cf723f 100644
> > > > >>> --- a/drivers/usb/host/xhci.c
> > > > >>> +++ b/drivers/usb/host/xhci.c
> > > > >>> @@ -2882,6 +2882,12 @@ static int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
> > > > >>>     xhci_dbg(xhci, "%s called for udev %p\n", __func__, udev);
> > > > >>>     virt_dev = xhci->devs[udev->slot_id];
> > > > >>>
> > > > >>> +   if (xhci->quirks & XHCI_MTK_HOST) {
> > > > >>> +           ret = xhci_mtk_check_bandwidth(hcd, udev);
> > > > >>> +           if (ret < 0)
> > > > >>> +                   return ret;
> > > > >>> +   }
> > > > >>> +
> > > > >>
> > > > >> Just noticed that XHCI_MTK_HOST quirk is only set in xhci-mtk.c.
> > > > >> xhci-mtk.c calls xhci_init_driver(..., xhci_mtk_overrides) with a .reset override function.
> > > > >>
> > > > >> why not add override functions for .check_bandwidth and .reset_bandwidth to xhci_mtk_overrides instead?
> > > > >>
> > > > >> Another patch to add similar overrides for .add_endpoint and .drop_endpoint should probably be
> > > > >> done so that we can get rid of the xhci_mtk_add/drop_ep_quirk() calls in xhci.c as well
> > > > > You mean, we can export xhci_add/drop_endpoint()?
> > > >
> > > > I think so, unless you have a better idea.
> > > > I prefer exporting the generic add/drop_endpoint functions rather than the vendor specific quirk functions.
> > > >
> > >
> > > When moving out all MTK_HOST quirks and unlink xhci-mtk-sch from xhci,
> > > xhci-mtk-sch still needs to touch the xhci internals, at least struct
> > > xhci_ep_ctx.
> > >
> > > My naive idea is just let xhci export one more function to expose xhci_ep_ctx.
> > > But I'm not sure whether this is acceptable:
> > I find that xhci_add_endpoint() ignores some errors with return 0, for
> > these cases we needn't call xhci_mtk_add_ep-quirk(), so may be not a
> > good way to just export xhci_add_endpoint().
> 
> yeah, maybe that's from ep0 case?
> 
> And I've thought that we could also unlink xhci-mtk-sch from the xhci module
> if MTK_HOST quirk functions are moved out to mtk platform driver's overrides.
> I guess I've gone too far.
> 
> If we keep xhci-mtk-sch being built with the xhci module,
> xhci-mtk-sch can directly access input control context and its drop/add flags,
> so I think we can simply remove {add|drop}_endpoint() quirks and just handle
> them all in {check|reset}_bandwidth() overrides.
It's ok if check_bandwidth() could get added ep and update context.

I'll spend some time to look at the code and test it.

Thank you

> 
> >
> > >
> > > +struct xhci_ep_ctx* xhci_get_ep_contex(struct xhci_hcd *xhci, struct
> > > usb_host_endpoint *ep)
> > > +{ ... }
> > > +EXPORT_SYMBOL(xhci_get_ep_context);
> > >
> > > But for v6, I'm going to submit a patch with {check|reset}_bandwidth()
> > > quirk function
> > >  switched into xhci_driver_overrides first. (and preserve existing
> > > MTK_HOST quirk functions).
> > >
> > > Thanks!
> > >
> > > > -Mathias
> > > >
> >

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-01-19  2:42 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-29  6:24 [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data Ikjoon Jang
2021-01-07 11:09 ` Mathias Nyman
2021-01-08  2:56   ` Ikjoon Jang
2021-01-08  6:11   ` Chunfeng Yun
2021-01-08 14:46     ` Mathias Nyman
2021-01-12  5:48       ` Ikjoon Jang
2021-01-14  2:23         ` Chunfeng Yun
2021-01-14  8:29         ` Chunfeng Yun
2021-01-15  2:51           ` Ikjoon Jang
2021-01-19  2:33             ` Chunfeng Yun
2021-01-08  6:34 ` Chunfeng Yun
2021-01-08 10:50   ` Ikjoon Jang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).