Skip to contentSkip to conversation

usb: renesas-xhci: Fix handling of unknown ROM state

PATCH Superseded Moritz Fischer 3 participants 5 messages Jun 15, 2021

Subsystems (3)
Stable branch USB USB xHCI driver

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Moritz Fischer

author
#0 lore raw

PATCH 1/1 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(-)
drivers/usb/host/xhci-pci-renesas.c +7 -8
index f97ac9f52bf4..dfe54f0afc4b 100644
@@ -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
Signature · 1 line
2.31.1
maintainer
#1 lore raw
Quoted 3 lines · 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.

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

Quoted 29 lines
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(-)
drivers/usb/host/xhci-pci-renesas.c +2 -1
index f97ac9f52bf4..dfe54f0afc4b 100644
@@ -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;

drivers/usb/host/xhci-pci-renesas.c static int renesas_fw_check_running(struct pci_dev *pdev) · line 230

Quoted 10 lines
drivers/usb/host/xhci-pci-renesas.c +3 -7
-	/* 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:

drivers/usb/host/xhci-pci-renesas.c static int renesas_fw_check_running(struct pci_dev *pdev) · line 235

Quoted 5 lines
drivers/usb/host/xhci-pci-renesas.c +2 -0
+	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

Tested-by Reviewed-by
#2 lore raw
Quoted 6 lines · Greg KH
Quoted 3 lines · 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.

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]>
Signature · 1 line
~Vinod
maintainer
#3 lore raw
Quoted 11 lines · Vinod Koul
Quoted 6 lines · Greg KH
Quoted 3 lines · 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.

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

#4 lore raw
Quoted 7 lines · Greg KH
Quoted 6 lines · Vinod Koul
Quoted 5 lines · Greg KH
Quoted 3 lines · 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.

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

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

Signature · 1 line
~Vinod