Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Christian Lamparter <chunkeey@gmail.com>
To:     Vinod Koul <vkoul@kernel.org>
Cc:     Mathias Nyman <mathias.nyman@linux.intel.com>,
        Mathias Nyman <mathias.nyman@intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-arm-msm@vger.kernel.org,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
        Christian Lamparter <chunkeey@googlemail.com>,
        John Stultz <john.stultz@linaro.org>,
        Alan Stern <stern@rowland.harvard.edu>,
        Andreas =?ISO-8859-1?Q?B=F6hler?= <dev@aboehler.at>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v8 3/5] usb: xhci: Add support for Renesas controller with memory
Date:   Sat, 04 Apr 2020 12:08:32 +0200
Message-ID: <226575470.unBdNOV7p8@debian64>
In-Reply-To: <20200401161852.GD72691@vkoul-mobl>
References: <20200323170601.419809-1-vkoul@kernel.org> <9854066.43zovN3OMW@debian64> <20200401161852.GD72691@vkoul-mobl>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On Wednesday, 1 April 2020 18:18:52 CEST Vinod Koul wrote:
> On 01-04-20, 17:39, Christian Lamparter wrote:
> > On Wednesday, 1 April 2020 14:57:48 CEST Vinod Koul wrote:
> > > On 26-03-20, 17:21, Vinod Koul wrote:
> > > > On 26-03-20, 13:29, Mathias Nyman wrote:
> > > > > On 23.3.2020 19.05, Vinod Koul wrote:
> > > > > > Some rensas controller like uPD720201 and uPD720202 need firmware to be
> > > > > > loaded. Add these devices in table and invoke renesas firmware loader
> > > > > > functions to check and load the firmware into device memory when
> > > > > > required.
> > > > > > 
> > > > > > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > > > > > ---
> > > > > >  drivers/usb/host/xhci-pci-renesas.c |  1 +
> > > > > >  drivers/usb/host/xhci-pci.c         | 29 ++++++++++++++++++++++++++++-
> > > > > >  drivers/usb/host/xhci-pci.h         |  3 +++
> > > > > >  3 files changed, 32 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > 
> > > > > It's unfortunate if firmware loading couldn't be initiated in a PCI fixup hook
> > > > > for this Renesas controller. What was the reason it failed?
> > > > > 
> > > > > Nicolas Saenz Julienne just submitted a solution like that for Raspberry Pi 4
> > > > > where firmware loading is initiated in pci-quirks.c quirk_usb_early_handoff()
> > > > > 
> > > > > https://lore.kernel.org/lkml/20200324182812.20420-1-nsaenzjulienne@suse.de
> > > > > 
> > > > > Is he doing something different than what was done for the Renesas controller?
> > > > 
> > > > I tried and everytime ended up not getting firmware. Though I did not
> > > > investigate a lot. Christian seemed to have tested sometime back as
> > > > well.
> > > > 
> > > > Another problem is that we dont get driver_data in the quirk and there
> > > > didnt seem a way to find the firmware name.
> > > > 
> > > > > > diff --git a/drivers/usb/host/xhci-pci-renesas.c b/drivers/usb/host/xhci-pci-renesas.c
> > > > > > index c588277ac9b8..d413d53df94b 100644
> > > > > > --- a/drivers/usb/host/xhci-pci-renesas.c
> > > > > > +++ b/drivers/usb/host/xhci-pci-renesas.c
> > > > > > @@ -336,6 +336,7 @@ static void renesas_fw_callback(const struct firmware *fw,
> > > > > >  		goto cleanup;
> > > > > >  	}
> > > > > >  
> > > > > > +	xhci_pci_probe(pdev, ctx->id);
> > > > > >  	return;
> > > > > 
> > > > > I haven't looked into this but instead of calling xhci_pci_probe() here in the async fw
> > > > > loading callback could we just return -EPROBE_DEFER until firmware is loaded when
> > > > > xhci_pci_probe() is originally called?
> > > > 
> > > > Hmm, initially my thinking was how to tell device core to probe again,
> > > > and then digging up I saw wait_for_device_probe() which can be used, let
> > > > me try that
> > > 
> > > Sorry to report back that it doesn't work as planned :(
> > > 
> > > I modified the code to invoke the request_firmware_nowait() which will load
> > > the firmware and provide the firmware in callback. Meanwhile return -EPROBE_DEFER.
> > > 
> > > After a bit, the core invokes the driver probe again and we hit the
> > > roadblock. The request_firmware uses devres and allocates resources for
> > > loading the firmware. The problem is that device core checks for this:
> > > 
> > > bus: 'pci': really_probe: probing driver xhci_hcd_pci with device 0000:01:00.0
> > > pci 0000:01:00.0: Resources present before probing
> > > 
> > > And here the probe fails. In some cases the firmware_callback finishes
> > > before this and we can probe again, but that is not very reliable.
> > > 
> > > I tested another way to use request_firmware() (sync version) and then
> > > load the firmware in probe and load. The request is done only for
> > > renesas devices if they dont have firmware already running.
> > > So rest of the devices wont have any impact.
> > > 
> > > Now should we continue this way in the patchset or move to sync version.
> > > Am okay either way.
> > 
> > Just a word of caution.
> > 
> > The problem with the usage of "sync" request_firmware in drivers is that if the
> > code is built into the kernel the request_firmware() could be called before the
> > (root) filesystem on which the firmware resides is ready.... So this will get
> > weird during boot because what is the sync request_firmware() going to do? From what
> > I know, this is why the funny _async firmware request APIs are even a thing...
> 
> So is your usage a module or inbuilt. I am using it as a module, so
> seems okay. For inbuilt, someone needs to do make it in kernel or make
> sure the initramfs has this :)

Yes, after testing on the device, I can state that everything went as intended :-).

Cheers,
Christian