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

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

[Moritz Fischer]

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: stable@vger.kernel.org
Cc: Mathias Nyman <mathias.nyman@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Vinod Koul <vkoul@kernel.org>
Fixes: 2478be82de44 ("usb: renesas-xhci: Add ROM loader for uPD720201")
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 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

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

[Greg KH]

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: stable@vger.kernel.org
> Cc: Mathias Nyman <mathias.nyman@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Vinod Koul <vkoul@kernel.org>
> Fixes: 2478be82de44 ("usb: renesas-xhci: Add ROM loader for uPD720201")
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
>  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

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

[Vinod Koul]

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 <vkoul@kernel.org>
Reviewed-by: Vinod Koul <vkoul@kernel.org>

-- 
~Vinod

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

[Greg KH]

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

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

[Vinod Koul]

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