| Message ID | 20201229142406.v5.1.Id0d31b5f3ddf5e734d2ab11161ac5821921b1e1e@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5] usb: xhci-mtk: fix unreleased bandwidth data | expand |
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
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 >
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 >
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>
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> >
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
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 >
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 > >
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 > >
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 > > > >
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 > > > > > >
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 */
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(-)