[PATCH] usb: renesas-xhci: Prefer firmware loading on unknown ROM state

[PATCH] usb: renesas-xhci: Prefer firmware loading on unknown ROM state

[Takashi Iwai]

The recent attempt to handle an unknown ROM state in the commit
d143825baf15 ("usb: renesas-xhci: Fix handling of unknown ROM state")
resulted in a regression and reverted later by the commit 44cf53602f5a
("Revert "usb: renesas-xhci: Fix handling of unknown ROM state"").
The problem of the former fix was that it treated the failure of
firmware loading as a fatal error.  Since the firmware files aren't
included in the standard linux-firmware tree, most users don't have
them, hence they got the non-working system after that.  The revert
fixed the regression, but also it didn't make the firmware loading
triggered even on the devices that do need it.  So we need still a fix
for them.

This is another attempt to handle the unknown ROM state.  Like the
previous fix, this also tries to load the firmware when ROM shows
unknown state.  In this patch, however, the failure of a firmware
loading (such as a missing firmware file) isn't handled as a fatal
error any longer when ROM has been already detected, but it falls back
to the ROM mode like before.  The error is returned only when no ROM
is detected and the firmware loading failed.

Along with it, for simplifying the code flow, the detection and the
check of ROM is factored out from renesas_fw_check_running() and done
in the caller side, renesas_xhci_check_request_fw().  It avoids the
redundant ROM checks.

The patch was tested on Lenovo Thinkpad T14 gen (BIOS 1.34).  Also it
was confirmed that no regression is seen on another Thinkpad T14
machine that has worked without the patch, too.

Fixes: 44cf53602f5a ("Revert "usb: renesas-xhci: Fix handling of unknown ROM state"")
BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1189207
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/usb/host/xhci-pci-renesas.c | 35 +++++++++++++++++++----------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/host/xhci-pci-renesas.c b/drivers/usb/host/xhci-pci-renesas.c
index aa88e57649a9..52599d96634f 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");
+			return -ENOENT;
 
 		case RENESAS_ROM_STATUS_ERROR: /* Error State */
 		default: /* All other states are marked as "Reserved states" */
@@ -224,14 +225,6 @@ 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;
-	}
-
 	/*
 	 * Test if the device is actually needing the firmware. As most
 	 * BIOSes will initialize the device for us. If the device is
@@ -591,21 +584,39 @@ int renesas_xhci_check_request_fw(struct pci_dev *pdev,
 			(struct xhci_driver_data *)id->driver_data;
 	const char *fw_name = driver_data->firmware;
 	const struct firmware *fw;
+	bool has_rom;
 	int err;
 
+	/* Check if device has ROM and loaded, if so skip everything */
+	has_rom = renesas_check_rom(pdev);
+	if (has_rom) {
+		err = renesas_check_rom_state(pdev);
+		if (!err)
+			return 0;
+		else if (err != -ENOENT)
+			has_rom = false;
+	}
+
 	err = renesas_fw_check_running(pdev);
 	/* Continue ahead, if the firmware is already running. */
 	if (!err)
 		return 0;
 
+	/* no firmware interface available */
 	if (err != 1)
-		return err;
+		return has_rom ? 0 : err;
 
 	pci_dev_get(pdev);
-	err = request_firmware(&fw, fw_name, &pdev->dev);
+	err = firmware_request_nowarn(&fw, fw_name, &pdev->dev);
 	pci_dev_put(pdev);
 	if (err) {
-		dev_err(&pdev->dev, "request_firmware failed: %d\n", err);
+		if (has_rom) {
+			dev_info(&pdev->dev, "failed to load firmware %s, fallback to ROM\n",
+				 fw_name);
+			return 0;
+		}
+		dev_err(&pdev->dev, "failed to load firmware %s: %d\n",
+			fw_name, err);
 		return err;
 	}
 
-- 
2.26.2

Re: [PATCH] usb: renesas-xhci: Prefer firmware loading on unknown ROM state

[Takashi Iwai]

On Thu, 19 Aug 2021 13:34:27 +0200,
Takashi Iwai wrote:
> 
> The recent attempt to handle an unknown ROM state in the commit
> d143825baf15 ("usb: renesas-xhci: Fix handling of unknown ROM state")
> resulted in a regression and reverted later by the commit 44cf53602f5a
> ("Revert "usb: renesas-xhci: Fix handling of unknown ROM state"").
> The problem of the former fix was that it treated the failure of
> firmware loading as a fatal error.  Since the firmware files aren't
> included in the standard linux-firmware tree, most users don't have
> them, hence they got the non-working system after that.  The revert
> fixed the regression, but also it didn't make the firmware loading
> triggered even on the devices that do need it.  So we need still a fix
> for them.
> 
> This is another attempt to handle the unknown ROM state.  Like the
> previous fix, this also tries to load the firmware when ROM shows
> unknown state.  In this patch, however, the failure of a firmware
> loading (such as a missing firmware file) isn't handled as a fatal
> error any longer when ROM has been already detected, but it falls back
> to the ROM mode like before.  The error is returned only when no ROM
> is detected and the firmware loading failed.
> 
> Along with it, for simplifying the code flow, the detection and the
> check of ROM is factored out from renesas_fw_check_running() and done
> in the caller side, renesas_xhci_check_request_fw().  It avoids the
> redundant ROM checks.
> 
> The patch was tested on Lenovo Thinkpad T14 gen (BIOS 1.34).  Also it
> was confirmed that no regression is seen on another Thinkpad T14
> machine that has worked without the patch, too.
> 
> Fixes: 44cf53602f5a ("Revert "usb: renesas-xhci: Fix handling of unknown ROM state"")
> BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1189207
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

A gentle ping to confirm whether this gets a review or not.


thanks,

Takashi


> ---
>  drivers/usb/host/xhci-pci-renesas.c | 35 +++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-pci-renesas.c b/drivers/usb/host/xhci-pci-renesas.c
> index aa88e57649a9..52599d96634f 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");
> +			return -ENOENT;
>  
>  		case RENESAS_ROM_STATUS_ERROR: /* Error State */
>  		default: /* All other states are marked as "Reserved states" */
> @@ -224,14 +225,6 @@ 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;
> -	}
> -
>  	/*
>  	 * Test if the device is actually needing the firmware. As most
>  	 * BIOSes will initialize the device for us. If the device is
> @@ -591,21 +584,39 @@ int renesas_xhci_check_request_fw(struct pci_dev *pdev,
>  			(struct xhci_driver_data *)id->driver_data;
>  	const char *fw_name = driver_data->firmware;
>  	const struct firmware *fw;
> +	bool has_rom;
>  	int err;
>  
> +	/* Check if device has ROM and loaded, if so skip everything */
> +	has_rom = renesas_check_rom(pdev);
> +	if (has_rom) {
> +		err = renesas_check_rom_state(pdev);
> +		if (!err)
> +			return 0;
> +		else if (err != -ENOENT)
> +			has_rom = false;
> +	}
> +
>  	err = renesas_fw_check_running(pdev);
>  	/* Continue ahead, if the firmware is already running. */
>  	if (!err)
>  		return 0;
>  
> +	/* no firmware interface available */
>  	if (err != 1)
> -		return err;
> +		return has_rom ? 0 : err;
>  
>  	pci_dev_get(pdev);
> -	err = request_firmware(&fw, fw_name, &pdev->dev);
> +	err = firmware_request_nowarn(&fw, fw_name, &pdev->dev);
>  	pci_dev_put(pdev);
>  	if (err) {
> -		dev_err(&pdev->dev, "request_firmware failed: %d\n", err);
> +		if (has_rom) {
> +			dev_info(&pdev->dev, "failed to load firmware %s, fallback to ROM\n",
> +				 fw_name);
> +			return 0;
> +		}
> +		dev_err(&pdev->dev, "failed to load firmware %s: %d\n",
> +			fw_name, err);
>  		return err;
>  	}
>  
> -- 
> 2.26.2
> 

Re: [PATCH] usb: renesas-xhci: Prefer firmware loading on unknown ROM state

[Greg Kroah-Hartman]

On Thu, Aug 26, 2021 at 12:28:21PM +0200, Takashi Iwai wrote:
> On Thu, 19 Aug 2021 13:34:27 +0200,
> Takashi Iwai wrote:
> > 
> > The recent attempt to handle an unknown ROM state in the commit
> > d143825baf15 ("usb: renesas-xhci: Fix handling of unknown ROM state")
> > resulted in a regression and reverted later by the commit 44cf53602f5a
> > ("Revert "usb: renesas-xhci: Fix handling of unknown ROM state"").
> > The problem of the former fix was that it treated the failure of
> > firmware loading as a fatal error.  Since the firmware files aren't
> > included in the standard linux-firmware tree, most users don't have
> > them, hence they got the non-working system after that.  The revert
> > fixed the regression, but also it didn't make the firmware loading
> > triggered even on the devices that do need it.  So we need still a fix
> > for them.
> > 
> > This is another attempt to handle the unknown ROM state.  Like the
> > previous fix, this also tries to load the firmware when ROM shows
> > unknown state.  In this patch, however, the failure of a firmware
> > loading (such as a missing firmware file) isn't handled as a fatal
> > error any longer when ROM has been already detected, but it falls back
> > to the ROM mode like before.  The error is returned only when no ROM
> > is detected and the firmware loading failed.
> > 
> > Along with it, for simplifying the code flow, the detection and the
> > check of ROM is factored out from renesas_fw_check_running() and done
> > in the caller side, renesas_xhci_check_request_fw().  It avoids the
> > redundant ROM checks.
> > 
> > The patch was tested on Lenovo Thinkpad T14 gen (BIOS 1.34).  Also it
> > was confirmed that no regression is seen on another Thinkpad T14
> > machine that has worked without the patch, too.
> > 
> > Fixes: 44cf53602f5a ("Revert "usb: renesas-xhci: Fix handling of unknown ROM state"")
> > BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1189207
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> A gentle ping to confirm whether this gets a review or not.

Given no one else objected, I'll go take it now, thanks.

greg k-h

Re: [PATCH] usb: renesas-xhci: Prefer firmware loading on unknown ROM state

[Greg Kroah-Hartman]

On Thu, Aug 19, 2021 at 01:34:27PM +0200, Takashi Iwai wrote:
> The recent attempt to handle an unknown ROM state in the commit
> d143825baf15 ("usb: renesas-xhci: Fix handling of unknown ROM state")
> resulted in a regression and reverted later by the commit 44cf53602f5a
> ("Revert "usb: renesas-xhci: Fix handling of unknown ROM state"").
> The problem of the former fix was that it treated the failure of
> firmware loading as a fatal error.  Since the firmware files aren't
> included in the standard linux-firmware tree, most users don't have
> them, hence they got the non-working system after that.  The revert
> fixed the regression, but also it didn't make the firmware loading
> triggered even on the devices that do need it.  So we need still a fix
> for them.
> 
> This is another attempt to handle the unknown ROM state.  Like the
> previous fix, this also tries to load the firmware when ROM shows
> unknown state.  In this patch, however, the failure of a firmware
> loading (such as a missing firmware file) isn't handled as a fatal
> error any longer when ROM has been already detected, but it falls back
> to the ROM mode like before.  The error is returned only when no ROM
> is detected and the firmware loading failed.
> 
> Along with it, for simplifying the code flow, the detection and the
> check of ROM is factored out from renesas_fw_check_running() and done
> in the caller side, renesas_xhci_check_request_fw().  It avoids the
> redundant ROM checks.
> 
> The patch was tested on Lenovo Thinkpad T14 gen (BIOS 1.34).  Also it
> was confirmed that no regression is seen on another Thinkpad T14
> machine that has worked without the patch, too.
> 
> Fixes: 44cf53602f5a ("Revert "usb: renesas-xhci: Fix handling of unknown ROM state"")
> BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1189207
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/usb/host/xhci-pci-renesas.c | 35 +++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 12 deletions(-)

This does not apply to my usb-linus branch, are you sure it is still
needed in Linus's tree right now?

thanks,

greg k-h

Re: [PATCH] usb: renesas-xhci: Prefer firmware loading on unknown ROM state

[Takashi Iwai]

On Thu, 26 Aug 2021 13:50:13 +0200,
Greg Kroah-Hartman wrote:
> 
> On Thu, Aug 19, 2021 at 01:34:27PM +0200, Takashi Iwai wrote:
> > The recent attempt to handle an unknown ROM state in the commit
> > d143825baf15 ("usb: renesas-xhci: Fix handling of unknown ROM state")
> > resulted in a regression and reverted later by the commit 44cf53602f5a
> > ("Revert "usb: renesas-xhci: Fix handling of unknown ROM state"").
> > The problem of the former fix was that it treated the failure of
> > firmware loading as a fatal error.  Since the firmware files aren't
> > included in the standard linux-firmware tree, most users don't have
> > them, hence they got the non-working system after that.  The revert
> > fixed the regression, but also it didn't make the firmware loading
> > triggered even on the devices that do need it.  So we need still a fix
> > for them.
> > 
> > This is another attempt to handle the unknown ROM state.  Like the
> > previous fix, this also tries to load the firmware when ROM shows
> > unknown state.  In this patch, however, the failure of a firmware
> > loading (such as a missing firmware file) isn't handled as a fatal
> > error any longer when ROM has been already detected, but it falls back
> > to the ROM mode like before.  The error is returned only when no ROM
> > is detected and the firmware loading failed.
> > 
> > Along with it, for simplifying the code flow, the detection and the
> > check of ROM is factored out from renesas_fw_check_running() and done
> > in the caller side, renesas_xhci_check_request_fw().  It avoids the
> > redundant ROM checks.
> > 
> > The patch was tested on Lenovo Thinkpad T14 gen (BIOS 1.34).  Also it
> > was confirmed that no regression is seen on another Thinkpad T14
> > machine that has worked without the patch, too.
> > 
> > Fixes: 44cf53602f5a ("Revert "usb: renesas-xhci: Fix handling of unknown ROM state"")
> > BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1189207
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/usb/host/xhci-pci-renesas.c | 35 +++++++++++++++++++----------
> >  1 file changed, 23 insertions(+), 12 deletions(-)
> 
> This does not apply to my usb-linus branch, are you sure it is still
> needed in Linus's tree right now?

I guess we can postpone for 5.15.  The patch was written for the code
on linux-next, and I see there have been a few code clean up there.

But the patch itself could be applied to Linus tree with a slight
fuzz, so the stable backport should be fine.

If it's still not cleanly applicable, let me know.  I'll refresh the
patch for whatever preferred branch.


thanks,

Takashi

Re: [PATCH] usb: renesas-xhci: Prefer firmware loading on unknown ROM state

[Greg Kroah-Hartman]

On Thu, Aug 26, 2021 at 01:55:54PM +0200, Takashi Iwai wrote:
> On Thu, 26 Aug 2021 13:50:13 +0200,
> Greg Kroah-Hartman wrote:
> > 
> > On Thu, Aug 19, 2021 at 01:34:27PM +0200, Takashi Iwai wrote:
> > > The recent attempt to handle an unknown ROM state in the commit
> > > d143825baf15 ("usb: renesas-xhci: Fix handling of unknown ROM state")
> > > resulted in a regression and reverted later by the commit 44cf53602f5a
> > > ("Revert "usb: renesas-xhci: Fix handling of unknown ROM state"").
> > > The problem of the former fix was that it treated the failure of
> > > firmware loading as a fatal error.  Since the firmware files aren't
> > > included in the standard linux-firmware tree, most users don't have
> > > them, hence they got the non-working system after that.  The revert
> > > fixed the regression, but also it didn't make the firmware loading
> > > triggered even on the devices that do need it.  So we need still a fix
> > > for them.
> > > 
> > > This is another attempt to handle the unknown ROM state.  Like the
> > > previous fix, this also tries to load the firmware when ROM shows
> > > unknown state.  In this patch, however, the failure of a firmware
> > > loading (such as a missing firmware file) isn't handled as a fatal
> > > error any longer when ROM has been already detected, but it falls back
> > > to the ROM mode like before.  The error is returned only when no ROM
> > > is detected and the firmware loading failed.
> > > 
> > > Along with it, for simplifying the code flow, the detection and the
> > > check of ROM is factored out from renesas_fw_check_running() and done
> > > in the caller side, renesas_xhci_check_request_fw().  It avoids the
> > > redundant ROM checks.
> > > 
> > > The patch was tested on Lenovo Thinkpad T14 gen (BIOS 1.34).  Also it
> > > was confirmed that no regression is seen on another Thinkpad T14
> > > machine that has worked without the patch, too.
> > > 
> > > Fixes: 44cf53602f5a ("Revert "usb: renesas-xhci: Fix handling of unknown ROM state"")
> > > BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1189207
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > >  drivers/usb/host/xhci-pci-renesas.c | 35 +++++++++++++++++++----------
> > >  1 file changed, 23 insertions(+), 12 deletions(-)
> > 
> > This does not apply to my usb-linus branch, are you sure it is still
> > needed in Linus's tree right now?
> 
> I guess we can postpone for 5.15.  The patch was written for the code
> on linux-next, and I see there have been a few code clean up there.
> 
> But the patch itself could be applied to Linus tree with a slight
> fuzz, so the stable backport should be fine.
> 
> If it's still not cleanly applicable, let me know.  I'll refresh the
> patch for whatever preferred branch.

It was not cleanly applicable, 'git am' did not like it against my
usb-linus branch (which is 5.14-rc7 + a few other USB patches not in
this driver).

So if you want to rebase it against that, I will be glad to take it for
5.14-final to resolve this issue.

thanks,

greg k-h

Re: [PATCH] usb: renesas-xhci: Prefer firmware loading on unknown ROM state

[Takashi Iwai]

On Thu, 26 Aug 2021 14:21:46 +0200,
Greg Kroah-Hartman wrote:
> 
> On Thu, Aug 26, 2021 at 01:55:54PM +0200, Takashi Iwai wrote:
> > On Thu, 26 Aug 2021 13:50:13 +0200,
> > Greg Kroah-Hartman wrote:
> > > 
> > > On Thu, Aug 19, 2021 at 01:34:27PM +0200, Takashi Iwai wrote:
> > > > The recent attempt to handle an unknown ROM state in the commit
> > > > d143825baf15 ("usb: renesas-xhci: Fix handling of unknown ROM state")
> > > > resulted in a regression and reverted later by the commit 44cf53602f5a
> > > > ("Revert "usb: renesas-xhci: Fix handling of unknown ROM state"").
> > > > The problem of the former fix was that it treated the failure of
> > > > firmware loading as a fatal error.  Since the firmware files aren't
> > > > included in the standard linux-firmware tree, most users don't have
> > > > them, hence they got the non-working system after that.  The revert
> > > > fixed the regression, but also it didn't make the firmware loading
> > > > triggered even on the devices that do need it.  So we need still a fix
> > > > for them.
> > > > 
> > > > This is another attempt to handle the unknown ROM state.  Like the
> > > > previous fix, this also tries to load the firmware when ROM shows
> > > > unknown state.  In this patch, however, the failure of a firmware
> > > > loading (such as a missing firmware file) isn't handled as a fatal
> > > > error any longer when ROM has been already detected, but it falls back
> > > > to the ROM mode like before.  The error is returned only when no ROM
> > > > is detected and the firmware loading failed.
> > > > 
> > > > Along with it, for simplifying the code flow, the detection and the
> > > > check of ROM is factored out from renesas_fw_check_running() and done
> > > > in the caller side, renesas_xhci_check_request_fw().  It avoids the
> > > > redundant ROM checks.
> > > > 
> > > > The patch was tested on Lenovo Thinkpad T14 gen (BIOS 1.34).  Also it
> > > > was confirmed that no regression is seen on another Thinkpad T14
> > > > machine that has worked without the patch, too.
> > > > 
> > > > Fixes: 44cf53602f5a ("Revert "usb: renesas-xhci: Fix handling of unknown ROM state"")
> > > > BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1189207
> > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > ---
> > > >  drivers/usb/host/xhci-pci-renesas.c | 35 +++++++++++++++++++----------
> > > >  1 file changed, 23 insertions(+), 12 deletions(-)
> > > 
> > > This does not apply to my usb-linus branch, are you sure it is still
> > > needed in Linus's tree right now?
> > 
> > I guess we can postpone for 5.15.  The patch was written for the code
> > on linux-next, and I see there have been a few code clean up there.
> > 
> > But the patch itself could be applied to Linus tree with a slight
> > fuzz, so the stable backport should be fine.
> > 
> > If it's still not cleanly applicable, let me know.  I'll refresh the
> > patch for whatever preferred branch.
> 
> It was not cleanly applicable, 'git am' did not like it against my
> usb-linus branch (which is 5.14-rc7 + a few other USB patches not in
> this driver).
> 
> So if you want to rebase it against that, I will be glad to take it for
> 5.14-final to resolve this issue.

OK, then let me resubmit.  Wait for a minute...


thanks,

Takashi