diff mbox series

[v5] usb: xhci-mtk: fix unreleased bandwidth data

Message ID 20201229142406.v5.1.Id0d31b5f3ddf5e734d2ab11161ac5821921b1e1e@changeid
State New
Headers show
Series [v5] usb: xhci-mtk: fix unreleased bandwidth data | expand

Commit Message

Ikjoon Jang Dec. 29, 2020, 6:24 a.m. UTC
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(-)

Comments

Mathias Nyman Jan. 7, 2021, 11:09 a.m. UTC | #1
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
Ikjoon Jang Jan. 8, 2021, 2:56 a.m. UTC | #2
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

>
Chunfeng Yun (云春峰) Jan. 8, 2021, 6:11 a.m. UTC | #3
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

>
Chunfeng Yun (云春峰) Jan. 8, 2021, 6:34 a.m. UTC | #4
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>
Ikjoon Jang Jan. 8, 2021, 10:50 a.m. UTC | #5
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>

>
Mathias Nyman Jan. 8, 2021, 2:46 p.m. UTC | #6
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
Ikjoon Jang Jan. 12, 2021, 5:48 a.m. UTC | #7
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

>
Chunfeng Yun (云春峰) Jan. 14, 2021, 2:23 a.m. UTC | #8
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

> >
Chunfeng Yun (云春峰) Jan. 14, 2021, 8:29 a.m. UTC | #9
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

> >
Ikjoon Jang Jan. 15, 2021, 2:51 a.m. UTC | #10
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

> > >

>
Chunfeng Yun (云春峰) Jan. 19, 2021, 2:33 a.m. UTC | #11
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 mbox series

Patch

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