Re: [PATCH v6 0/5] usb: xhci: Add support for Renesas USB controllers

[Vinod Koul]

Hi Mathias,

On 30-01-20, 19:07, Mathias Nyman wrote:
> On 25.1.2020 7.32, Vinod Koul wrote:
> > > > > > > 
> > > > > > > On Mon, Jan 13, 2020 at 12:42 AM Vinod Koul <vkoul@xxxxxxxxxx> wrote:
> > > > > > > > 
> > > > > > > > This series add support for Renesas USB controllers uPD720201 and uPD720202.
> > > > > > > > These require firmware to be loaded and in case devices have ROM those can
> > > > > > > > also be programmed if empty. If ROM is programmed, it runs from ROM as well.
> > > > > > > > 
> > > > > > > > This includes two patches from Christian which supported these controllers
> > > > > > > > w/o ROM and later my patches for ROM support and multiple firmware versions,
> > > > > > > > debugfs hook for rom erase and export of xhci-pci functions.
> > > > > > > > 
> ...
> > 
> > Mathias, any comments on this series..?
> > 
> 
> Hi Vinod
> 
> Sorry about the delay.
> 
> Maybe a firmware loading driver like this that wraps the xhci pci driver could
> work.
> 
> One benefit is that we could skip searching for the right firmware name based
> on PCI ID. Each of these Renesas controllers now have their own pci_device_id
> entry in the pci_ids[] table, and could have the supported firmware name(s)
> in .driver_data. This way we wouldn't need to add the renesas_fw_table[] or
> maybe even the renesas_needs_fw_dl() function in this series.
> 
> I realize this can't be easily changed because usb_hcd_pci_probe() takes the
> pci_device_id pointer as an argument, and expects id.driver_data to be a
> HC driver pointer.
> 
> So this turns out to be a question for Greg and Alan:
> 
> Would it make sense to change usb_hcd_pci_probe() to take a HC driver pointer
> as an argument instead of a pointer to pci_device_id?
> pci_device_id pointer is only used to extract the HC driver handle.
> This way the driver_data could be used for, well, driver data.
> 
> Heikki actually suggested this some time ago to me, back then the idea was to
> improve xhci quirks code by using driver_data for quirk flags instead of
> finding and setting them later.
> 
> There are a few other opens regarding this series. Mostly because I'm not (yet)
> familiar with all the details, so I'll just just list them here.
> 
> - Is it really enough to add the Renesas driver to Makefile before xhci-pci
>   driver to make sure it gets matched and probed based on vendor/device id
>   before xhci-pci driver is matched and probed based on pci class?
>   What if the Renesas driver is a module and xhci-pci compiled in?

The driver loading rules are based on level and makefile order. So
renesas will always be loaded before xhci driver. This is required since
xhci claims all devices, so we need to make sure it loads before.

I think we should make it inbuilt driver rather than a module. xhci
driver is only inbuilt.

If there is a better way to handle this, am all for it.

> - Previously probe didn't return before hcd's were added and everything set up.
>   Now with request_firmware_nowait() probe returns early successfully, and the
>   old xhci_pci_probe() which sets up everything is called later by the request
>   firmware callback. So there could be whole new set of races possible.
>   For example pci remove can be called mid firmware loading, or when the old
>   xhci_pci_probe is still setting up things.

I think this is a valid concern. Let me think about how to solve this..
maybe add a flag in remove which ensure remove doesnt run untill the
probe/firmware callback is completed.

>   I understood that a synchronous request_firmware() in probe has its own
>   issues, not sure if there is a good solution for this.

Yeah with userspace not available, that can be tricky!

> - Before the firmware is written to the controller the firmware version is
>   compared against a hardcoded number in the drivers renesas_fw_table[].
>   This means new firmware versions can't be supported without patching the driver.
>   Is this intentional?

Yes, we have only bunch of validated versions. There maybe more in the
wild but we dont know. The vendor is not very helpful here :(

Across all folks using this, there seems to be only few versions and
vendor is not supporting it anymore, so chances of new versions seems
remote


Thanks
-- 
~Vinod