Re: [PATCH v5 06/28] fpga: add device feature list support

Wu Hao
2018-06-06T12:22:04Z

On Tue, Jun 05, 2018 at 03:21:31PM -0500, Alan Tull wrote:
> On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
>
> Hi Hao,
>
> I understand that you are implementing support for something that
> already has been defined and already exists. With that, I have some
> minor suggestions below. I have some questions below about how new
> features are added and suggestions for where some comments could be
> added to guide anyone who is adding feature devices or sub-features so
> they will do it cleanly in the way that you intend. Also some
> suggestions so that when a new feature is added, less places in code
> have to be touched.

Hi Alan,

I fully understand your point, thanks a lot for the review. Please see
my response below.

>
> > Device Feature List (DFL) defines a feature list structure that creates
> > a link list of feature headers within the MMIO space to provide an
> > extensible way of adding features. This patch introduces a kernel module
> > to provide basic infrastructure to support FPGA devices which implement
> > the Device Feature List.
> >
> > Usually there will be different features and their sub features linked into
> > the DFL. This code provides common APIs for feature enumeration, it creates
> > a container device (FPGA base region), walks through the DFLs and creates
> > platform devices for feature devices (Currently it only supports two
> > different feature devices, FPGA Management Engine (FME) and Port which
> > the Accelerator Function Unit (AFU) connected to). In order to enumerate
> > the DFLs, the common APIs required low level driver to provide necessary
> > enumeration information (e.g address for each device feature list for
> > given device) and fill it to the dfl_fpga_enum_info data structure. Please
> > refer to below description for APIs added for enumeration.
> >
> > Functions for enumeration information preparation:
> > *dfl_fpga_enum_info_alloc
> > allocate enumeration information data structure.
> >
> > *dfl_fpga_enum_info_add_dfl
> > add a device feature list to dfl_fpga_enum_info data structure.
> >
> > *dfl_fpga_enum_info_free
> > free dfl_fpga_enum_info data structure and related resources.
> >
> > Functions for feature device enumeration:
> > *dfl_fpga_enumerate_feature_devs
> > enumerate feature devices and return container device.
> >
> > *dfl_fpga_remove_feature_devs
> > remove feature devices under given container device.
>
> How about dfl_fpga_feature_devs_enumerate/remove?

Sure, will rename it in v6.

>
> >
> > Signed-off-by: Tim Whisonant <tim.whisonant@intel.com>
> > Signed-off-by: Enno Luebbers <enno.luebbers@intel.com>
> > Signed-off-by: Shiva Rao <shiva.rao@intel.com>
> > Signed-off-by: Christopher Rauer <christopher.rauer@intel.com>
> > Signed-off-by: Zhang Yi <yi.z.zhang@intel.com>
> > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > ---
> > v3: split from another patch.
> > separate dfl enumeration code from original pcie driver.
> > provide common data structures and APIs for enumeration.
> > update device feature list parsing process according to latest hw.
> > add dperf/iperf/hssi sub feature placeholder according to latest hw.
> > remove build_info_add_sub_feature and other small functions.
> > replace *_feature_num function with macro.
> > remove writeq/readq.
> > v4: fix SPDX license issue
> > rename files to dfl.[ch], fix typo and add more comments.
> > remove static feature_info tables for FME and Port.
> > remove check on next_afu link list as only FIU has next_afu ptr.
> > remove unused macro in header file.
> > add more comments for functions.
> > v5: add "dfl_" prefix to functions and data structures.
> > remove port related functions from DFL framework.
> > use BIT_ULL for 64bit register definition.
> > save dfl_fpga_cdev in pdata for feature platform devices.
> > rebase due to fpga region api changes.
> > ---
> > drivers/fpga/Kconfig | 16 ++
> > drivers/fpga/Makefile | 3 +
> > drivers/fpga/dfl.c | 720 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > drivers/fpga/dfl.h | 279 +++++++++++++++++++
> > 4 files changed, 1018 insertions(+)
> > create mode 100644 drivers/fpga/dfl.c
> > create mode 100644 drivers/fpga/dfl.h
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index f47ef84..01ad31f 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -124,4 +124,20 @@ config OF_FPGA_REGION
> > Support for loading FPGA images by applying a Device Tree
> > overlay.
> >
> > +config FPGA_DFL
> > + tristate "FPGA Device Feature List (DFL) support"
> > + select FPGA_BRIDGE
> > + select FPGA_REGION
> > + help
> > + Device Feature List (DFL) defines a feature list structure that
> > + creates a link list of feature headers within the MMIO space
> > + to provide an extensible way of adding features for FPGA.
> > + Driver can walk through the feature headers to enumerate feature
> > + devices (e.g FPGA Management Engine, Port and Accelerator
> > + Function Unit) and their private features for target FPGA devices.
> > +
> > + Select this option to enable common support for Field-Programmable
> > + Gate Array (FPGA) solutions which implement Device Feature List.
> > + It provides enumeration APIs, and feature device infrastructure.
> > +
> > endif # FPGA
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index 3cb276a..c4c62b9 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -27,3 +27,6 @@ obj-$(CONFIG_XILINX_PR_DECOUPLER) += xilinx-pr-decoupler.o
> > # High Level Interfaces
> > obj-$(CONFIG_FPGA_REGION) += fpga-region.o
> > obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.o
> > +
> > +# FPGA Device Feature List Support
> > +obj-$(CONFIG_FPGA_DFL) += dfl.o
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > new file mode 100644
> > index 0000000..c1462e9
> > --- /dev/null
> > +++ b/drivers/fpga/dfl.c
> > @@ -0,0 +1,720 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for FPGA Device Feature List (DFL) Support
> > + *
> > + * Copyright (C) 2017 Intel Corporation, Inc.
>
> 2017-2018

I will fix this in all driver files in v6.

>
> > + *
> > + * Authors:
> > + * Kang Luwei <luwei.kang@intel.com>
> > + * Zhang Yi <yi.z.zhang@intel.com>
> > + * Wu Hao <hao.wu@intel.com>
> > + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
> > + */
> > +#include <linux/module.h>
> > +
> > +#include "dfl.h"
> > +
> > +static DEFINE_MUTEX(dfl_id_mutex);
> > +
> > +enum dfl_id_type {
> > + FME_ID, /* fme id allocation and mapping */
> > + PORT_ID, /* port id allocation and mapping */
> > + DFL_ID_MAX,
> > +};
>
> Please add a comment that new values of DFL_ID need to be added to this enum.
>
> It may make sense to add dfl_chrdevs[] (from patch 7) here and add the
> DFH_ID id to that struct. That would give you one struct that has all
> that info together in one place and new feature drivers would be added
> to it. Any translations between dfl_id, char dev name, and
> dfl_fpga_devt_type could use that one table, and it could cut down on
> the number of if statements and special cases involved in parsing. I
> give a few examples of usage below.

Sure, understand your point on this. Will try to fix this, and also provide
clear comments on how to add new ones.

Actually dfl_id maps to platform device name, and dfl_fpga_dev_type to char
device name. will try to find a way to do translations or just combine them
as currently each feature device (platform device) only need one chardev.

>
> > +
> > +/* it is protected by dfl_id_mutex */
> > +static struct idr dfl_ids[DFL_ID_MAX];
> > +
> > +static void dfl_ids_init(void)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(dfl_ids); i++)
> > + idr_init(dfl_ids + i);
> > +}
> > +
> > +static void dfl_ids_destroy(void)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(dfl_ids); i++)
> > + idr_destroy(dfl_ids + i);
> > +}
> > +
> > +static int alloc_dfl_id(enum dfl_id_type type, struct device *dev)
> > +{
> > + int id;
> > +
> > + WARN_ON(type >= DFL_ID_MAX);
> > + mutex_lock(&dfl_id_mutex);
> > + id = idr_alloc(dfl_ids + type, dev, 0, 0, GFP_KERNEL);
> > + mutex_unlock(&dfl_id_mutex);
> > +
> > + return id;
> > +}
> > +
> > +static void free_dfl_id(enum dfl_id_type type, int id)
> > +{
> > + WARN_ON(type >= DFL_ID_MAX);
> > + mutex_lock(&dfl_id_mutex);
> > + idr_remove(dfl_ids + type, id);
> > + mutex_unlock(&dfl_id_mutex);
> > +}
>
> We discussed function name groups having a matching prefix on another
> thread for another patch. Same thing here, please, such as
> dfl_id_alloc/free.

Yes, will fix this in v6.

>
> > +
> > +static enum dfl_id_type feature_dev_id_type(struct platform_device *pdev)
> > +{
> > + if (!strcmp(pdev->name, DFL_FPGA_FEATURE_DEV_FME))
> > + return FME_ID;
> > +
> > + if (!strcmp(pdev->name, DFL_FPGA_FEATURE_DEV_PORT))
> > + return PORT_ID;
>
> Will a new if statement need to be added to this function for each
> added DFH_ID? IIUC, at the point this function is called, the feature
> device exists and has private data. Perhaps it is worth saving the ID
> in its private data so you don't have to do this reverse lookup (and
> won't have to change this function for each new feature).
>
> Alternatively, if dfl_chrdevs[] is added in this patch, you could use
> it here, using a loop that goes through and does the lookup from that
> struct and you won't ever have to touch this function again.
>
> In either case, if you add the names of your devices to a table like
> dfl_chrdevs[], hopefully things can be coded such that words like
> DFL_FPGA_FEATURE_DEV_FME/PORT only have to show up in that table (and
> in the individual drivers).

Understand your point, will fix this.

>
> > +
> > + WARN_ON(1);
> > +
> > + return DFL_ID_MAX;
> > +}
> > +
> > +/**
> > + * struct build_feature_devs_info - info collected during feature dev build.
> > + *
> > + * @dev: device to enumerate.
> > + * @cdev: the container device for all feature devices.
> > + * @feature_dev: current feature device.
> > + * @ioaddr: header register region address of feature device in enumeration.
> > + * @sub_features: a sub features link list for feature device in enumeration.
> > + * @feature_num: number of sub features for feature device in enumeration.
> > + */
> > +struct build_feature_devs_info {
> > + struct device *dev;
> > + struct dfl_fpga_cdev *cdev;
> > + struct platform_device *feature_dev;
> > + void __iomem *ioaddr;
> > + struct list_head sub_features;
> > + int feature_num;
> > +};
> > +
> > +/**
> > + * struct dfl_feature_info - sub feature info collected during feature dev build
> > + *
> > + * @fid: id of this sub feature.
> > + * @mmio_res: mmio resource of this sub feature.
> > + * @ioaddr: mapped base address of mmio resource.
> > + * @node: node in sub_features link list.
> > + */
> > +struct dfl_feature_info {
> > + u64 fid;
> > + struct resource mmio_res;
> > + void __iomem *ioaddr;
> > + struct list_head node;
> > +};
> > +
> > +static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev,
> > + struct platform_device *port)
> > +{
> > + struct dfl_feature_platform_data *pdata = dev_get_platdata(&port->dev);
> > +
> > + mutex_lock(&cdev->lock);
> > + list_add(&pdata->node, &cdev->port_dev_list);
> > + get_device(&pdata->dev->dev);
> > + mutex_unlock(&cdev->lock);
> > +}
> > +
> > +/*
> > + * register current feature device, it is called when we need to switch to
> > + * another feature parsing or we have parsed all features on given device
> > + * feature list.
> > + */
> > +static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> > +{
> > + struct platform_device *fdev = binfo->feature_dev;
> > + struct dfl_feature_platform_data *pdata;
> > + struct dfl_feature_info *finfo, *p;
> > + int ret, index = 0;
> > +
> > + if (!fdev)
> > + return 0;
> > +
> > + /*
> > + * we do not need to care for the memory which is associated with
> > + * the platform device. After calling platform_device_unregister(),
> > + * it will be automatically freed by device's release() callback,
> > + * platform_device_release().
> > + */
> > + pdata = kzalloc(dfl_feature_platform_data_size(binfo->feature_num),
> > + GFP_KERNEL);
> > + if (pdata) {
> > + pdata->dev = fdev;
> > + pdata->num = binfo->feature_num;
> > + pdata->dfl_cdev = binfo->cdev;
> > + mutex_init(&pdata->lock);
> > + } else {
> > + return -ENOMEM;
> > + }
>
> if (!pdata)
> return -ENOMEM;
>
> pdata->dev = fdev; so on

will fix this.

>
> > +
> > + /*
> > + * the count should be initialized to 0 to make sure
> > + *__fpga_port_enable() following __fpga_port_disable()
> > + * works properly for port device.
> > + * and it should always be 0 for fme device.
> > + */
> > + WARN_ON(pdata->disable_count);
> > +
> > + fdev->dev.platform_data = pdata;
> > +
> > + /* each sub feature has one MMIO resource */
> > + fdev->num_resources = binfo->feature_num;
> > + fdev->resource = kcalloc(binfo->feature_num, sizeof(*fdev->resource),
> > + GFP_KERNEL);
> > + if (!fdev->resource)
> > + return -ENOMEM;
> > +
> > + /* fill features and resource information for feature dev */
> > + list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
> > + struct dfl_feature *feature = &pdata->features[index];
> > +
> > + /* save resource information for each feature */
> > + feature->id = finfo->fid;
> > + feature->resource_index = index;
> > + feature->ioaddr = finfo->ioaddr;
> > + fdev->resource[index++] = finfo->mmio_res;
> > +
> > + list_del(&finfo->node);
> > + kfree(finfo);
> > + }
> > +
> > + ret = platform_device_add(binfo->feature_dev);
> > + if (!ret) {
> > + if (feature_dev_id_type(binfo->feature_dev) == PORT_ID)
> > + dfl_fpga_cdev_add_port_dev(binfo->cdev,
> > + binfo->feature_dev);
> > + else
> > + binfo->cdev->fme_dev =
> > + get_device(&binfo->feature_dev->dev);
> > + /*
> > + * reset it to avoid build_info_free() freeing their resource.
> > + *
> > + * The resource of successfully registered feature devices
> > + * will be freed by platform_device_unregister(). See the
> > + * comments in build_info_create_dev().
> > + */
> > + binfo->feature_dev = NULL;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int
> > +build_info_create_dev(struct build_feature_devs_info *binfo,
> > + enum dfl_id_type type, const char *name,
>
> If dfl_chrdevs[] were moved to this patch, build_info_create_dev could
> use it to look up the name. That way you won't have to have separate
> functions for parse_feature_fme and parse_feature_port.

Yes, if we have the mapping table, then we don't need to pass the
related platform device name to this function. will fix this in v6.

>
> > + void __iomem *ioaddr)
> > +{
> > + struct platform_device *fdev;
> > + int ret;
> > +
> > + /* we will create a new device, commit current device first */
> > + ret = build_info_commit_dev(binfo);
> > + if (ret)
> > + return ret;
> > +
> > + /*
> > + * we use -ENODEV as the initialization indicator which indicates
> > + * whether the id need to be reclaimed
> > + */
> > + fdev = platform_device_alloc(name, -ENODEV);
> > + if (!fdev)
> > + return -ENOMEM;
> > +
> > + binfo->feature_dev = fdev;
> > + binfo->feature_num = 0;
> > + binfo->ioaddr = ioaddr;
> > + INIT_LIST_HEAD(&binfo->sub_features);
> > +
> > + fdev->id = alloc_dfl_id(type, &fdev->dev);
> > + if (fdev->id < 0)
> > + return fdev->id;
> > +
> > + fdev->dev.parent = &binfo->cdev->region->dev;
> > +
> > + return 0;
> > +}
> > +
> > +static void build_info_free(struct build_feature_devs_info *binfo)
> > +{
> > + struct dfl_feature_info *finfo, *p;
> > +
> > + /*
> > + * it is a valid id, free it. See comments in
> > + * build_info_create_dev()
> > + */
> > + if (binfo->feature_dev && binfo->feature_dev->id >= 0) {
> > + free_dfl_id(feature_dev_id_type(binfo->feature_dev),
> > + binfo->feature_dev->id);
> > +
> > + list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
> > + list_del(&finfo->node);
> > + kfree(finfo);
> > + }
> > + }
> > +
> > + platform_device_put(binfo->feature_dev);
> > +
> > + devm_kfree(binfo->dev, binfo);
> > +}
> > +
> > +static inline u32 feature_size(void __iomem *start)
> > +{
> > + u64 v = readq(start + DFH);
> > + u32 ofst = FIELD_GET(DFH_NEXT_HDR_OFST, v);
> > + /* workaround for private features with invalid size, use 4K instead */
> > + return ofst ? ofst : 4096;
> > +}
> > +
> > +static u64 feature_id(void __iomem *start)
> > +{
> > + u64 v = readq(start + DFH);
> > + u16 id = FIELD_GET(DFH_ID, v);
> > + u8 type = FIELD_GET(DFH_TYPE, v);
> > +
> > + if (type == DFH_TYPE_FIU)
> > + return FEATURE_ID_FIU_HEADER;
> > + else if (type == DFH_TYPE_PRIVATE)
> > + return id;
> > + else if (type == DFH_TYPE_AFU)
> > + return FEATURE_ID_AFU;
> > +
> > + WARN_ON(1);
> > + return 0;
> > +}
> > +
> > +/*
> > + * when create sub feature instances, for private features, it doesn't need
> > + * to provide resource size and feature id as they could be read from DFH
> > + * register. For afu sub feature, its register region only contains user
> > + * defined registers, so never trust any information from it, just use the
> > + * resource size information provided by its parent FIU.
> > + */
> > +static int
> > +create_feature_instance(struct build_feature_devs_info *binfo,
> > + struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst,
> > + resource_size_t size, u64 fid)
> > +{
> > + struct dfl_feature_info *finfo;
> > +
> > + /* read feature size and id if inputs are invalid */
> > + size = size ? size : feature_size(dfl->ioaddr + ofst);
> > + fid = fid ? fid : feature_id(dfl->ioaddr + ofst);
> > +
> > + if (dfl->len - ofst < size)
> > + return -EINVAL;
> > +
> > + finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
> > + if (!finfo)
> > + return -ENOMEM;
> > +
> > + finfo->fid = fid;
> > + finfo->mmio_res.start = dfl->start + ofst;
> > + finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> > + finfo->mmio_res.flags = IORESOURCE_MEM;
> > + finfo->ioaddr = dfl->ioaddr + ofst;
> > +
> > + list_add_tail(&finfo->node, &binfo->sub_features);
> > + binfo->feature_num++;
> > +
> > + return 0;
> > +}
> > +
> > +static int parse_feature_fme(struct build_feature_devs_info *binfo,
> > + struct dfl_fpga_enum_dfl *dfl,
> > + resource_size_t ofst)
> > +{
> > + int ret;
> > +
> > + ret = build_info_create_dev(binfo, FME_ID, DFL_FPGA_FEATURE_DEV_FME,
> > + dfl->ioaddr + ofst);
> > + if (ret)
> > + return ret;
> > +
> > + return create_feature_instance(binfo, dfl, ofst, 0, 0);
> > +}
> > +
> > +static int parse_feature_port(struct build_feature_devs_info *binfo,
> > + struct dfl_fpga_enum_dfl *dfl,
> > + resource_size_t ofst)
> > +{
> > + int ret;
> > +
> > + ret = build_info_create_dev(binfo, PORT_ID, DFL_FPGA_FEATURE_DEV_PORT,
> > + dfl->ioaddr + ofst);
> > + if (ret)
> > + return ret;
> > +
> > + return create_feature_instance(binfo, dfl, ofst, 0, 0);
> > +}
> > +
> > +static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
> > + struct dfl_fpga_enum_dfl *dfl,
> > + resource_size_t ofst)
> > +{
> > + u64 v = readq(binfo->ioaddr + PORT_HDR_CAP);
> > + u32 size = FIELD_GET(PORT_CAP_MMIO_SIZE, v) << 10;
> > +
> > + WARN_ON(!size);
> > +
> > + return create_feature_instance(binfo, dfl, ofst, size, FEATURE_ID_AFU);
> > +}
> > +
> > +static int parse_feature_afu(struct build_feature_devs_info *binfo,
> > + struct dfl_fpga_enum_dfl *dfl,
> > + resource_size_t ofst)
> > +{
> > + if (!binfo->feature_dev) {
> > + dev_err(binfo->dev, "this AFU does not belong to any FIU.\n");
> > + return -EINVAL;
> > + }
> > +
> > + switch (feature_dev_id_type(binfo->feature_dev)) {
> > + case PORT_ID:
> > + return parse_feature_port_afu(binfo, dfl, ofst);
> > + default:
> > + dev_info(binfo->dev, "AFU belonging to FIU %s is not supported yet.\n",
> > + binfo->feature_dev->name);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int parse_feature_fiu(struct build_feature_devs_info *binfo,
> > + struct dfl_fpga_enum_dfl *dfl,
> > + resource_size_t ofst)
> > +{
> > + u32 id, offset;
> > + u64 v;
> > + int ret = 0;
> > +
> > + v = readq(dfl->ioaddr + ofst + DFH);
> > + id = FIELD_GET(DFH_ID, v);
> > +
> > + switch (id) {
> > + case DFH_ID_FIU_FME:
> > + ret = parse_feature_fme(binfo, dfl, ofst);
> > + break;
> > + case DFH_ID_FIU_PORT:
> > + ret = parse_feature_port(binfo, dfl, ofst);
> > + break;
> > + default:
> > + dev_info(binfo->dev, "FIU TYPE %d is not supported yet.\n",
> > + id);
> > + }
>
> If the name lookup is added to build_info_create_dev(), then this
> switch statement goes away and the contents of parse_feature_fme/port
> are identical and trivial enough to be included here. My reason for
> looking for these things is to reduce, where possible, the places
> where a function needs to be added or changed to parse each new ID.

I see, will improve this.

Thanks
Hao