[PATCH] usb: dwc3: leave default DMA for PCI devices

[PATCH] usb: dwc3: leave default DMA for PCI devices

[Fabio Aiuto]

in case of a PCI dwc3 controller, leave the default DMA
mask. Calling of a 64 bit DMA mask breaks the driver on
cherrytrail based tablets like Cyberbook T116.

Fixes: 45d39448b4d0 ("usb: dwc3: support 64 bit DMA in platform driver")
Reported-by: Hans De Goede <hdegoede@redhat.com>
Tested-by: Fabio Aiuto <fabioaiuto83@gmail.com>
Signed-off-by: Fabio Aiuto <fabioaiuto83@gmail.com>
---
 drivers/usb/dwc3/core.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 643239d7d370..f4c09951b517 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1594,9 +1594,11 @@ static int dwc3_probe(struct platform_device *pdev)
 
 	dwc3_get_properties(dwc);
 
-	ret = dma_set_mask_and_coherent(dwc->sysdev, DMA_BIT_MASK(64));
-	if (ret)
-		return ret;
+	if (!dwc->sysdev_is_parent) {
+		ret = dma_set_mask_and_coherent(dwc->sysdev, DMA_BIT_MASK(64));
+		if (ret)
+			return ret;
+	}
 
 	dwc->reset = devm_reset_control_array_get_optional_shared(dev);
 	if (IS_ERR(dwc->reset))
-- 
2.30.2

Re: [PATCH] usb: dwc3: leave default DMA for PCI devices

[Sven Peter]

On Sat, Nov 13, 2021, at 15:29, Fabio Aiuto wrote:
> in case of a PCI dwc3 controller, leave the default DMA
> mask. Calling of a 64 bit DMA mask breaks the driver on
> cherrytrail based tablets like Cyberbook T116.
>
> Fixes: 45d39448b4d0 ("usb: dwc3: support 64 bit DMA in platform driver")
> Reported-by: Hans De Goede <hdegoede@redhat.com>
> Tested-by: Fabio Aiuto <fabioaiuto83@gmail.com>
> Signed-off-by: Fabio Aiuto <fabioaiuto83@gmail.com>
> ---
>  drivers/usb/dwc3/core.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 643239d7d370..f4c09951b517 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1594,9 +1594,11 @@ static int dwc3_probe(struct platform_device *pdev)
> 
>  	dwc3_get_properties(dwc);
> 
> -	ret = dma_set_mask_and_coherent(dwc->sysdev, DMA_BIT_MASK(64));
> -	if (ret)
> -		return ret;
> +	if (!dwc->sysdev_is_parent) {


Are you sure it's the dwc3 controller that limits DMA to 32 bit addresses and
not the PCI bus itself?
I also think the xhci driver will call dma_set_mask_and_coherent again
later on when dwc3 is used in host mode.



Sven

Re: [PATCH] usb: dwc3: leave default DMA for PCI devices

[Hans de Goede]

Hi,

On 11/14/21 12:56, Sven Peter wrote:
> On Sat, Nov 13, 2021, at 15:29, Fabio Aiuto wrote:
>> in case of a PCI dwc3 controller, leave the default DMA
>> mask. Calling of a 64 bit DMA mask breaks the driver on
>> cherrytrail based tablets like Cyberbook T116.
>>
>> Fixes: 45d39448b4d0 ("usb: dwc3: support 64 bit DMA in platform driver")
>> Reported-by: Hans De Goede <hdegoede@redhat.com>
>> Tested-by: Fabio Aiuto <fabioaiuto83@gmail.com>
>> Signed-off-by: Fabio Aiuto <fabioaiuto83@gmail.com>
>> ---
>>  drivers/usb/dwc3/core.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 643239d7d370..f4c09951b517 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1594,9 +1594,11 @@ static int dwc3_probe(struct platform_device *pdev)
>>
>>  	dwc3_get_properties(dwc);
>>
>> -	ret = dma_set_mask_and_coherent(dwc->sysdev, DMA_BIT_MASK(64));
>> -	if (ret)
>> -		return ret;
>> +	if (!dwc->sysdev_is_parent) {
> 
> 
> Are you sure it's the dwc3 controller that limits DMA to 32 bit addresses and
> not the PCI bus itself?

This is unknown, until the commit which added the dma_set_mask call
everything worked fine; and that commit was found with a git-bisect.

But since these SoCs can never have more then 4G RAM it makes sense for
both the bus and the DWC3 to be limited to 32 bit DMA only, since everything
else is just wasted extra silicon.

The dwc3 driver has this somewhat convoluted design where the PCI-driver
instantiates a platform-device and then a platform-driver is used instead of
having some generic core + separate platform and PCI drivers.

Fixing this convoluted design is way out of scope for fixing the *regression* with
DWC3 on Intel Cherry Trail (and likely also Bay Trails) SoCs, since that
requires a lot of refactoring.

So taken this convoluted design as a given, then it is clear that the platform-driver
should not be calling dma_set_mask on the PCI device, generally speaking
for PCI this is taken care of by the PCI subsytem itself; and if this requires
special handling then this special handling for PCI devices belongs in the
drivers/usb/dwc3/dwc3-pci.c code, not in the core code.

Part of the weirdness with the PCI driver instantiating a platform_device
and then using a platform_driver is that that code path (and only that
code path) sets the dwc->sysdev_is_parent flag, so we can simply avoid
the unwanted poking of the dma-mask by only doing so when that flag is
not set.

This way we only poke the dma-mask if we are actually dealing with a real
platform-device; and not when dealing with the PCI-driver instantiated
faux platform-device, fixing the regression.

> I also think the xhci driver will call dma_set_mask_and_coherent again
> later on when dwc3 is used in host mode.

The Intel Cherry Trail SoC has a separate full/normal XHCI controller for
host mode and the DWC3 controller is only used in gadget mode.

Regards,

Hans

Re: [PATCH] usb: dwc3: leave default DMA for PCI devices

[Fabio Aiuto]

[resend with all CC]

Hello Sven,

On Sun, Nov 14, 2021 at 12:56:02PM +0100, Sven Peter wrote:
> On Sat, Nov 13, 2021, at 15:29, Fabio Aiuto wrote:
> > in case of a PCI dwc3 controller, leave the default DMA
> > mask. Calling of a 64 bit DMA mask breaks the driver on
> > cherrytrail based tablets like Cyberbook T116.
> >
> > Fixes: 45d39448b4d0 ("usb: dwc3: support 64 bit DMA in platform driver")
> > Reported-by: Hans De Goede <hdegoede@redhat.com>
> > Tested-by: Fabio Aiuto <fabioaiuto83@gmail.com>
> > Signed-off-by: Fabio Aiuto <fabioaiuto83@gmail.com>
> > ---
> >  drivers/usb/dwc3/core.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 643239d7d370..f4c09951b517 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1594,9 +1594,11 @@ static int dwc3_probe(struct platform_device *pdev)
> > 
> >  	dwc3_get_properties(dwc);
> > 
> > -	ret = dma_set_mask_and_coherent(dwc->sysdev, DMA_BIT_MASK(64));
> > -	if (ret)
> > -		return ret;
> > +	if (!dwc->sysdev_is_parent) {
> 
> 
> Are you sure it's the dwc3 controller that limits DMA to 32 bit addresses and
> not the PCI bus itself?
> I also think the xhci driver will call dma_set_mask_and_coherent again
> later on when dwc3 is used in host mode.

I could not have answered better than Hans, I'm just adding the bisect
log:

git bisect start
# bad: [7d2a07b769330c34b4deabeed939325c77a7ec2f] Linux 5.14
git bisect bad 7d2a07b769330c34b4deabeed939325c77a7ec2f
# good: [62fb9874f5da54fdb243003b386128037319b219] Linux 5.13
git bisect good 7cf3dead1ad70c72edb03e2d98e1f3dcd332cdb2
# good: [406254918b232db198ed60f5bf1f8b84d96bca00] Merge tag 'perf-tools-for-v5.14-2021-07-01' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux
git bisect good 406254918b232db198ed60f5bf1f8b84d96bca00
# bad: [4ea90317956718e0648e1f87e56530db809a5a04] Merge tag 'for-linus-5.14-rc1-tag' of git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip
git bisect bad 4ea90317956718e0648e1f87e56530db809a5a04
# good: [8fc4fb1728855a22f9149079ba51877f5ee61fc9] Merge tag 'm68knommu-for-v5.14' of git://git.kernel.org/pub/scm/linux/kernel/git/gerg/m68knommu
git bisect good 8fc4fb1728855a22f9149079ba51877f5ee61fc9
# good: [77ad1f0e99bd00af024e650b862cfda3137af660] staging: hi6421-spmi-pmic: cleanup some macros
git bisect good 77ad1f0e99bd00af024e650b862cfda3137af660
# good: [eed0218e8cae9fcd186c30e9fcf5fe46a87e056e] Merge tag 'char-misc-5.14-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc
git bisect good eed0218e8cae9fcd186c30e9fcf5fe46a87e056e
# good: [c932ed0adb09a7fa6d6649ee04dd78c83ab07ada] Merge tag 'tty-5.14-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty
git bisect good c932ed0adb09a7fa6d6649ee04dd78c83ab07ada
# bad: [738d5ad104bbbe5d1bfb6c0553bb4a1eb91cc433] Revert "of/platform: Add stubs for of_platform_device_create/destroy()"
git bisect bad 738d5ad104bbbe5d1bfb6c0553bb4a1eb91cc433
# good: [8268acfe1cc967dbe9fbb05b5f07a19675a81cff] usb: isp1760: isp1760-udc: Provide missing description for 'udc' param
git bisect good 8268acfe1cc967dbe9fbb05b5f07a19675a81cff
# bad: [318324e6df9787f8ec06660224f555471c8f36d1] usb: musb: Implement tracing for state change events
git bisect bad 318324e6df9787f8ec06660224f555471c8f36d1
# good: [ca5ce82529104e96ccc5e1888979258e233e1644] usb: typec: intel_pmc_mux: Update IOM port status offset for AlderLake
git bisect good ca5ce82529104e96ccc5e1888979258e233e1644
# bad: [ecfbd7b9054bddb12cea07fda41bb3a79a7b0149] usb: gadget: f_fs: Fix setting of device and driver data cross-references
git bisect bad ecfbd7b9054bddb12cea07fda41bb3a79a7b0149
# good: [87191ca9f90244d4e003fbe5c77390b5e585a5ef] USB: UDC: Implement udc_async_callbacks in net2272
git bisect good 87191ca9f90244d4e003fbe5c77390b5e585a5ef
# good: [307462a6f5c5a563ec084bb315f4e0279dfb2026] usb: gadget: function: printer: use list_move instead of list_del/list_add
git bisect good 307462a6f5c5a563ec084bb315f4e0279dfb2026
# bad: [45d39448b4d0260743f25d88fd929451ec8296f2] usb: dwc3: support 64 bit DMA in platform driver
git bisect bad 45d39448b4d0260743f25d88fd929451ec8296f2
# good: [60dfe484cef45293e631b3a6e8995f1689818172] USB: core: Avoid WARNings for 0-length descriptor requests
git bisect good 60dfe484cef45293e631b3a6e8995f1689818172
# first bad commit: [45d39448b4d0260743f25d88fd929451ec8296f2] usb: dwc3: support 64 bit DMA in platform driver

> 
> 
> 
> Sven

thank you,

fabio

Re: [PATCH] usb: dwc3: leave default DMA for PCI devices

[Hans de Goede]

Hi,

On 11/13/21 15:29, Fabio Aiuto wrote:
> in case of a PCI dwc3 controller, leave the default DMA
> mask. Calling of a 64 bit DMA mask breaks the driver on
> cherrytrail based tablets like Cyberbook T116.
> 
> Fixes: 45d39448b4d0 ("usb: dwc3: support 64 bit DMA in platform driver")
> Reported-by: Hans De Goede <hdegoede@redhat.com>
> Tested-by: Fabio Aiuto <fabioaiuto83@gmail.com>
> Signed-off-by: Fabio Aiuto <fabioaiuto83@gmail.com>

I can confirm that this fixes things for me to:

Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> ---
>  drivers/usb/dwc3/core.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 643239d7d370..f4c09951b517 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1594,9 +1594,11 @@ static int dwc3_probe(struct platform_device *pdev)
>  
>  	dwc3_get_properties(dwc);
>  
> -	ret = dma_set_mask_and_coherent(dwc->sysdev, DMA_BIT_MASK(64));
> -	if (ret)
> -		return ret;
> +	if (!dwc->sysdev_is_parent) {
> +		ret = dma_set_mask_and_coherent(dwc->sysdev, DMA_BIT_MASK(64));
> +		if (ret)
> +			return ret;
> +	}
>  
>  	dwc->reset = devm_reset_control_array_get_optional_shared(dev);
>  	if (IS_ERR(dwc->reset))
>