Re: [PATCH v6 1/2] fpga: dfl: add the userspace I/O device support for DFL devices

[Xu Yilun]

On Thu, Jan 21, 2021 at 12:03:47PM -0800, Moritz Fischer wrote:
> Hi Tom,
> 
> On Thu, Jan 21, 2021 at 06:30:20AM -0800, Tom Rix wrote:
> > 
> > On 1/17/21 8:22 AM, Moritz Fischer wrote:
> > > Greg,
> > >
> > > On Sun, Jan 17, 2021 at 04:45:04PM +0100, Greg KH wrote:
> > >> On Wed, Jan 13, 2021 at 09:54:07AM +0800, Xu Yilun wrote:
> > >>> This patch supports the DFL drivers be written in userspace. This is
> > >>> realized by exposing the userspace I/O device interfaces.
> > >>>
> > >>> The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
> > >>> platform device with the DFL device's resources, and let the generic UIO
> > >>> platform device driver provide support to userspace access to kernel
> > >>> interrupts and memory locations.
> > >> Why doesn't the existing uio driver work for this, why do you need a new
> > >> one?
> > >>
> > >>> ---
> > >>>  drivers/fpga/Kconfig        | 10 +++++
> > >>>  drivers/fpga/Makefile       |  1 +
> > >>>  drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++
> > >> uio drivers traditionally go in drivers/uio/ and start with "uio", so
> > >> shouldn't this be drivers/uio/uio_dfl_pdev.c to match the same naming
> > >> scheme?
> > > I had considered suggesting that, but ultimately this driver only
> > > creates a 'uio_pdrv_genirq' platform device, so it didn't seem like a
> > > good fit.
> > >> But again, you need to explain in detail, why the existing uio driver
> > >> doesn't work properly, or why you can't just add a few lines to an
> > >> existing one.
> > > Ultimately there are three options I see:
> > > 1) Do what Xu does, which is re-use the 'uio_pdrv_genirq' uio driver by
> > >   creating a platform device for it as sub-device of the dfl device that
> > >   we bind to uio_pdrv_genirq
> > > 2) Add a module_dfl_driver part to drivers/uio/uio_pdrv_genirq.c and
> > >   corresponding id table
> > > 3) Create a new uio_dfl_genirq kind of driver that uses the dfl bus and
> > >   that would make sense to then put into drivers/uio. (This would
> > >   duplicate code in uio_pdrv_genirq to some extend)
> > >
> > > Overall I think in terms of code re-use I think Xu's choice might be
> > > less new code as it simply wraps the uio platform device driver, and
> > > allows for defining the resources passed to the UIO driver to be defined
> > > by hardware through a DFL.
> > >
> > > I've seen the pattern that Xu proposed used in other places like the
> > > macb network driver where you'd have macb_main (the platform driver) and
> > > macb_pci that wraps it for a pci usage.
> > >
> > > - Moritz
> > 
> > Thinking of this problem more generally.
> > 
> > Every fpga will have a handful of sub devices.
> > 
> > Do we want to carry them in the fpga subsystem or carry them in the other subsystems ?
> 
> Yeah no we really don't. I think that was the point of the whole DFL
> bus :)
> > 
> > Consider the short term reviewing and long term maintenance of the sub devices by the subsystem maintainers.
> > 
> > It easier for them if the sub devices are in the other subsystems.
> 
> Agreed.
> > 
> > 
> > Applying this to specifically for dfl_uio.
> > 
> > No one from the uio subsystem reviewing this change is a problem.
> 
> Greg will.
> > I think this change needs to go to the uio subsystem.
> 
> Yeah I've thought about this some for the last few days, maybe it's
> easier that way.
> 
> Tbh, there's so little code here even if we went with option 3 above
> it's probably fairly short. It would set a better prcedent.
> 
> Xu, how do you feel about giving that a spin? See if option 3 will be
> way more code.

Yes, I'll try to put it to drivers/uio.

I see the implementation in vfio_platform.c/vfio_amba.c/vfio_platform_common.c.
I'm wondering if we could handle it in that way.

Thanks,
yilun