Re: [PATCH 1/2] fpga: dfl: add driver_override support

[Xu Yilun <yilun.xu@intel.com>]

Hi Greg,

We created a 'driver_override' api for dfl bus, and a driver for
userspace I/O access for dfl devices.

Some new comments inline.

On Wed, Oct 21, 2020 at 03:25:32PM +0800, Xu Yilun wrote:
> Hi Greg,
> 
> On Tue, Oct 20, 2020 at 11:21:55AM +0200, Greg KH wrote:
> > On Tue, Oct 20, 2020 at 04:57:23PM +0800, Xu Yilun wrote:
> > > On Tue, Oct 20, 2020 at 09:32:59AM +0200, Greg KH wrote:
> > > > On Tue, Oct 20, 2020 at 03:11:58PM +0800, Xu Yilun wrote:
> > > > > I think it is normal case that a driver is successfully registered but
> > > > > doesn't match any device because it provides no id_table.
> > > > 
> > > > How is that "normal"?  What would ever cause that driver to be bound to
> > > > a device then?
> > > 
> > > This patchset adds the driver_override sysfs node, to let userspace
> > > specify which driver (by name matching) to be bound to a device. The
> > > driver "dfl-uio-pdev" doesn't provide an id_table, it could only be
> > > bound to a device whose driver_override is set to "dfl-uio-pdev".
> > > 
> > > Sorry, this is actually not "normal". Usually dfl drivers with valid id_table
> > > should be used to drive the dfl devices. But we also want to give an option
> > > for userspace to take full control of the device, some IP blocks in FPGA are
> > > written for specific purposes by FPGA user so a userspace driver may
> > > serve them better.
> > > 
> > > > 
> > > > And you better not say userspace is responsible for it...
> > > 
> > > Actually it is the userspace's decision which device they want to
> > > access directly ...
> > > 
> > > I'm not sure if this idea is OK. I see similar implementation for
> > > pci/platform devices.
> > 
> > That is there for debugging and for being able to support things when
> > the kernel is not updated with a new device id yet.
> 
> We are trying to use the driver_override for similar purposes on FPGA.
> The FPGA card could be reprogramed with a newly developed Hardware
> component by users. Sometimes only the FPGA users know which dfl device
> they want to operate on. This is why we give the option to users for
> direct I/O access.
> 
> Also FPGA users have the requirement for diagnostic of the newly
> developed components.
> 
> It is possible we define an id_table for dfl_uio_pdev driver, but then
> users have to modify the driver code everytime they create & reprogram
> a new HW component.
> 
> > 
> > The virtio people took this to a new extreme and use it to bind real
> > devices through to virtual machines, but really, that's horrid.  And
> > they have problems with it as is seen in random patches at times.

I searched the previous patches, it seems the main issue is the user
could unbind the vfio-pci for a device and bind it to another kernel
driver, while its iommu group is still being used by vfio for other
devices in the same group, then the integrity of the group is
compromised. The conclusion is the user should be responsible for the
correct operation of bind/unbind/driver_override/new_id ...

In this patchset, we use the driver_override for UIO case, I think the
issue doesn't impact it, is it?

> > 
> > Don't make this the only way to support this, that's not an ok api.

We are not going to match driver for every dfl device by user assignment.
We are developing generic kernel drivers for them, e.g. dfl-n3000-nios,
dfl-emif, as you can see in recent patches. They follows the generic
id_table matching.

We use the driver_override for UIO case. Similar to what you've mentioned,
FPGA users develop their own hardware components on FPGA, so they need to
run diagnostic tools for white-box test.
They also need to support some hardware designed for special purposes
and does not fit into one of the standard kernel subsystems.

The FPGA users are the developer of some hardware component, they have the
knowledge of the new dfl devices. So the driver_override give the option
to the users for driver assignment.

For these purposes, is it OK we use the driver_override?


Thanks,
Yilun


> > 
> > greg k-h