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

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

[Moritz Fischer]

This reverts commit d143825baf15f204dac60acdf95e428182aa3374.

Justin reports some of his systems now fail as result of this commit:

 xhci_hcd 0000:04:00.0: Direct firmware load for renesas_usb_fw.mem failed with error -2
 xhci_hcd 0000:04:00.0: request_firmware failed: -2
 xhci_hcd: probe of 0000:04:00.0 failed with error -2

The revert brings back the original issue the commit tried to solve but
at least unbreaks existing systems relying on previous behavior.

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>
Cc: Justin Forbes <jmforbes@linuxtx.org>
Reported-by: Justin Forbes <jmforbes@linuxtx.org>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---

Justin,

would you be able to help out testing follow up patches to this?

I don't have a machine to test your use-case and mine definitly requires
a firmware load on RENESAS_ROM_STATUS_NO_RESULT.

Thanks
- Moritz

---
 drivers/usb/host/xhci-pci-renesas.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xhci-pci-renesas.c b/drivers/usb/host/xhci-pci-renesas.c
index 1da647961c25..5923844ed821 100644
--- a/drivers/usb/host/xhci-pci-renesas.c
+++ b/drivers/usb/host/xhci-pci-renesas.c
@@ -207,8 +207,7 @@ static int renesas_check_rom_state(struct pci_dev *pdev)
 			return 0;
 
 		case RENESAS_ROM_STATUS_NO_RESULT: /* No result yet */
-			dev_dbg(&pdev->dev, "Unknown ROM status ...\n");
-			break;
+			return 0;
 
 		case RENESAS_ROM_STATUS_ERROR: /* Error State */
 		default: /* All other states are marked as "Reserved states" */
@@ -225,12 +224,13 @@ static int renesas_fw_check_running(struct pci_dev *pdev)
 	u8 fw_state;
 	int 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;
+	/* 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;
+	}
 
 	/*
 	 * Test if the device is actually needing the firmware. As most
-- 
2.32.0

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

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 3394 bytes --]

On 7/19/21 9:05 AM, Moritz Fischer wrote:
> This reverts commit d143825baf15f204dac60acdf95e428182aa3374.
> 
> Justin reports some of his systems now fail as result of this commit:
> 
>   xhci_hcd 0000:04:00.0: Direct firmware load for renesas_usb_fw.mem failed with error -2
>   xhci_hcd 0000:04:00.0: request_firmware failed: -2
>   xhci_hcd: probe of 0000:04:00.0 failed with error -2
> 
> The revert brings back the original issue the commit tried to solve but
> at least unbreaks existing systems relying on previous behavior.
> 
> 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>
> Cc: Justin Forbes <jmforbes@linuxtx.org>
> Reported-by: Justin Forbes <jmforbes@linuxtx.org>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
> 
> Justin,
> 
> would you be able to help out testing follow up patches to this?
> 
> I don't have a machine to test your use-case and mine definitly requires
> a firmware load on RENESAS_ROM_STATUS_NO_RESULT.
> 
> Thanks
> - Moritz


Hi Moritz,

as an additional data point, here's the behaviour of my system, a Thinkpad 
T14 AMD with:

06:00.0 USB controller [0c03]: Renesas Technology Corp. uPD720202 USB 3.0 
Host Controller [1912:0015] (rev 02)

- On Kernel 5.13.1, no firmware: USB controller resets in an endless loop 
when the system is running from battery
- On Kernel 5.13.4, no firmware: USB controller probe fails with the 
mentioned firmware load error
- On Kernel 5.13.4, with renesas_usb_fw.mem: everything is working fine, 
the reset issue is gone

So it seems to me that requiring a firmware is generally the correct driver 
behaviour for this hardware. The firmware I found in the Arch User 
Repository [1] unfortunately has a very restrictive license...

Kind regards,
Matthias


[1] https://github.com/denisandroid/uPD72020x-Firmware



> 
> ---
>   drivers/usb/host/xhci-pci-renesas.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-pci-renesas.c b/drivers/usb/host/xhci-pci-renesas.c
> index 1da647961c25..5923844ed821 100644
> --- a/drivers/usb/host/xhci-pci-renesas.c
> +++ b/drivers/usb/host/xhci-pci-renesas.c
> @@ -207,8 +207,7 @@ static int renesas_check_rom_state(struct pci_dev *pdev)
>   			return 0;
>   
>   		case RENESAS_ROM_STATUS_NO_RESULT: /* No result yet */
> -			dev_dbg(&pdev->dev, "Unknown ROM status ...\n");
> -			break;
> +			return 0;
>   
>   		case RENESAS_ROM_STATUS_ERROR: /* Error State */
>   		default: /* All other states are marked as "Reserved states" */
> @@ -225,12 +224,13 @@ static int renesas_fw_check_running(struct pci_dev *pdev)
>   	u8 fw_state;
>   	int 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;
> +	/* 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;
> +	}
>   
>   	/*
>   	 * Test if the device is actually needing the firmware. As most
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Moritz Fischer]

On Wed, Jul 21, 2021 at 05:28:21PM +0200, Matthias Schiffer wrote:
> On 7/19/21 9:05 AM, Moritz Fischer wrote:
> > This reverts commit d143825baf15f204dac60acdf95e428182aa3374.
> > 
> > Justin reports some of his systems now fail as result of this commit:
> > 
> >   xhci_hcd 0000:04:00.0: Direct firmware load for renesas_usb_fw.mem failed with error -2
> >   xhci_hcd 0000:04:00.0: request_firmware failed: -2
> >   xhci_hcd: probe of 0000:04:00.0 failed with error -2
> > 
> > The revert brings back the original issue the commit tried to solve but
> > at least unbreaks existing systems relying on previous behavior.
> > 
> > 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>
> > Cc: Justin Forbes <jmforbes@linuxtx.org>
> > Reported-by: Justin Forbes <jmforbes@linuxtx.org>
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > ---
> > 
> > Justin,
> > 
> > would you be able to help out testing follow up patches to this?
> > 
> > I don't have a machine to test your use-case and mine definitly requires
> > a firmware load on RENESAS_ROM_STATUS_NO_RESULT.
> > 
> > Thanks
> > - Moritz
> 
> 
> Hi Moritz,
> 
> as an additional data point, here's the behaviour of my system, a Thinkpad
> T14 AMD with:

Thanks!
> 
> 06:00.0 USB controller [0c03]: Renesas Technology Corp. uPD720202 USB 3.0
> Host Controller [1912:0015] (rev 02)
> 
> - On Kernel 5.13.1, no firmware: USB controller resets in an endless loop
> when the system is running from battery
> - On Kernel 5.13.4, no firmware: USB controller probe fails with the
> mentioned firmware load error
> - On Kernel 5.13.4, with renesas_usb_fw.mem: everything is working fine, the
> reset issue is gone
> 
> So it seems to me that requiring a firmware is generally the correct driver
> behaviour for this hardware. The firmware I found in the Arch User
> Repository [1] unfortunately has a very restrictive license...

Yeah, the chip definitely needs the firmware. It can either initialize
from external ROM or runtime loaded firmware.

I think the problem really lies in how the current (and reverted) code
detects the need for firmware loading.

The current code looks at two indicators:
- Is there an external ROM and if so, did somebody try to program the
  external ROM and succeed? (renesas_check_rom_state)
- Did somebody try to runtime-load firmware, and if so did they succeed?
  (renesas_fw_check_running, after the early return)

The first one (and resulting early return) does *not* tell you whether
the controller actually has firwmare. That's what breaks my systems.

The second one is only really useful *if* we also check that FW_DOWNLOAD
was locked.

Neither of the above captures the case where you actually have an
external ROM that is programmed with proper firmware and caused the chip
to be loaded with said firmware.

Now before the patch that was reverted, since nobody tried to program
the ROM, it feel through to the "do nothing" in this case -- which
worked since it configured itself from external ROM.

Now how do we properly determine we do or don't need firwmare?

Looking at the datasheet I see two options.
- The version register? I need to investigate what that resets to with
  an unprogrammed/corrupted ROM. If that reliably gives a detectable value
  this could be used as an indicator.
  
- The USBSTS register according to the datasheet will report an error
  through the HCE bit:
  "If both uDP720201 and uDP720202 detect no correct firmware in Serial
  ROM, this flag will be set"

I'll put up an RFC in the next couple of days ...

- Moritz

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

[Florian Klink]

On 21-07-21 09:56:27, Moritz Fischer wrote:
>On Wed, Jul 21, 2021 at 05:28:21PM +0200, Matthias Schiffer wrote:
>> On 7/19/21 9:05 AM, Moritz Fischer wrote:
>> > This reverts commit d143825baf15f204dac60acdf95e428182aa3374.
>> >
>> > Justin reports some of his systems now fail as result of this commit:
>> >
>> >   xhci_hcd 0000:04:00.0: Direct firmware load for renesas_usb_fw.mem failed with error -2
>> >   xhci_hcd 0000:04:00.0: request_firmware failed: -2
>> >   xhci_hcd: probe of 0000:04:00.0 failed with error -2
>> >
>> > The revert brings back the original issue the commit tried to solve but
>> > at least unbreaks existing systems relying on previous behavior.
>> >
>> > 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>
>> > Cc: Justin Forbes <jmforbes@linuxtx.org>
>> > Reported-by: Justin Forbes <jmforbes@linuxtx.org>
>> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
>> > ---
>> >
>> > Justin,
>> >
>> > would you be able to help out testing follow up patches to this?
>> >
>> > I don't have a machine to test your use-case and mine definitly requires
>> > a firmware load on RENESAS_ROM_STATUS_NO_RESULT.
>> >
>> > Thanks
>> > - Moritz
>>
>>
>> Hi Moritz,
>>
>> as an additional data point, here's the behaviour of my system, a Thinkpad
>> T14 AMD with:
>
>Thanks!

Other Thinkpad (X13 AMD) user here.

> 06:00.0 USB controller: Renesas Technology Corp. uPD720202 USB 3.0 Host Controller (rev 02)

When upgrading from 5.13 5.13.2, suddenly the internal webcam, connected
via USB (and possibly other peripherals) was gone.

It took me some digging until I came to this thread.

I see the same firmware load failures:

> xhci_hcd 0000:06:00.0: Direct firmware load for renesas_usb_fw.mem failed with error -2
> xhci_hcd 0000:06:00.0: request_firmware failed: -2
> xhci_hcd: probe of 0000:06:00.0 failed with error -2

I can confirm a revert of d143825baf15f204dac60acdf95e428182aa3374 fixes
it.

>>
>> 06:00.0 USB controller [0c03]: Renesas Technology Corp. uPD720202 USB 3.0
>> Host Controller [1912:0015] (rev 02)
>>
>> - On Kernel 5.13.1, no firmware: USB controller resets in an endless loop
>> when the system is running from battery
>> - On Kernel 5.13.4, no firmware: USB controller probe fails with the
>> mentioned firmware load error
>> - On Kernel 5.13.4, with renesas_usb_fw.mem: everything is working fine, the
>> reset issue is gone
>>
>> So it seems to me that requiring a firmware is generally the correct driver
>> behaviour for this hardware. The firmware I found in the Arch User
>> Repository [1] unfortunately has a very restrictive license...
>
>Yeah, the chip definitely needs the firmware. It can either initialize
>from external ROM or runtime loaded firmware.
>
>I think the problem really lies in how the current (and reverted) code
>detects the need for firmware loading.
>
>The current code looks at two indicators:
>- Is there an external ROM and if so, did somebody try to program the
>  external ROM and succeed? (renesas_check_rom_state)
>- Did somebody try to runtime-load firmware, and if so did they succeed?
>  (renesas_fw_check_running, after the early return)
>
>The first one (and resulting early return) does *not* tell you whether
>the controller actually has firwmare. That's what breaks my systems.
>
>The second one is only really useful *if* we also check that FW_DOWNLOAD
>was locked.
>
>Neither of the above captures the case where you actually have an
>external ROM that is programmed with proper firmware and caused the chip
>to be loaded with said firmware.
>
>Now before the patch that was reverted, since nobody tried to program
>the ROM, it feel through to the "do nothing" in this case -- which
>worked since it configured itself from external ROM.
>
>Now how do we properly determine we do or don't need firwmare?
>
>Looking at the datasheet I see two options.
>- The version register? I need to investigate what that resets to with
>  an unprogrammed/corrupted ROM. If that reliably gives a detectable value
>  this could be used as an indicator.
>
>- The USBSTS register according to the datasheet will report an error
>  through the HCE bit:
>  "If both uDP720201 and uDP720202 detect no correct firmware in Serial
>  ROM, this flag will be set"
>
>I'll put up an RFC in the next couple of days ...

Is the RFC already out somewhere?

Regardless of that, maybe we should push the trivial revert to
linux-stable first, so users don't run into this unexpectedly.

Regards,
Florian

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

[Greg Kroah-Hartman]

On Wed, Jul 28, 2021 at 02:37:55PM +0200, Florian Klink wrote:
> On 21-07-21 09:56:27, Moritz Fischer wrote:
> > On Wed, Jul 21, 2021 at 05:28:21PM +0200, Matthias Schiffer wrote:
> > > On 7/19/21 9:05 AM, Moritz Fischer wrote:
> > > > This reverts commit d143825baf15f204dac60acdf95e428182aa3374.
> > > >
> > > > Justin reports some of his systems now fail as result of this commit:
> > > >
> > > >   xhci_hcd 0000:04:00.0: Direct firmware load for renesas_usb_fw.mem failed with error -2
> > > >   xhci_hcd 0000:04:00.0: request_firmware failed: -2
> > > >   xhci_hcd: probe of 0000:04:00.0 failed with error -2
> > > >
> > > > The revert brings back the original issue the commit tried to solve but
> > > > at least unbreaks existing systems relying on previous behavior.
> > > >
> > > > 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>
> > > > Cc: Justin Forbes <jmforbes@linuxtx.org>
> > > > Reported-by: Justin Forbes <jmforbes@linuxtx.org>
> > > > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > > > ---
> > > >
> > > > Justin,
> > > >
> > > > would you be able to help out testing follow up patches to this?
> > > >
> > > > I don't have a machine to test your use-case and mine definitly requires
> > > > a firmware load on RENESAS_ROM_STATUS_NO_RESULT.
> > > >
> > > > Thanks
> > > > - Moritz
> > > 
> > > 
> > > Hi Moritz,
> > > 
> > > as an additional data point, here's the behaviour of my system, a Thinkpad
> > > T14 AMD with:
> > 
> > Thanks!
> 
> Other Thinkpad (X13 AMD) user here.
> 
> > 06:00.0 USB controller: Renesas Technology Corp. uPD720202 USB 3.0 Host Controller (rev 02)
> 
> When upgrading from 5.13 5.13.2, suddenly the internal webcam, connected
> via USB (and possibly other peripherals) was gone.
> 
> It took me some digging until I came to this thread.
> 
> I see the same firmware load failures:
> 
> > xhci_hcd 0000:06:00.0: Direct firmware load for renesas_usb_fw.mem failed with error -2
> > xhci_hcd 0000:06:00.0: request_firmware failed: -2
> > xhci_hcd: probe of 0000:06:00.0 failed with error -2
> 
> I can confirm a revert of d143825baf15f204dac60acdf95e428182aa3374 fixes
> it.
> 
> > > 
> > > 06:00.0 USB controller [0c03]: Renesas Technology Corp. uPD720202 USB 3.0
> > > Host Controller [1912:0015] (rev 02)
> > > 
> > > - On Kernel 5.13.1, no firmware: USB controller resets in an endless loop
> > > when the system is running from battery
> > > - On Kernel 5.13.4, no firmware: USB controller probe fails with the
> > > mentioned firmware load error
> > > - On Kernel 5.13.4, with renesas_usb_fw.mem: everything is working fine, the
> > > reset issue is gone
> > > 
> > > So it seems to me that requiring a firmware is generally the correct driver
> > > behaviour for this hardware. The firmware I found in the Arch User
> > > Repository [1] unfortunately has a very restrictive license...
> > 
> > Yeah, the chip definitely needs the firmware. It can either initialize
> > from external ROM or runtime loaded firmware.
> > 
> > I think the problem really lies in how the current (and reverted) code
> > detects the need for firmware loading.
> > 
> > The current code looks at two indicators:
> > - Is there an external ROM and if so, did somebody try to program the
> >  external ROM and succeed? (renesas_check_rom_state)
> > - Did somebody try to runtime-load firmware, and if so did they succeed?
> >  (renesas_fw_check_running, after the early return)
> > 
> > The first one (and resulting early return) does *not* tell you whether
> > the controller actually has firwmare. That's what breaks my systems.
> > 
> > The second one is only really useful *if* we also check that FW_DOWNLOAD
> > was locked.
> > 
> > Neither of the above captures the case where you actually have an
> > external ROM that is programmed with proper firmware and caused the chip
> > to be loaded with said firmware.
> > 
> > Now before the patch that was reverted, since nobody tried to program
> > the ROM, it feel through to the "do nothing" in this case -- which
> > worked since it configured itself from external ROM.
> > 
> > Now how do we properly determine we do or don't need firwmare?
> > 
> > Looking at the datasheet I see two options.
> > - The version register? I need to investigate what that resets to with
> >  an unprogrammed/corrupted ROM. If that reliably gives a detectable value
> >  this could be used as an indicator.
> > 
> > - The USBSTS register according to the datasheet will report an error
> >  through the HCE bit:
> >  "If both uDP720201 and uDP720202 detect no correct firmware in Serial
> >  ROM, this flag will be set"
> > 
> > I'll put up an RFC in the next couple of days ...
> 
> Is the RFC already out somewhere?
> 
> Regardless of that, maybe we should push the trivial revert to
> linux-stable first, so users don't run into this unexpectedly.

It's already merged in the stable trees, right?

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

[Florian Klink]

>> > I'll put up an RFC in the next couple of days ...
>>
>> Is the RFC already out somewhere?
>>
>> Regardless of that, maybe we should push the trivial revert to
>> linux-stable first, so users don't run into this unexpectedly.
>
>It's already merged in the stable trees, right?

It's in 5.13.6, which was pushed 27mins ago ;-)

Thanks!

Florian