Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
On Fri, 2021-01-15 at 10:51 +0800, Ikjoon Jang wrote:
> On Thu, Jan 14, 2021 at 4:30 PM Chunfeng Yun
> 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
> > > 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
> > > > >>>
> > > > >>
> > > > >> ...
> > > > >>
> > > > >>>
> > > > >>> 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
> > > >
> >
Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
On Thu, Jan 14, 2021 at 4:30 PM Chunfeng Yun 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
> > 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
> > > >>>
> > > >>
> > > >> ...
> > > >>
> > > >>>
> > > >>> 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
> > >
>
Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
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
> 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
> > >>>
> > >>
> > >> ...
> > >>
> > >>>
> > >>> 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
> >
Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
On Tue, 2021-01-12 at 13:48 +0800, Ikjoon Jang wrote:
> On Fri, Jan 8, 2021 at 10:44 PM Mathias Nyman
> 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
> > >>>
> > >>
> > >> ...
> > >>
> > >>>
> > >>> 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
> >
Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
On Fri, Jan 8, 2021 at 10:44 PM Mathias Nyman
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
> >>>
> >>
> >> ...
> >>
> >>>
> >>> 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
>
Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
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
>>>
>>
>> ...
>>
>>>
>>> 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
Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
On Fri, Jan 8, 2021 at 2:34 PM Chunfeng Yun 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
>
Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
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
Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
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
> >
>
> ...
>
> >
> > 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
>
Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
On Thu, Jan 7, 2021 at 7:07 PM 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
> >
>
> ...
>
> >
> > 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
>
Re: [PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
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
>
...
>
> 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
[PATCH v5] usb: xhci-mtk: fix unreleased bandwidth data
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
---
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(_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(>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(>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(>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 = _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(_ep->endpoint, >bw_ep_list_new);
- list_add_tail(_ep->endpoint, _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 = >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(_ep->endpoint);
- return 0;
+ if (sch_ep->sch_tt) {
+ list_del(_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;
-
