RE: [PATCH 0/2] Modularization of DFL private feature drivers

["Wu, Hao" <hao.wu@intel.com>]

> Subject: Re: [PATCH 0/2] Modularization of DFL private feature drivers
> 
> Generally i think this is a good approach.
> 
> However I do have concern.
> 
> The feature_id in dfl is magic number, similar to pci id but without a vendor
> id.
> 
> Is it possible to add something like a vendor id so different vendors would
> not have to be so careful to use a unique id ?

Hi Tom,

Thanks for the comments.

Here is only one field defined in spec, that is feature id to distinguish one
feature with another one. There is no vendor id here I think, and several
cards are using this for now and seems not possible to change DFH format
for these products. : (

I fully understand the concern is the feature id management, and potential
conflicts there from different vendors. One possible method to resolve this
is managing the ids in a public place (web? Or just the driver header file for
feature id definition), all vendors can request some feature ids, and other
people can see which feature ids have been used so that they can avoid
using conflict ones with other people.

And in the later version DFH, this feature id will be replaced with GUID
I think, so it can resolve the conflict problems from different vendors?
But now, we still need to handle the existing ones. : )

Thanks
Hao

> 
> This touches some of the matching function of the bus ops.  Could there be a
> way for bus ops to be used so that there could be multiple matching
> function.  Maybe the one in 0002 as a default so users could override it ?
> 
> The use case I am worrying about is an OEM.  The OEM uses an unclaimed
> feature_id and supplies their own platform device device driver to match the
> feature_id.  But some later version of the kernel claims this feature_id and
> the OEM's driver no longer works and since the value comes from pci config
> space it is difficult to change.
> 
> So looking for something like
> 
> register_feature_matcher((*bus_match)(struct device *dev, struct
> device_driver *drv))
> 
> static int dfl_bus_match_default(struct device *dev, struct device_driver *drv)
> {
>     struct dfl_device *dfl_dev = to_dfl_dev(dev);
>     struct dfl_driver *dfl_drv = to_dfl_drv(drv);
>     const struct dfl_device_id *id_entry = dfl_drv->id_table;
> 
>     while (id_entry->feature_id) {
>         if (dfl_match_one_device(id_entry, dfl_dev)) {
>             dfl_dev->id_entry = id_entry;
>             return 1;
>         }
>         id_entry++;
>     }
> 
>     return 0;
> }
> 
> register_feature_matcher(&dfl_bus_match_default)
> 
> static int dfl_bus_match(struct device *dev, struct device_driver *drv)
> {
> 
>     struct dfl_device *dfl_dev = to_dfl_dev(dev);
>     struct dfl_driver *dfl_drv = to_dfl_drv(drv);
>     const struct dfl_device_id *id_entry = dfl_drv->id_table;
> 
>     while (id_entry->feature_id) {
> 
>         matcher = Loop over matchers()
> 
>         if (matcher(dev, drv)) {
>             dfl_dev->id_entry = id_entry;
>             return 1;
>         }
>         id_entry++;
>     }
> 
>     return 0;
> }
> 
> Or maybe use some of the unused bits in the dfl header to add a second
> vendor-like id and change existing matcher to look feature_id and
> vendor_like_id.
> 
> Tom
> 
> 
> 
> On 7/14/20 10:38 PM, Xu Yilun wrote:
> > This patchset makes it possible to develop independent driver modules
> > for DFL private features. It also helps to leverage existing kernel
> > drivers to enable some IP blocks in DFL.
> >
> > Xu Yilun (2):
> >   fpga: dfl: map feature mmio resources in their own feature drivers
> >   fpga: dfl: create a dfl bus type to support DFL devices
> >
> >  Documentation/ABI/testing/sysfs-bus-dfl |  15 ++
> >  drivers/fpga/dfl-pci.c                  |  21 +-
> >  drivers/fpga/dfl.c                      | 435 +++++++++++++++++++++++++++-----
> >  drivers/fpga/dfl.h                      |  91 ++++++-
> >  4 files changed, 492 insertions(+), 70 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
> >