LinuxLists.cc - [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks

[permalink] [raw]

Subject: Re: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks

On Tue, Jun 05, 2018 at 08:50:56PM +0530, Anurag Kumar Vulisha wrote:
> In xhci_plat_probe() both sysdev and pdev->dev are being used
> for finding quirks. There are some drivers(like dwc3 host.c)
> which adds quirks(like usb3-lpm-capable) into pdev and the logic
> present in xhci_plat_probe() checks for quirks in either sysdev
> or pdev for finding the quirks. Because of this logic, some of
> the quirks are getting missed(usb3-lpm-capable quirk added by dwc3
> host.c driver is getting missed).This patch fixes this by iterating
> over all the available parents for finding the quirks. In this way
> all the quirks which are present in child or parent are correctly
> updated.
>
> Signed-off-by: Anurag Kumar Vulisha <[email protected]>

So this is a bugfix? If so, how far back in the kernel releases should
it go to?

thanks,

greg k-h

2018-06-05 18:43:23

[permalink] [raw]

Subject: RE: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks

Hi Greg,

>-----Original Message-----
>From: Greg Kroah-Hartman [mailto:[email protected]]
>Sent: Tuesday, June 05, 2018 8:55 PM
>To: Anurag Kumar Vulisha <[email protected]>
>Cc: Mathias Nyman <[email protected]>; [email protected]; linux-
>[email protected]
>Subject: Re: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks
>
>On Tue, Jun 05, 2018 at 08:50:56PM +0530, Anurag Kumar Vulisha wrote:
>> In xhci_plat_probe() both sysdev and pdev->dev are being used for
>> finding quirks. There are some drivers(like dwc3 host.c) which adds
>> quirks(like usb3-lpm-capable) into pdev and the logic present in
>> xhci_plat_probe() checks for quirks in either sysdev or pdev for
>> finding the quirks. Because of this logic, some of the quirks are
>> getting missed(usb3-lpm-capable quirk added by dwc3 host.c driver is
>> getting missed).This patch fixes this by iterating over all the
>> available parents for finding the quirks. In this way all the quirks
>> which are present in child or parent are correctly updated.
>>
>> Signed-off-by: Anurag Kumar Vulisha <[email protected]>
>
>So this is a bugfix? If so, how far back in the kernel releases should it go to?
>
I feel this is as a bugfix. This problem started with this patch
"usb: xhci: use bus->sysdev for DMA configuration (4c39d4b949d36faf)"
where pdev->dev is replaced with sysdev. If I am not wrong this bug is
present since 4.12 kernel release.

Thanks,
Anurag kumar Vulisha

>thanks,
>
>greg k-h

2018-06-12 07:04:23

[permalink] [raw]

Subject: RE: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks

Hi Greg,

Please let me know if you have any comments on this patch.

Thanks,
Anurag Kumar Vulisha

>-----Original Message-----
>From: Anurag Kumar Vulisha
>Sent: Wednesday, June 06, 2018 12:11 AM
>To: Greg Kroah-Hartman <[email protected]>
>Cc: Mathias Nyman <[email protected]>; [email protected]; linux-
>[email protected]
>Subject: RE: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks
>
>Hi Greg,
>
>>-----Original Message-----
>>From: Greg Kroah-Hartman [mailto:[email protected]]
>>Sent: Tuesday, June 05, 2018 8:55 PM
>>To: Anurag Kumar Vulisha <[email protected]>
>>Cc: Mathias Nyman <[email protected]>; [email protected];
>>linux- [email protected]
>>Subject: Re: [PATCH] usb: host: xhci-plat: Iterate over parent nodes
>>for finding quirks
>>
>>On Tue, Jun 05, 2018 at 08:50:56PM +0530, Anurag Kumar Vulisha wrote:
>>> In xhci_plat_probe() both sysdev and pdev->dev are being used for
>>> finding quirks. There are some drivers(like dwc3 host.c) which adds
>>> quirks(like usb3-lpm-capable) into pdev and the logic present in
>>> xhci_plat_probe() checks for quirks in either sysdev or pdev for
>>> finding the quirks. Because of this logic, some of the quirks are
>>> getting missed(usb3-lpm-capable quirk added by dwc3 host.c driver is
>>> getting missed).This patch fixes this by iterating over all the
>>> available parents for finding the quirks. In this way all the quirks
>>> which are present in child or parent are correctly updated.
>>>
>>> Signed-off-by: Anurag Kumar Vulisha <[email protected]>
>>
>>So this is a bugfix? If so, how far back in the kernel releases should it go to?
>>
>I feel this is as a bugfix. This problem started with this patch
>"usb: xhci: use bus->sysdev for DMA configuration (4c39d4b949d36faf)"
>where pdev->dev is replaced with sysdev. If I am not wrong this bug is present since
>4.12 kernel release.
>
>Thanks,
>Anurag kumar Vulisha
>
>>thanks,
>>
>>greg k-h

2018-06-12 08:05:11

[permalink] [raw]

Subject: Re: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks

On Tue, Jun 12, 2018 at 07:03:39AM +0000, Anurag Kumar Vulisha wrote:
> Hi Greg,
>
> Please let me know if you have any comments on this patch.

It is the middle of the merge window, I can not do anything with any
patches until after 4.18-rc1 is out.

Please be patient.

thanks,

greg k-h

2018-07-14 15:38:15

[permalink] [raw]

Subject: RE: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks

HI Greg,

Ping!
Thought of giving a friendly remainder about this patch.

Thanks,
Anurag Kumar Vulisha

>-----Original Message-----
>From: Greg Kroah-Hartman [mailto:[email protected]]
>Sent: Tuesday, June 12, 2018 1:33 PM
>To: Anurag Kumar Vulisha <[email protected]>
>Cc: Mathias Nyman <[email protected]>; [email protected]; linux-
>[email protected]
>Subject: Re: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks
>
>On Tue, Jun 12, 2018 at 07:03:39AM +0000, Anurag Kumar Vulisha wrote:
>> Hi Greg,
>>
>> Please let me know if you have any comments on this patch.
>
>It is the middle of the merge window, I can not do anything with any patches until
>after 4.18-rc1 is out.
>
>Please be patient.
>
>thanks,
>
>greg k-h

2018-07-15 08:03:27

[permalink] [raw]

Subject: Re: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks

On Sat, Jul 14, 2018 at 03:37:20PM +0000, Anurag Kumar Vulisha wrote:
> HI Greg,
>
> Ping!
> Thought of giving a friendly remainder about this patch.

I am not the xhci maintainer, I need to wait from an ack from him before
I can take it.

thanks,

greg k-h

2018-07-15 08:12:29

[permalink] [raw]

Subject: Re: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks

On Sun, Jul 15, 2018 at 10:02:06AM +0200, Greg Kroah-Hartman wrote:
> On Sat, Jul 14, 2018 at 03:37:20PM +0000, Anurag Kumar Vulisha wrote:
> > HI Greg,
> >
> > Ping!
> > Thought of giving a friendly remainder about this patch.
>
> I am not the xhci maintainer, I need to wait from an ack from him before
> I can take it.

And he's on vacation until later this week, so just relax.

greg k-h

2018-07-20 14:40:38

[permalink] [raw]

Subject: Re: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks

Hi

Sorry about the delay with this patch,
I have a couple comments inline.

On 05.06.2018 18:20, Anurag Kumar Vulisha wrote:
> In xhci_plat_probe() both sysdev and pdev->dev are being used
> for finding quirks. There are some drivers(like dwc3 host.c)
> which adds quirks(like usb3-lpm-capable) into pdev and the logic
> present in xhci_plat_probe() checks for quirks in either sysdev
> or pdev for finding the quirks. Because of this logic, some of
> the quirks are getting missed(usb3-lpm-capable quirk added by dwc3
> host.c driver is getting missed).This patch fixes this by iterating
> over all the available parents for finding the quirks. In this way
> all the quirks which are present in child or parent are correctly
> updated.
>
> Signed-off-by: Anurag Kumar Vulisha <[email protected]>
> ---
> drivers/usb/host/xhci-plat.c | 29 ++++++++++++++++++-----------
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index c1b22fc..0cd0489 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -152,7 +152,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> {
> const struct xhci_plat_priv *priv_match;
> const struct hc_driver *driver;
> - struct device *sysdev;
> + struct device *sysdev, *tmpdev;
> struct xhci_hcd *xhci;
> struct resource *res;
> struct usb_hcd *hcd;
> @@ -272,19 +272,26 @@ static int xhci_plat_probe(struct platform_device *pdev)
> goto disable_clk;
> }
>
> - if (device_property_read_bool(sysdev, "usb2-lpm-disable"))
> - xhci->quirks |= XHCI_HW_LPM_DISABLE;
> + /* Iterate over all parent nodes for finding quirks */
> + for (tmpdev = &pdev->dev; tmpdev; tmpdev = tmpdev->parent) {

Isn't sysdev at this point the topmost device that can have any of those
device properties set?
We could loop from &pdev->dev up to and including sysdev.

It doesn't matter much but maybe avoid walking some extra parents.

>
> - if (device_property_read_bool(sysdev, "usb3-lpm-capable"))
> - xhci->quirks |= XHCI_LPM_SUPPORT;
> + if (device_property_read_bool(tmpdev, "usb2-lpm-disable"))
> + xhci->quirks |= XHCI_HW_LPM_DISABLE;
>
> - if (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
> - xhci->quirks |= XHCI_BROKEN_PORT_PED;
> + if (device_property_read_bool(tmpdev, "usb3-lpm-capable"))
> + xhci->quirks |= XHCI_LPM_SUPPORT;
>
> - /* imod_interval is the interrupt moderation value in nanoseconds. */
> - xhci->imod_interval = 40000;

Setting the default imod_interval could be moved before the for() loop

> - device_property_read_u32(sysdev, "imod-interval-ns",
> - &xhci->imod_interval);
> + if (device_property_read_bool(tmpdev, "quirk-broken-port-ped"))
> + xhci->quirks |= XHCI_BROKEN_PORT_PED;
> +
> + /*
> + * imod_interval is the interrupt moderation
> + * value in nanoseconds.
> + */
> + xhci->imod_interval = 40000;
> + device_property_read_u32(tmpdev, "imod-interval-ns",
> + &xhci->imod_interval);
> + }
>
> hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev, "usb-phy", 0);
> if (IS_ERR(hcd->usb_phy)) {
>

Otherwise everything looks fine.
I will unfortunately be away again for another two weeks.

-Mathias

2018-07-20 14:52:20

[permalink] [raw]

Subject: Re: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks

On 15.07.2018 11:11, Greg Kroah-Hartman wrote:
> On Sun, Jul 15, 2018 at 10:02:06AM +0200, Greg Kroah-Hartman wrote:
>> On Sat, Jul 14, 2018 at 03:37:20PM +0000, Anurag Kumar Vulisha wrote:
>>> HI Greg,
>>>
>>> Ping!
>>> Thought of giving a friendly remainder about this patch.
>>
>> I am not the xhci maintainer, I need to wait from an ack from him before
>> I can take it.
>
> And he's on vacation until later this week, so just relax.
>

I added a couple minor comments.
I will be away for two more weeks, and I don't want to block this,
so if it is important to get this to 4.18 I won't object picking it as is.

-Mathias

2018-07-20 15:40:54

[permalink] [raw]

Subject: RE: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks

Hi Mathias,

Thanks for providing your comments, please find my comments inline

>-----Original Message-----
>From: Mathias Nyman [mailto:[email protected]]
>Sent: Friday, July 20, 2018 8:13 PM
>To: Anurag Kumar Vulisha <[email protected]>; Greg Kroah-Hartman
><[email protected]>
>Cc: [email protected]; [email protected]
>Subject: Re: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks
>
>Hi
>
>Sorry about the delay with this patch,
>I have a couple comments inline.
>

No probs, I can understand. I was having some internal dependency on this patch,
so pinged again. Sorry for troubling you.

>On 05.06.2018 18:20, Anurag Kumar Vulisha wrote:
>> In xhci_plat_probe() both sysdev and pdev->dev are being used for
>> finding quirks. There are some drivers(like dwc3 host.c) which adds
>> quirks(like usb3-lpm-capable) into pdev and the logic present in
>> xhci_plat_probe() checks for quirks in either sysdev or pdev for
>> finding the quirks. Because of this logic, some of the quirks are
>> getting missed(usb3-lpm-capable quirk added by dwc3 host.c driver is
>> getting missed).This patch fixes this by iterating over all the
>> available parents for finding the quirks. In this way all the quirks
>> which are present in child or parent are correctly updated.
>>
>> Signed-off-by: Anurag Kumar Vulisha <[email protected]>
>> ---
>> drivers/usb/host/xhci-plat.c | 29 ++++++++++++++++++-----------
>> 1 file changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-plat.c
>> b/drivers/usb/host/xhci-plat.c index c1b22fc..0cd0489 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -152,7 +152,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>> {
>> const struct xhci_plat_priv *priv_match;
>> const struct hc_driver *driver;
>> - struct device *sysdev;
>> + struct device *sysdev, *tmpdev;
>> struct xhci_hcd *xhci;
>> struct resource *res;
>> struct usb_hcd *hcd;
>> @@ -272,19 +272,26 @@ static int xhci_plat_probe(struct platform_device *pdev)
>> goto disable_clk;
>> }
>>
>> - if (device_property_read_bool(sysdev, "usb2-lpm-disable"))
>> - xhci->quirks |= XHCI_HW_LPM_DISABLE;
>> + /* Iterate over all parent nodes for finding quirks */
>> + for (tmpdev = &pdev->dev; tmpdev; tmpdev = tmpdev->parent) {
>
>Isn't sysdev at this point the topmost device that can have any of those device
>properties set?
>We could loop from &pdev->dev up to and including sysdev.
>
>It doesn't matter much but maybe avoid walking some extra parents.
>

I have seen some drivers ( like dwc3 host.c) which create a child dev, which is
the pdev that is being passed to xhci_plat_probe(). So, sysdev may not be the
topmost parent. There could be some properties which may be present in parent
and may not be populated in the child. So, I made this change to search on all
available parents for finding a valid property

>>
>> - if (device_property_read_bool(sysdev, "usb3-lpm-capable"))
>> - xhci->quirks |= XHCI_LPM_SUPPORT;
>> + if (device_property_read_bool(tmpdev, "usb2-lpm-disable"))
>> + xhci->quirks |= XHCI_HW_LPM_DISABLE;
>>
>> - if (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
>> - xhci->quirks |= XHCI_BROKEN_PORT_PED;
>> + if (device_property_read_bool(tmpdev, "usb3-lpm-capable"))
>> + xhci->quirks |= XHCI_LPM_SUPPORT;
>>
>> - /* imod_interval is the interrupt moderation value in nanoseconds. */
>> - xhci->imod_interval = 40000;
>
>Setting the default imod_interval could be moved before the for() loop
>

I thought it would be better if everything related to imod_interval is in one place,
so kept it as is. I can fix this in v2 if you suggest.

>> - device_property_read_u32(sysdev, "imod-interval-ns",
>> - &xhci->imod_interval);
>> + if (device_property_read_bool(tmpdev, "quirk-broken-port-ped"))
>> + xhci->quirks |= XHCI_BROKEN_PORT_PED;
>> +
>> + /*
>> + * imod_interval is the interrupt moderation
>> + * value in nanoseconds.
>> + */
>> + xhci->imod_interval = 40000;
>> + device_property_read_u32(tmpdev, "imod-interval-ns",
>> + &xhci->imod_interval);
>> + }
>>
>> hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev, "usb-phy", 0);
>> if (IS_ERR(hcd->usb_phy)) {
>>
>
>Otherwise everything looks fine.
>I will unfortunately be away again for another two weeks.

No probs. If you feel there is no harm in applying and satisfied with the changes, it
would be helpful if you can apply this patch. This is because I have some dependency
on this patch waiting to be applied. Sorry for troubling you again.

Thanks,
Anurag Kumar Vulisha

>
>-Mathias

2018-08-06 08:00:00

[permalink] [raw]

Subject: RE: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks

Hi Greg,

>-----Original Message-----
>From: Mathias Nyman [mailto:[email protected]]
>Sent: Friday, July 20, 2018 8:24 PM
>To: Greg Kroah-Hartman <[email protected]>; Anurag Kumar Vulisha
><[email protected]>
>Cc: [email protected]; [email protected];
>[email protected]
>Subject: Re: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks
>
>On 15.07.2018 11:11, Greg Kroah-Hartman wrote:
>> On Sun, Jul 15, 2018 at 10:02:06AM +0200, Greg Kroah-Hartman wrote:
>>> On Sat, Jul 14, 2018 at 03:37:20PM +0000, Anurag Kumar Vulisha wrote:
>>>> HI Greg,
>>>>
>>>> Ping!
>>>> Thought of giving a friendly remainder about this patch.
>>>
>>> I am not the xhci maintainer, I need to wait from an ack from him
>>> before I can take it.
>>
>> And he's on vacation until later this week, so just relax.
>>
>
>I added a couple minor comments.
>I will be away for two more weeks, and I don't want to block this, so if it is important
>to get this to 4.18 I won't object picking it as is.
>

I have replied to the comments that Mathias has previously given on this patch and waiting
on his reply, but looks like Mathias is on vacation. Since he is okay with applying the patch,
would it be possible to get this patch applied? If you have any suggestions I can work on
fixing them and re-send the patch. Thanks for your help in this regard.

Regards,
Anurag Kumar Vulisha

>-Mathias

2018-08-06 10:55:13

[permalink] [raw]

Subject: Re: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks

Hi,

Back from vacation

On 20.07.2018 18:39, Anurag Kumar Vulisha wrote:
>
> Hi Mathias,
>
> Thanks for providing your comments, please find my comments inline
>
>>>
>>> - if (device_property_read_bool(sysdev, "usb2-lpm-disable"))
>>> - xhci->quirks |= XHCI_HW_LPM_DISABLE;
>>> + /* Iterate over all parent nodes for finding quirks */
>>> + for (tmpdev = &pdev->dev; tmpdev; tmpdev = tmpdev->parent) {
>>
>> Isn't sysdev at this point the topmost device that can have any of those device
>> properties set?
>> We could loop from &pdev->dev up to and including sysdev.
>>
>> It doesn't matter much but maybe avoid walking some extra parents.
>>>
> I have seen some drivers ( like dwc3 host.c) which create a child dev, which is
> the pdev that is being passed to xhci_plat_probe(). So, sysdev may not be the
> topmost parent. There could be some properties which may be present in parent
> and may not be populated in the child. So, I made this change to search on all
> available parents for finding a valid property

Ok, lets keep it like you wrote it

>
>>>
>>> - if (device_property_read_bool(sysdev, "usb3-lpm-capable"))
>>> - xhci->quirks |= XHCI_LPM_SUPPORT;
>>> + if (device_property_read_bool(tmpdev, "usb2-lpm-disable"))
>>> + xhci->quirks |= XHCI_HW_LPM_DISABLE;
>>>
>>> - if (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
>>> - xhci->quirks |= XHCI_BROKEN_PORT_PED;
>>> + if (device_property_read_bool(tmpdev, "usb3-lpm-capable"))
>>> + xhci->quirks |= XHCI_LPM_SUPPORT;
>>>
>>> - /* imod_interval is the interrupt moderation value in nanoseconds. */
>>> - xhci->imod_interval = 40000;
>>
>> Setting the default imod_interval could be moved before the for() loop
>>
>
> I thought it would be better if everything related to imod_interval is in one place,
> so kept it as is. I can fix this in v2 if you suggest.

Default imod_interval needs to be set before the for loop to avoid overwriting a value
got from device property

So a v2 is needed

-Mathias

2018-08-06 11:22:54

[permalink] [raw]

Subject: RE: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks

Hi Mathais,
>>
>>>>
>>>> - if (device_property_read_bool(sysdev, "usb3-lpm-capable"))
>>>> - xhci->quirks |= XHCI_LPM_SUPPORT;
>>>> + if (device_property_read_bool(tmpdev, "usb2-lpm-disable"))
>>>> + xhci->quirks |= XHCI_HW_LPM_DISABLE;
>>>>
>>>> - if (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
>>>> - xhci->quirks |= XHCI_BROKEN_PORT_PED;
>>>> + if (device_property_read_bool(tmpdev, "usb3-lpm-capable"))
>>>> + xhci->quirks |= XHCI_LPM_SUPPORT;
>>>>
>>>> - /* imod_interval is the interrupt moderation value in nanoseconds. */
>>>> - xhci->imod_interval = 40000;
>>>
>>> Setting the default imod_interval could be moved before the for()
>>> loop
>>>
>>
>> I thought it would be better if everything related to imod_interval is
>> in one place, so kept it as is. I can fix this in v2 if you suggest.
>
>Default imod_interval needs to be set before the for loop to avoid overwriting a value
>got from device property
>
>So a v2 is needed
>

Thanks for correcting, will fix and send v2

Thanks,
Anurag Kumar Vulisha

>-Mathias

2018-08-06 12:50:48

[permalink] [raw]

Subject: RE: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks

>>
>> I have replied to the comments that Mathias has previously given on
>> this patch and waiting on his reply, but looks like Mathias is on
>> vacation. Since he is okay with applying the patch, would it be
>> possible to get this patch applied? If you have any suggestions I can work on fixing
>them and re-send the patch. Thanks for your help in this regard.
>>
>
>I'm back.
>Just noted that there is one change still needed, (commented on actual patch) I'll pick
>up the next version of the patch, but we are in rc8 already so your fix will go to 4.19-
>rc2 earliest.
>
Hi Mathias,

4.19-rc2 is fine. I have corrected the patch and sent a v2. Thanks a lot for
your help,

Regards,
Anurag Kumar Vulisha

2018-08-06 13:02:44