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@xxxxxxx>

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
>