linux-kernel - Re: [PATCH 0/2] Modularization of DFL private feature drivers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Jul 2020 15:50:51 -0700
From:   Tom Rix <trix@...hat.com>
To:     Xu Yilun <yilun.xu@...el.com>, mdf@...nel.org,
        linux-fpga@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     lgoncalv@...hat.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 ?

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
>