[PATCH] usb: xhci: Fix compare to use physical addresses in xhci_bulk_tx()

[PATCH] usb: xhci: Fix compare to use physical addresses in xhci_bulk_tx()

[Stefan Roese]

Testing with v2021.01 on MIPS Octeon has shown, that the latest patch
for the "short packet event trb handling" did introduce a bug on
platforms with virtual address != physical address. This patch fixes
this issue by using the correct address types in the compare (both
physical in this case).

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Aaron Williams <awilliams@marvell.com>
Cc: Chandrakala Chavva <cchavva@marvell.com>
Cc: Ran Wang <ran.wang_1@nxp.com>
Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Cc: Marek Vasut <marex@denx.de>
Cc: Bin Meng <bmeng.cn@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d708fc928b..d6c47d579b 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -723,8 +723,8 @@ again:
 		return -ETIMEDOUT;
 	}
 
-	if ((uintptr_t)(le64_to_cpu(event->trans_event.buffer))
-			!= (uintptr_t)last_transfer_trb_addr) {
+	if ((uintptr_t)(le64_to_cpu(event->trans_event.buffer)) !=
+	    (uintptr_t)virt_to_phys(last_transfer_trb_addr)) {
 		available_length -=
 			(int)EVENT_TRB_LEN(le32_to_cpu(event->trans_event.transfer_len));
 		xhci_acknowledge_event(ctrl);
-- 
2.30.0

[PATCH] usb: xhci: Fix compare to use physical addresses in xhci_bulk_tx()

[Bin Meng]

Hi Stefan,

On Fri, Jan 15, 2021 at 3:53 PM Stefan Roese <sr@denx.de> wrote:
>
> Testing with v2021.01 on MIPS Octeon has shown, that the latest patch
> for the "short packet event trb handling" did introduce a bug on
> platforms with virtual address != physical address. This patch fixes
> this issue by using the correct address types in the compare (both
> physical in this case).
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Aaron Williams <awilliams@marvell.com>
> Cc: Chandrakala Chavva <cchavva@marvell.com>
> Cc: Ran Wang <ran.wang_1@nxp.com>
> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> ---
>  drivers/usb/host/xhci-ring.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index d708fc928b..d6c47d579b 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -723,8 +723,8 @@ again:
>                 return -ETIMEDOUT;
>         }
>
> -       if ((uintptr_t)(le64_to_cpu(event->trans_event.buffer))
> -                       != (uintptr_t)last_transfer_trb_addr) {
> +       if ((uintptr_t)(le64_to_cpu(event->trans_event.buffer)) !=
> +           (uintptr_t)virt_to_phys(last_transfer_trb_addr)) {

Is last_transfer_trb_addr virtual address? I thought queue_trb()
returns physical address?

>                 available_length -=
>                         (int)EVENT_TRB_LEN(le32_to_cpu(event->trans_event.transfer_len));
>                 xhci_acknowledge_event(ctrl);
> --

Regards,
Bin

[PATCH] usb: xhci: Fix compare to use physical addresses in xhci_bulk_tx()

[Stefan Roese]

Hi Bin,

On 15.01.21 09:16, Bin Meng wrote:
> Hi Stefan,
> 
> On Fri, Jan 15, 2021 at 3:53 PM Stefan Roese <sr@denx.de> wrote:
>>
>> Testing with v2021.01 on MIPS Octeon has shown, that the latest patch
>> for the "short packet event trb handling" did introduce a bug on
>> platforms with virtual address != physical address. This patch fixes
>> this issue by using the correct address types in the compare (both
>> physical in this case).
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Aaron Williams <awilliams@marvell.com>
>> Cc: Chandrakala Chavva <cchavva@marvell.com>
>> Cc: Ran Wang <ran.wang_1@nxp.com>
>> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>   drivers/usb/host/xhci-ring.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index d708fc928b..d6c47d579b 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -723,8 +723,8 @@ again:
>>                  return -ETIMEDOUT;
>>          }
>>
>> -       if ((uintptr_t)(le64_to_cpu(event->trans_event.buffer))
>> -                       != (uintptr_t)last_transfer_trb_addr) {
>> +       if ((uintptr_t)(le64_to_cpu(event->trans_event.buffer)) !=
>> +           (uintptr_t)virt_to_phys(last_transfer_trb_addr)) {
> 
> Is last_transfer_trb_addr virtual address? I thought queue_trb()
> returns physical address?

No, queue_trb() returns a virtual address accessible from the CPU.

Thanks,
Stefan

[PATCH] usb: xhci: Fix compare to use physical addresses in xhci_bulk_tx()

[Nicolas Saenz Julienne]

On Fri, 2021-01-15 at 08:52 +0100, Stefan Roese wrote:
> Testing with v2021.01 on MIPS Octeon has shown, that the latest patch
> for the "short packet event trb handling" did introduce a bug on
> platforms with virtual address != physical address. This patch fixes
> this issue by using the correct address types in the compare (both
> physical in this case).
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Aaron Williams <awilliams@marvell.com>
> Cc: Chandrakala Chavva <cchavva@marvell.com>
> Cc: Ran Wang <ran.wang_1@nxp.com>
> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> ---
> ?drivers/usb/host/xhci-ring.c | 4 ++--
> ?1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index d708fc928b..d6c47d579b 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -723,8 +723,8 @@ again:
> ?		return -ETIMEDOUT;
> ?	}
> ?
> 
> -	if ((uintptr_t)(le64_to_cpu(event->trans_event.buffer))
> -			!= (uintptr_t)last_transfer_trb_addr) {
> +	if ((uintptr_t)(le64_to_cpu(event->trans_event.buffer)) !=
> +	    (uintptr_t)virt_to_phys(last_transfer_trb_addr)) {
> ?		available_length -=
> ?			(int)EVENT_TRB_LEN(le32_to_cpu(event->trans_event.transfer_len));
> ?		xhci_acknowledge_event(ctrl);

FWIW I also addressed this in my Rpi400/CM4 enablement series:
https://lists.denx.de/pipermail/u-boot/2021-January/437150.html

That said,

Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Regards,
Nicolas


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210115/521c8fae/attachment.sig>

[PATCH] usb: xhci: Fix compare to use physical addresses in xhci_bulk_tx()

[Stefan Roese]

On 15.01.21 11:27, Nicolas Saenz Julienne wrote:
> On Fri, 2021-01-15 at 08:52 +0100, Stefan Roese wrote:
>> Testing with v2021.01 on MIPS Octeon has shown, that the latest patch
>> for the "short packet event trb handling" did introduce a bug on
>> platforms with virtual address != physical address. This patch fixes
>> this issue by using the correct address types in the compare (both
>> physical in this case).
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Aaron Williams <awilliams@marvell.com>
>> Cc: Chandrakala Chavva <cchavva@marvell.com>
>> Cc: Ran Wang <ran.wang_1@nxp.com>
>> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>  ?drivers/usb/host/xhci-ring.c | 4 ++--
>>  ?1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index d708fc928b..d6c47d579b 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -723,8 +723,8 @@ again:
>>  ?		return -ETIMEDOUT;
>>  ?	}
>>   
>>
>> -	if ((uintptr_t)(le64_to_cpu(event->trans_event.buffer))
>> -			!= (uintptr_t)last_transfer_trb_addr) {
>> +	if ((uintptr_t)(le64_to_cpu(event->trans_event.buffer)) !=
>> +	    (uintptr_t)virt_to_phys(last_transfer_trb_addr)) {
>>  ?		available_length -=
>>  ?			(int)EVENT_TRB_LEN(le32_to_cpu(event->trans_event.transfer_len));
>>  ?		xhci_acknowledge_event(ctrl);
> 
> FWIW I also addressed this in my Rpi400/CM4 enablement series:
> https://lists.denx.de/pipermail/u-boot/2021-January/437150.html

Yes, thanks. I've seen your patch and also looked into it while
debugging this issue on my MIPS platform this morning.

Actually I would prefer my fix, as it uses virt_to_phys() and not the
other way around. Currently only virt_to_phys() is used in the USB code
and not phys_to_virt(), which does not seem to work correctly on my
platform BTW (as I discovered this morning).

> That said,
> 
> Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Thanks,
Stefan

[PATCH] usb: xhci: Fix compare to use physical addresses in xhci_bulk_tx()

[Nicolas Saenz Julienne]

+Matthias, just so you're aware.

On Fri, 2021-01-15 at 11:33 +0100, Stefan Roese wrote:
> On 15.01.21 11:27, Nicolas Saenz Julienne wrote:
> > On Fri, 2021-01-15 at 08:52 +0100, Stefan Roese wrote:
> > > Testing with v2021.01 on MIPS Octeon has shown, that the latest patch
> > > for the "short packet event trb handling" did introduce a bug on
> > > platforms with virtual address != physical address. This patch fixes
> > > this issue by using the correct address types in the compare (both
> > > physical in this case).
> > > 
> > > Signed-off-by: Stefan Roese <sr@denx.de>
> > > Cc: Aaron Williams <awilliams@marvell.com>
> > > Cc: Chandrakala Chavva <cchavva@marvell.com>
> > > Cc: Ran Wang <ran.wang_1@nxp.com>
> > > Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > > Cc: Marek Vasut <marex@denx.de>
> > > Cc: Bin Meng <bmeng.cn@gmail.com>
> > > ---
> > > ??drivers/usb/host/xhci-ring.c | 4 ++--
> > > ??1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > > index d708fc928b..d6c47d579b 100644
> > > --- a/drivers/usb/host/xhci-ring.c
> > > +++ b/drivers/usb/host/xhci-ring.c
> > > @@ -723,8 +723,8 @@ again:
> > > ??		return -ETIMEDOUT;
> > > ??	}
> > > ??
> > > 
> > > 
> > > -	if ((uintptr_t)(le64_to_cpu(event->trans_event.buffer))
> > > -			!= (uintptr_t)last_transfer_trb_addr) {
> > > +	if ((uintptr_t)(le64_to_cpu(event->trans_event.buffer)) !=
> > > +	    (uintptr_t)virt_to_phys(last_transfer_trb_addr)) {
> > > ??		available_length -=
> > > ??			(int)EVENT_TRB_LEN(le32_to_cpu(event->trans_event.transfer_len));
> > > ??		xhci_acknowledge_event(ctrl);
> > 
> > FWIW I also addressed this in my Rpi400/CM4 enablement series:
> > https://lists.denx.de/pipermail/u-boot/2021-January/437150.html
> 
> Yes, thanks. I've seen your patch and also looked into it while
> debugging this issue on my MIPS platform this morning.
> 
> Actually I would prefer my fix, as it uses virt_to_phys() and not the
> other way around. Currently only virt_to_phys() is used in the USB code
> and not phys_to_virt(), which does not seem to work correctly on my
> platform BTW (as I discovered this morning).

Fair enough, I'll rebase my series on top of this once it's picked :).

Regards,
Nicolas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210115/651a7a00/attachment.sig>

[PATCH] usb: xhci: Fix compare to use physical addresses in xhci_bulk_tx()

[Nicolas Saenz Julienne]

+Matthias, just so you're aware.

On Fri, 2021-01-15 at 11:33 +0100, Stefan Roese wrote:
> On 15.01.21 11:27, Nicolas Saenz Julienne wrote:
> > On Fri, 2021-01-15 at 08:52 +0100, Stefan Roese wrote:
> > > Testing with v2021.01 on MIPS Octeon has shown, that the latest patch
> > > for the "short packet event trb handling" did introduce a bug on
> > > platforms with virtual address != physical address. This patch fixes
> > > this issue by using the correct address types in the compare (both
> > > physical in this case).
> > > 
> > > Signed-off-by: Stefan Roese <sr@denx.de>
> > > Cc: Aaron Williams <awilliams@marvell.com>
> > > Cc: Chandrakala Chavva <cchavva@marvell.com>
> > > Cc: Ran Wang <ran.wang_1@nxp.com>
> > > Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > > Cc: Marek Vasut <marex@denx.de>
> > > Cc: Bin Meng <bmeng.cn@gmail.com>
> > > ---
> > > ??drivers/usb/host/xhci-ring.c | 4 ++--
> > > ??1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > > index d708fc928b..d6c47d579b 100644
> > > --- a/drivers/usb/host/xhci-ring.c
> > > +++ b/drivers/usb/host/xhci-ring.c
> > > @@ -723,8 +723,8 @@ again:
> > > ??		return -ETIMEDOUT;
> > > ??	}
> > > ??
> > > 
> > > 
> > > -	if ((uintptr_t)(le64_to_cpu(event->trans_event.buffer))
> > > -			!= (uintptr_t)last_transfer_trb_addr) {
> > > +	if ((uintptr_t)(le64_to_cpu(event->trans_event.buffer)) !=
> > > +	    (uintptr_t)virt_to_phys(last_transfer_trb_addr)) {
> > > ??		available_length -=
> > > ??			(int)EVENT_TRB_LEN(le32_to_cpu(event->trans_event.transfer_len));
> > > ??		xhci_acknowledge_event(ctrl);
> > 
> > FWIW I also addressed this in my Rpi400/CM4 enablement series:
> > https://lists.denx.de/pipermail/u-boot/2021-January/437150.html
> 
> Yes, thanks. I've seen your patch and also looked into it while
> debugging this issue on my MIPS platform this morning.
> 
> Actually I would prefer my fix, as it uses virt_to_phys() and not the
> other way around. Currently only virt_to_phys() is used in the USB code
> and not phys_to_virt(), which does not seem to work correctly on my
> platform BTW (as I discovered this morning).

Fair enough, I'll rebase my series on top of this once it's picked :).

Regards,
Nicolas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210115/43ec2153/attachment.sig>

[PATCH] usb: xhci: Fix compare to use physical addresses in xhci_bulk_tx()

[Stefan Roese]

Hi Marek,
Hi Bin,

On 15.01.21 08:52, Stefan Roese wrote:
> Testing with v2021.01 on MIPS Octeon has shown, that the latest patch
> for the "short packet event trb handling" did introduce a bug on
> platforms with virtual address != physical address. This patch fixes
> this issue by using the correct address types in the compare (both
> physical in this case).
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Aaron Williams <awilliams@marvell.com>
> Cc: Chandrakala Chavva <cchavva@marvell.com>
> Cc: Ran Wang <ran.wang_1@nxp.com>
> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Bin Meng <bmeng.cn@gmail.com>

If you see no issues with this patch, can you please pull this in
soon'ish?

Thanks,
Stefan

> ---
>   drivers/usb/host/xhci-ring.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index d708fc928b..d6c47d579b 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -723,8 +723,8 @@ again:
>   		return -ETIMEDOUT;
>   	}
>   
> -	if ((uintptr_t)(le64_to_cpu(event->trans_event.buffer))
> -			!= (uintptr_t)last_transfer_trb_addr) {
> +	if ((uintptr_t)(le64_to_cpu(event->trans_event.buffer)) !=
> +	    (uintptr_t)virt_to_phys(last_transfer_trb_addr)) {
>   		available_length -=
>   			(int)EVENT_TRB_LEN(le32_to_cpu(event->trans_event.transfer_len));
>   		xhci_acknowledge_event(ctrl);
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

[PATCH] usb: xhci: Fix compare to use physical addresses in xhci_bulk_tx()

[Marek Vasut]

On 2/8/21 1:39 PM, Stefan Roese wrote:
> Hi Marek,
> Hi Bin,
> 
> On 15.01.21 08:52, Stefan Roese wrote:
>> Testing with v2021.01 on MIPS Octeon has shown, that the latest patch
>> for the "short packet event trb handling" did introduce a bug on
>> platforms with virtual address != physical address. This patch fixes
>> this issue by using the correct address types in the compare (both
>> physical in this case).

Applied to usb/next (that's still in the pipeline for this release), thanks.