Message-ID: <07395839-dc14-3ff1-c9a2-81d53f68a042@redhat.com>
Date:   Sun, 14 Nov 2021 14:42:10 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.0
Subject: Re: [PATCH] usb: dwc3: leave default DMA for PCI devices
Content-Language: en-US
To:     Sven Peter <sven@svenpeter.dev>,
        Fabio Aiuto <fabioaiuto83@gmail.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Felipe Balbi <balbi@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
        "hch@lst.de" <hch@lst.de>, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20211113142959.27191-1-fabioaiuto83@gmail.com>
 <34347fd8-1d67-4d16-b09f-061bc0c0cb5a@www.fastmail.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <34347fd8-1d67-4d16-b09f-061bc0c0cb5a@www.fastmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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