| Message ID | 20210615022514.245274-1-mdf@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: renesas-xhci: Fix handling of unknown ROM state | expand |
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
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>
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
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
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
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(-)