2021-06-15 02:28:01

by Moritz Fischer

[permalink] [raw]
Subject: [PATCH] usb: renesas-xhci: Fix handling of unknown ROM state

If the ROM status returned is unknown (RENESAS_ROM_STATUS_NO_RESULT)
we need to attempt loading the firmware rather than just skipping
it all together.

Cc: [email protected]
Cc: Mathias Nyman <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Vinod Koul <[email protected]>
Fixes: 2478be82de44 ("usb: renesas-xhci: Add ROM loader for uPD720201")
Signed-off-by: Moritz Fischer <[email protected]>
---
drivers/usb/host/xhci-pci-renesas.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xhci-pci-renesas.c b/drivers/usb/host/xhci-pci-renesas.c
index f97ac9f52bf4..dfe54f0afc4b 100644
--- a/drivers/usb/host/xhci-pci-renesas.c
+++ b/drivers/usb/host/xhci-pci-renesas.c
@@ -207,7 +207,8 @@ static int renesas_check_rom_state(struct pci_dev *pdev)
return 0;

case RENESAS_ROM_STATUS_NO_RESULT: /* No result yet */
- return 0;
+ dev_dbg(&pdev->dev, "Unknown ROM status ...\n");
+ break;

case RENESAS_ROM_STATUS_ERROR: /* Error State */
default: /* All other states are marked as "Reserved states" */
@@ -224,13 +225,11 @@ static int renesas_fw_check_running(struct pci_dev *pdev)
u8 fw_state;
int err;

- /* Check if device has ROM and loaded, if so skip everything */
- err = renesas_check_rom(pdev);
- if (err) { /* we have rom */
- err = renesas_check_rom_state(pdev);
- if (!err)
- return err;
- }
+ /* Only if device has ROM and loaded FW we can skip loading and
+ * return success. Otherwise (even unknown state), attempt to load FW.
+ */
+ if (renesas_check_rom(pdev) && !renesas_check_rom_state(pdev))
+ return 0;

/*
* Test if the device is actually needing the firmware. As most
--
2.31.1


2021-06-15 07:16:51

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] usb: renesas-xhci: Fix handling of unknown ROM state

On Mon, Jun 14, 2021 at 07:25:14PM -0700, Moritz Fischer wrote:
> If the ROM status returned is unknown (RENESAS_ROM_STATUS_NO_RESULT)
> we need to attempt loading the firmware rather than just skipping
> it all together.

How can this happen? Can you provide more information here?

>
> Cc: [email protected]
> Cc: Mathias Nyman <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Vinod Koul <[email protected]>
> Fixes: 2478be82de44 ("usb: renesas-xhci: Add ROM loader for uPD720201")
> Signed-off-by: Moritz Fischer <[email protected]>
> ---
> drivers/usb/host/xhci-pci-renesas.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-pci-renesas.c b/drivers/usb/host/xhci-pci-renesas.c
> index f97ac9f52bf4..dfe54f0afc4b 100644
> --- a/drivers/usb/host/xhci-pci-renesas.c
> +++ b/drivers/usb/host/xhci-pci-renesas.c
> @@ -207,7 +207,8 @@ static int renesas_check_rom_state(struct pci_dev *pdev)
> return 0;
>
> case RENESAS_ROM_STATUS_NO_RESULT: /* No result yet */
> - return 0;
> + dev_dbg(&pdev->dev, "Unknown ROM status ...\n");
> + break;
>
> case RENESAS_ROM_STATUS_ERROR: /* Error State */
> default: /* All other states are marked as "Reserved states" */
> @@ -224,13 +225,11 @@ static int renesas_fw_check_running(struct pci_dev *pdev)
> u8 fw_state;
> int err;
>
> - /* Check if device has ROM and loaded, if so skip everything */
> - err = renesas_check_rom(pdev);
> - if (err) { /* we have rom */
> - err = renesas_check_rom_state(pdev);
> - if (!err)
> - return err;
> - }
> + /* Only if device has ROM and loaded FW we can skip loading and
> + * return success. Otherwise (even unknown state), attempt to load FW.
> + */

Nit, but please use the correct comment style format, like is used a few
lines below:

> + if (renesas_check_rom(pdev) && !renesas_check_rom_state(pdev))
> + return 0;
>
> /*
> * Test if the device is actually needing the firmware. As most

This isn't the networking tree :)

thanks,

greg k-h

2021-06-15 07:53:45

by Vinod Koul

[permalink] [raw]
Subject: Re: [PATCH] usb: renesas-xhci: Fix handling of unknown ROM state

On 15-06-21, 09:15, Greg KH wrote:
> On Mon, Jun 14, 2021 at 07:25:14PM -0700, Moritz Fischer wrote:
> > If the ROM status returned is unknown (RENESAS_ROM_STATUS_NO_RESULT)
> > we need to attempt loading the firmware rather than just skipping
> > it all together.
>
> How can this happen? Can you provide more information here?

Sometimes ROM load seems to return unknown status, this helps in those
cases by doing attempting RAM load. The status should be success of
fail, here it is neither :(

I have tested this on 96 boards RB3 on a loaded ROM device as well as
ROM erased one. Works good and is improvement in the cases where ROM
status is 'unknown'


Tested-by: Vinod Koul <[email protected]>
Reviewed-by: Vinod Koul <[email protected]>

--
~Vinod

2021-06-15 09:48:17

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] usb: renesas-xhci: Fix handling of unknown ROM state

On Tue, Jun 15, 2021 at 01:22:55PM +0530, Vinod Koul wrote:
> On 15-06-21, 09:15, Greg KH wrote:
> > On Mon, Jun 14, 2021 at 07:25:14PM -0700, Moritz Fischer wrote:
> > > If the ROM status returned is unknown (RENESAS_ROM_STATUS_NO_RESULT)
> > > we need to attempt loading the firmware rather than just skipping
> > > it all together.
> >
> > How can this happen? Can you provide more information here?
>
> Sometimes ROM load seems to return unknown status, this helps in those
> cases by doing attempting RAM load. The status should be success of
> fail, here it is neither :(

Then this needs to be added to the changelog text please.

thanks,

greg k-h

2021-06-15 11:08:16

by Vinod Koul

[permalink] [raw]
Subject: Re: [PATCH] usb: renesas-xhci: Fix handling of unknown ROM state

On 15-06-21, 11:44, Greg KH wrote:
> On Tue, Jun 15, 2021 at 01:22:55PM +0530, Vinod Koul wrote:
> > On 15-06-21, 09:15, Greg KH wrote:
> > > On Mon, Jun 14, 2021 at 07:25:14PM -0700, Moritz Fischer wrote:
> > > > If the ROM status returned is unknown (RENESAS_ROM_STATUS_NO_RESULT)
> > > > we need to attempt loading the firmware rather than just skipping
> > > > it all together.
> > >
> > > How can this happen? Can you provide more information here?
> >
> > Sometimes ROM load seems to return unknown status, this helps in those
> > cases by doing attempting RAM load. The status should be success of
> > fail, here it is neither :(
>
> Then this needs to be added to the changelog text please.

/me nods

--
~Vinod