* Re: [PATCH v5 01/28] docs: fpga: add a document for FPGA Device Feature List (DFL) Framework Overview
From: Alan Tull @ 2018-06-06 16:16 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Enno Luebbers, Xiao Guangrong
In-Reply-To: <1525229431-3087-2-git-send-email-hao.wu@intel.com>
On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
I've acked the remaining patches with some changes requested. In your
v6, please add a patch to add yourself to the MAINTAINERS file or
whoever is planning on maintaining fpga/drivers/*dfl*
> Add a document for FPGA Device Feature List (DFL) Framework Overview.
>
> Signed-off-by: Enno Luebbers <enno.luebbers@intel.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
Thanks,
Alan
^ permalink raw reply
* Re: [PATCH v5 28/28] fpga: dfl: afu: add DFL_FPGA_PORT_DMA_MAP/UNMAP ioctls support
From: Alan Tull @ 2018-06-06 16:09 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <1525229431-3087-29-git-send-email-hao.wu@intel.com>
On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
> DMA memory regions are required for Accelerated Function Unit (AFU) usage.
> These two ioctls allow user space applications to map user memory regions
> for dma, and unmap them after use. Iova is returned from driver to user
> space application via DFL_FPGA_PORT_DMA_MAP ioctl. Application needs to unmap
> it after use, otherwise, driver will unmap them in device file release
> operation.
>
> Each AFU has its own rb tree to keep track of its mapped DMA regions.
>
> Ioctl interfaces:
> * DFL_FPGA_PORT_DMA_MAP
> Do the dma mapping per user_addr and length which provided by user.
> Return iova in provided struct dfl_fpga_port_dma_map.
>
> * DFL_FPGA_PORT_DMA_UNMAP
> Unmap the dma region per iova provided by user.
>
> 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: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
^ permalink raw reply
* Re: [PATCH v5 17/28] fpga: dfl: fme: add partial reconfiguration sub feature support
From: Alan Tull @ 2018-06-06 16:08 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <1525229431-3087-18-git-send-email-hao.wu@intel.com>
On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
My patches that change the API for creating managers, bridges, and
regions is in Greg's char-misc-next branch now, so that will hopefully
be in v4.18. You'll need to adjust for that, but that will be minor.
> From: Kang Luwei <luwei.kang@intel.com>
>
> Partial Reconfiguration (PR) is the most important function for FME. It
> allows reconfiguration for given Port/Accelerated Function Unit (AFU).
>
> It creates platform devices for fpga-mgr, fpga-regions and fpga-bridges,
> and invokes fpga-region's interface (fpga_region_program_fpga) for PR
> operation once PR request received via ioctl. Below user space interface
> is exposed by this sub feature.
>
> Ioctl interface:
> * DFL_FPGA_FME_PORT_PR
> Do partial reconfiguration per information from userspace, including
> target port(AFU), buffer size and address info. It returns error code
> to userspace if failed. For detailed PR error information, user needs
> to read fpga-mgr's status sysfs interface.
>
> 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: Kang Luwei <luwei.kang@intel.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
> ---
> v2: moved the code to drivers/fpga folder as suggested by Alan Tull.
> switched to GPLv2 license.
> removed status from FPGA_FME_PORT_PR ioctl data structure.
> added platform devices creation for fpga-mgr/fpga-region/fpga-bridge.
> switched to fpga-region interface fpga_region_program_fpga for PR.
> fixed comments from Alan Tull on FPGA_MGR_PARTIAL_RECONFIG flag usage.
> fixed kbuild warnings.
> v3: rename driver files to dfl-fme-*.
> rebase due to fpga APIs change.
> replace bitfields.
> switch to fpga_cdev_find_port to find port device.
> v4: rebase and correct comments for some function.
> fix SPDX license issue.
> remove unnecessary input parameter for destroy_bridge/region function.
> add dfl-fme-pr.h for PR sub feature data structure and registers.
> v5: rebase due to DFL framework API naming changes.
> improve naming for IOCTL API, functions and data structure.
> defer finding port platform device to fme bridge.
> pass mapped ioaddr to fme manager via pdata.
> remove useless devm_kfree().
> ---
> drivers/fpga/Makefile | 2 +-
> drivers/fpga/dfl-fme-main.c | 43 +++-
> drivers/fpga/dfl-fme-pr.c | 474 ++++++++++++++++++++++++++++++++++++++++++
> drivers/fpga/dfl-fme-pr.h | 84 ++++++++
> drivers/fpga/dfl-fme.h | 38 ++++
> include/uapi/linux/fpga-dfl.h | 28 +++
> 6 files changed, 667 insertions(+), 2 deletions(-)
> create mode 100644 drivers/fpga/dfl-fme-pr.c
> create mode 100644 drivers/fpga/dfl-fme-pr.h
> create mode 100644 drivers/fpga/dfl-fme.h
>
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index fbd1c85..3c44fc9 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -32,7 +32,7 @@ obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.o
> obj-$(CONFIG_FPGA_DFL) += dfl.o
> obj-$(CONFIG_FPGA_DFL_FME) += dfl-fme.o
>
> -dfl-fme-objs := dfl-fme-main.o
> +dfl-fme-objs := dfl-fme-main.o dfl-fme-pr.o
>
> # Drivers for FPGAs which implement DFL
> obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
> diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
> index 6a59c07..bcb9325 100644
> --- a/drivers/fpga/dfl-fme-main.c
> +++ b/drivers/fpga/dfl-fme-main.c
> @@ -19,6 +19,7 @@
> #include <linux/fpga-dfl.h>
>
> #include "dfl.h"
> +#include "dfl-fme.h"
>
> static ssize_t ports_num_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> @@ -113,6 +114,10 @@ static ssize_t bitstream_metadata_show(struct device *dev,
> .ops = &fme_hdr_ops,
> },
> {
> + .id = FME_FEATURE_ID_PR_MGMT,
> + .ops = &pr_mgmt_ops,
> + },
> + {
> .ops = NULL,
> },
> };
> @@ -187,6 +192,35 @@ static long fme_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> return -EINVAL;
> }
>
> +static int fme_dev_init(struct platform_device *pdev)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> + struct dfl_fme *fme;
> +
> + fme = devm_kzalloc(&pdev->dev, sizeof(*fme), GFP_KERNEL);
> + if (!fme)
> + return -ENOMEM;
> +
> + fme->pdata = pdata;
> +
> + mutex_lock(&pdata->lock);
> + dfl_fpga_pdata_set_private(pdata, fme);
> + mutex_unlock(&pdata->lock);
> +
> + return 0;
> +}
> +
> +static void fme_dev_destroy(struct platform_device *pdev)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> + struct dfl_fme *fme;
> +
> + mutex_lock(&pdata->lock);
> + fme = dfl_fpga_pdata_get_private(pdata);
> + dfl_fpga_pdata_set_private(pdata, NULL);
> + mutex_unlock(&pdata->lock);
> +}
> +
> static const struct file_operations fme_fops = {
> .owner = THIS_MODULE,
> .open = fme_open,
> @@ -198,10 +232,14 @@ static int fme_probe(struct platform_device *pdev)
> {
> int ret;
>
> - ret = dfl_fpga_dev_feature_init(pdev, fme_feature_drvs);
> + ret = fme_dev_init(pdev);
> if (ret)
> goto exit;
>
> + ret = dfl_fpga_dev_feature_init(pdev, fme_feature_drvs);
> + if (ret)
> + goto dev_destroy;
> +
> ret = dfl_fpga_register_dev_ops(pdev, &fme_fops, THIS_MODULE);
> if (ret)
> goto feature_uinit;
> @@ -210,6 +248,8 @@ static int fme_probe(struct platform_device *pdev)
>
> feature_uinit:
> dfl_fpga_dev_feature_uinit(pdev);
> +dev_destroy:
> + fme_dev_destroy(pdev);
> exit:
> return ret;
> }
> @@ -218,6 +258,7 @@ static int fme_remove(struct platform_device *pdev)
> {
> dfl_fpga_unregister_dev_ops(pdev);
> dfl_fpga_dev_feature_uinit(pdev);
> + fme_dev_destroy(pdev);
>
> return 0;
> }
> diff --git a/drivers/fpga/dfl-fme-pr.c b/drivers/fpga/dfl-fme-pr.c
> new file mode 100644
> index 0000000..3d5f8b9
> --- /dev/null
> +++ b/drivers/fpga/dfl-fme-pr.c
> @@ -0,0 +1,474 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for FPGA Management Engine (FME) Partial Reconfiguration
> + *
> + * Copyright (C) 2017 Intel Corporation, Inc.
> + *
> + * Authors:
> + * Kang Luwei <luwei.kang@intel.com>
> + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
> + * Wu Hao <hao.wu@intel.com>
> + * Joseph Grecco <joe.grecco@intel.com>
> + * Enno Luebbers <enno.luebbers@intel.com>
> + * Tim Whisonant <tim.whisonant@intel.com>
> + * Ananda Ravuri <ananda.ravuri@intel.com>
> + * Christopher Rauer <christopher.rauer@intel.com>
> + * Henry Mitchel <henry.mitchel@intel.com>
> + */
> +
> +#include <linux/types.h>
> +#include <linux/device.h>
> +#include <linux/vmalloc.h>
> +#include <linux/uaccess.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/fpga/fpga-bridge.h>
> +#include <linux/fpga/fpga-region.h>
> +#include <linux/fpga-dfl.h>
> +
> +#include "dfl.h"
> +#include "dfl-fme.h"
> +#include "dfl-fme-pr.h"
> +
> +static struct dfl_fme_region *
> +find_fme_region_by_port_id(struct dfl_fme *fme, int port_id)
How about dfl_fme_region_find_by_port_id?
> +{
> + struct dfl_fme_region *fme_region;
> +
> + list_for_each_entry(fme_region, &fme->region_list, node)
> + if (fme_region->port_id == port_id)
> + return fme_region;
> +
> + return NULL;
> +}
> +
> +static int dfl_fme_region_match(struct device *dev, const void *data)
> +{
> + return dev->parent == data;
> +}
> +
> +static struct fpga_region *dfl_fme_region_find(struct dfl_fme *fme, int port_id)
> +{
> + struct dfl_fme_region *fme_region;
> + struct fpga_region *region;
> +
> + fme_region = find_fme_region_by_port_id(fme, port_id);
> + if (!fme_region)
> + return NULL;
> +
> + region = fpga_region_class_find(NULL, &fme_region->region->dev,
> + dfl_fme_region_match);
> + if (!region)
> + return NULL;
> +
> + return region;
> +}
> +
> +static int fme_pr(struct platform_device *pdev, unsigned long arg)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> + void __user *argp = (void __user *)arg;
> + struct dfl_fpga_fme_port_pr port_pr;
> + struct fpga_image_info *info;
> + struct fpga_region *region;
> + void __iomem *fme_hdr;
> + struct dfl_fme *fme;
> + unsigned long minsz;
> + void *buf = NULL;
> + int ret = 0;
> + u64 v;
> +
> + minsz = offsetofend(struct dfl_fpga_fme_port_pr, buffer_address);
> +
> + if (copy_from_user(&port_pr, argp, minsz))
> + return -EFAULT;
> +
> + if (port_pr.argsz < minsz || port_pr.flags)
> + return -EINVAL;
> +
> + if (!IS_ALIGNED(port_pr.buffer_size, 4))
> + return -EINVAL;
> +
> + /* get fme header region */
> + fme_hdr = dfl_get_feature_ioaddr_by_id(&pdev->dev,
> + FME_FEATURE_ID_HEADER);
> +
> + /* check port id */
> + v = readq(fme_hdr + FME_HDR_CAP);
> + if (port_pr.port_id >= FIELD_GET(FME_CAP_NUM_PORTS, v)) {
> + dev_dbg(&pdev->dev, "port number more than maximum\n");
> + return -EINVAL;
> + }
> +
> + if (!access_ok(VERIFY_READ,
> + (void __user *)(unsigned long)port_pr.buffer_address,
> + port_pr.buffer_size))
> + return -EFAULT;
> +
> + buf = vmalloc(port_pr.buffer_size);
> + if (!buf)
> + return -ENOMEM;
> +
> + if (copy_from_user(buf,
> + (void __user *)(unsigned long)port_pr.buffer_address,
> + port_pr.buffer_size)) {
> + ret = -EFAULT;
> + goto free_exit;
> + }
> +
> + /* prepare fpga_image_info for PR */
> + info = fpga_image_info_alloc(&pdev->dev);
> + if (!info) {
> + ret = -ENOMEM;
> + goto free_exit;
> + }
> +
> + info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
> +
> + mutex_lock(&pdata->lock);
> + fme = dfl_fpga_pdata_get_private(pdata);
> + /* fme device has been unregistered. */
> + if (!fme) {
> + ret = -EINVAL;
> + goto unlock_exit;
> + }
> +
> + region = dfl_fme_region_find(fme, port_pr.port_id);
> + if (!region) {
> + ret = -EINVAL;
> + goto unlock_exit;
> + }
> +
> + fpga_image_info_free(region->info);
> +
> + info->buf = buf;
> + info->count = port_pr.buffer_size;
> + info->region_id = port_pr.port_id;
> + region->info = info;
> +
> + ret = fpga_region_program_fpga(region);
> +
> + if (region->get_bridges)
> + fpga_bridges_put(®ion->bridge_list);
> +
> + put_device(®ion->dev);
> +unlock_exit:
> + mutex_unlock(&pdata->lock);
> +free_exit:
> + vfree(buf);
> + if (copy_to_user((void __user *)arg, &port_pr, minsz))
> + return -EFAULT;
> +
> + return ret;
> +}
> +
> +/**
> + * dfl_fme_create_mgr - create fpga mgr platform device as child device
> + *
> + * @pdata: fme platform_device's pdata
> + *
> + * Return: mgr platform device if successful, and error code otherwise.
> + */
> +static struct platform_device *
> +dfl_fme_create_mgr(struct dfl_feature_platform_data *pdata,
> + struct dfl_feature *feature)
> +{
> + struct platform_device *mgr, *fme = pdata->dev;
> + struct dfl_fme_mgr_pdata mgr_pdata;
> + int ret = -ENOMEM;
> +
> + if (!feature->ioaddr)
> + return ERR_PTR(-ENODEV);
> +
> + mgr_pdata.ioaddr = feature->ioaddr;
> +
> + /*
> + * Each FME has only one fpga-mgr, so allocate platform device using
> + * the same FME platform device id.
> + */
> + mgr = platform_device_alloc(DFL_FPGA_FME_MGR, fme->id);
> + if (!mgr)
> + return ERR_PTR(ret);
> +
> + mgr->dev.parent = &fme->dev;
> +
> + ret = platform_device_add_data(mgr, &mgr_pdata, sizeof(mgr_pdata));
> + if (ret)
> + goto create_mgr_err;
> +
> + ret = platform_device_add(mgr);
> + if (ret)
> + goto create_mgr_err;
> +
> + return mgr;
> +
> +create_mgr_err:
> + platform_device_put(mgr);
> + return ERR_PTR(ret);
> +}
> +
> +/**
> + * dfl_fme_destroy_mgr - destroy fpga mgr platform device
> + * @pdata: fme platform device's pdata
> + */
> +static void dfl_fme_destroy_mgr(struct dfl_feature_platform_data *pdata)
> +{
> + struct dfl_fme *priv = dfl_fpga_pdata_get_private(pdata);
> +
> + platform_device_unregister(priv->mgr);
> +}
> +
> +/**
> + * dfl_fme_create_bridge - create fme fpga bridge platform device as child
> + *
> + * @pdata: fme platform device's pdata
> + * @port_id: port id for the bridge to be created.
> + *
> + * Return: bridge platform device if successful, and error code otherwise.
> + */
> +static struct dfl_fme_bridge *
> +dfl_fme_create_bridge(struct dfl_feature_platform_data *pdata, int port_id)
> +{
> + struct device *dev = &pdata->dev->dev;
> + struct dfl_fme_br_pdata br_pdata;
> + struct dfl_fme_bridge *fme_br;
> + int ret = -ENOMEM;
> +
> + fme_br = devm_kzalloc(dev, sizeof(*fme_br), GFP_KERNEL);
> + if (!fme_br)
> + return ERR_PTR(ret);
> +
> + br_pdata.cdev = pdata->dfl_cdev;
> + br_pdata.port_id = port_id;
> +
> + fme_br->br = platform_device_alloc(DFL_FPGA_FME_BRIDGE,
> + PLATFORM_DEVID_AUTO);
> + if (!fme_br->br)
> + return ERR_PTR(ret);
> +
> + fme_br->br->dev.parent = dev;
> +
> + ret = platform_device_add_data(fme_br->br, &br_pdata, sizeof(br_pdata));
> + if (ret)
> + goto create_br_err;
> +
> + ret = platform_device_add(fme_br->br);
> + if (ret)
> + goto create_br_err;
> +
> + return fme_br;
> +
> +create_br_err:
> + platform_device_put(fme_br->br);
> + return ERR_PTR(ret);
> +}
> +
> +/**
> + * dfl_fme_destroy_bridge - destroy fpga bridge platform device
> + * @fme_br: fme bridge to destroy
> + */
> +static void dfl_fme_destroy_bridge(struct dfl_fme_bridge *fme_br)
> +{
> + platform_device_unregister(fme_br->br);
> +}
> +
> +/**
> + * dfl_fme_destroy_bridge - destroy all fpga bridge platform device
> + * @pdata: fme platform device's pdata
> + */
> +static void dfl_fme_destroy_bridges(struct dfl_feature_platform_data *pdata)
> +{
> + struct dfl_fme *priv = dfl_fpga_pdata_get_private(pdata);
> + struct dfl_fme_bridge *fbridge, *tmp;
> +
> + list_for_each_entry_safe(fbridge, tmp, &priv->bridge_list, node) {
> + list_del(&fbridge->node);
> + dfl_fme_destroy_bridge(fbridge);
> + }
> +}
> +
> +/**
> + * dfl_fme_create_region - create fpga region platform device as child
> + *
> + * @pdata: fme platform device's pdata
> + * @mgr: mgr platform device needed for region
> + * @br: br platform device needed for region
> + * @port_id: port id
> + *
> + * Return: fme region if successful, and error code otherwise.
> + */
> +static struct dfl_fme_region *
> +dfl_fme_create_region(struct dfl_feature_platform_data *pdata,
> + struct platform_device *mgr,
> + struct platform_device *br, int port_id)
> +{
> + struct dfl_fme_region_pdata region_pdata;
> + struct device *dev = &pdata->dev->dev;
> + struct dfl_fme_region *fme_region;
> + int ret = -ENOMEM;
> +
> + fme_region = devm_kzalloc(dev, sizeof(*fme_region), GFP_KERNEL);
> + if (!fme_region)
> + return ERR_PTR(ret);
> +
> + region_pdata.mgr = mgr;
> + region_pdata.br = br;
> +
> + /*
> + * Each FPGA device may have more than one port, so allocate platform
> + * device using the same port platform device id.
> + */
> + fme_region->region = platform_device_alloc(DFL_FPGA_FME_REGION, br->id);
> + if (!fme_region->region)
> + return ERR_PTR(ret);
> +
> + fme_region->region->dev.parent = dev;
> +
> + ret = platform_device_add_data(fme_region->region, ®ion_pdata,
> + sizeof(region_pdata));
> + if (ret)
> + goto create_region_err;
> +
> + ret = platform_device_add(fme_region->region);
> + if (ret)
> + goto create_region_err;
> +
> + fme_region->port_id = port_id;
> +
> + return fme_region;
> +
> +create_region_err:
> + platform_device_put(fme_region->region);
> + return ERR_PTR(ret);
> +}
> +
> +/**
> + * dfl_fme_destroy_region - destroy fme region
> + * @fme_region: fme region to destroy
> + */
> +static void dfl_fme_destroy_region(struct dfl_fme_region *fme_region)
> +{
> + platform_device_unregister(fme_region->region);
> +}
> +
> +/**
> + * dfl_fme_destroy_regions - destroy all fme regions
> + * @pdata: fme platform device's pdata
> + */
> +static void dfl_fme_destroy_regions(struct dfl_feature_platform_data *pdata)
> +{
> + struct dfl_fme *priv = dfl_fpga_pdata_get_private(pdata);
> + struct dfl_fme_region *fme_region, *tmp;
> +
> + list_for_each_entry_safe(fme_region, tmp, &priv->region_list, node) {
> + list_del(&fme_region->node);
> + dfl_fme_destroy_region(fme_region);
> + }
> +}
> +
> +static int
> +pr_mgmt_init(struct platform_device *pdev, struct dfl_feature *feature)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> + struct dfl_fme_region *fme_region;
> + struct dfl_fme_bridge *fme_br;
> + struct platform_device *mgr;
> + struct dfl_fme *priv;
> + void __iomem *fme_hdr;
> + int ret = -ENODEV, i = 0;
> + u64 fme_cap, port_offset;
> +
> + fme_hdr = dfl_get_feature_ioaddr_by_id(&pdev->dev,
> + FME_FEATURE_ID_HEADER);
> +
> + mutex_lock(&pdata->lock);
> + priv = dfl_fpga_pdata_get_private(pdata);
> +
> + /* Initialize the region and bridge sub device list */
> + INIT_LIST_HEAD(&priv->region_list);
> + INIT_LIST_HEAD(&priv->bridge_list);
> +
> + /* Create fpga mgr platform device */
> + mgr = dfl_fme_create_mgr(pdata, feature);
> + if (IS_ERR(mgr)) {
> + dev_err(&pdev->dev, "fail to create fpga mgr pdev\n");
> + goto unlock;
> + }
> +
> + priv->mgr = mgr;
> +
> + /* Read capability register to check number of regions and bridges */
> + fme_cap = readq(fme_hdr + FME_HDR_CAP);
> + for (; i < FIELD_GET(FME_CAP_NUM_PORTS, fme_cap); i++) {
> + port_offset = readq(fme_hdr + FME_HDR_PORT_OFST(i));
> + if (!(port_offset & FME_PORT_OFST_IMP))
> + continue;
> +
> + /* Create bridge for each port */
> + fme_br = dfl_fme_create_bridge(pdata, i);
> + if (IS_ERR(fme_br)) {
> + ret = PTR_ERR(fme_br);
> + goto destroy_region;
> + }
> +
> + list_add(&fme_br->node, &priv->bridge_list);
> +
> + /* Create region for each port */
> + fme_region = dfl_fme_create_region(pdata, mgr,
> + fme_br->br, i);
> + if (!fme_region) {
> + ret = PTR_ERR(fme_region);
> + goto destroy_region;
> + }
> +
> + list_add(&fme_region->node, &priv->region_list);
> + }
> + mutex_unlock(&pdata->lock);
> +
> + return 0;
> +
> +destroy_region:
> + dfl_fme_destroy_regions(pdata);
> + dfl_fme_destroy_bridges(pdata);
> + dfl_fme_destroy_mgr(pdata);
> +unlock:
> + mutex_unlock(&pdata->lock);
> + return ret;
> +}
> +
> +static void pr_mgmt_uinit(struct platform_device *pdev,
> + struct dfl_feature *feature)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> + struct dfl_fme *priv;
> +
> + mutex_lock(&pdata->lock);
> + priv = dfl_fpga_pdata_get_private(pdata);
> +
> + dfl_fme_destroy_regions(pdata);
> + dfl_fme_destroy_bridges(pdata);
> + dfl_fme_destroy_mgr(pdata);
> + mutex_unlock(&pdata->lock);
> +}
> +
> +static long fme_pr_ioctl(struct platform_device *pdev,
> + struct dfl_feature *feature,
> + unsigned int cmd, unsigned long arg)
> +{
> + long ret;
> +
> + switch (cmd) {
> + case DFL_FPGA_FME_PORT_PR:
> + ret = fme_pr(pdev, arg);
> + break;
> + default:
> + ret = -ENODEV;
> + }
> +
> + return ret;
> +}
> +
> +const struct dfl_feature_ops pr_mgmt_ops = {
> + .init = pr_mgmt_init,
> + .uinit = pr_mgmt_uinit,
> + .ioctl = fme_pr_ioctl,
> +};
> diff --git a/drivers/fpga/dfl-fme-pr.h b/drivers/fpga/dfl-fme-pr.h
> new file mode 100644
> index 0000000..2e60b53
> --- /dev/null
> +++ b/drivers/fpga/dfl-fme-pr.h
> @@ -0,0 +1,84 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Header file for FPGA Management Engine (FME) Partial Reconfiguration Driver
> + *
> + * Copyright (C) 2017 Intel Corporation, Inc.
> + *
> + * Authors:
> + * Kang Luwei <luwei.kang@intel.com>
> + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
> + * Wu Hao <hao.wu@intel.com>
> + * Joseph Grecco <joe.grecco@intel.com>
> + * Enno Luebbers <enno.luebbers@intel.com>
> + * Tim Whisonant <tim.whisonant@intel.com>
> + * Ananda Ravuri <ananda.ravuri@intel.com>
> + * Henry Mitchel <henry.mitchel@intel.com>
> + */
> +
> +#ifndef __DFL_FME_PR_H
> +#define __DFL_FME_PR_H
> +
> +#include <linux/platform_device.h>
> +
> +/**
> + * struct dfl_fme_region - FME fpga region data structure
> + *
> + * @region: platform device of the FPGA region.
> + * @node: used to link fme_region to a list.
> + * @port_id: indicate which port this region connected to.
> + */
> +struct dfl_fme_region {
> + struct platform_device *region;
> + struct list_head node;
> + int port_id;
> +};
> +
> +/**
> + * struct dfl_fme_region_pdata - platform data for FME region platform device.
> + *
> + * @mgr: platform device of the FPGA manager.
> + * @br: platform device of the FPGA bridge.
> + * @region_id: region id (same as port_id).
> + */
> +struct dfl_fme_region_pdata {
> + struct platform_device *mgr;
> + struct platform_device *br;
> + int region_id;
> +};
> +
> +/**
> + * struct dfl_fme_bridge - FME fpga bridge data structure
> + *
> + * @br: platform device of the FPGA bridge.
> + * @node: used to link fme_bridge to a list.
> + */
> +struct dfl_fme_bridge {
> + struct platform_device *br;
> + struct list_head node;
> +};
> +
> +/**
> + * struct dfl_fme_bridge_pdata - platform data for FME bridge platform device.
> + *
> + * @cdev: container device.
> + * @port_id: port id.
> + */
> +struct dfl_fme_br_pdata {
> + struct dfl_fpga_cdev *cdev;
> + int port_id;
> +};
> +
> +/**
> + * struct dfl_fme_mgr_pdata - platform data for FME manager platform device.
> + *
> + * @ioaddr: mapped io address for FME manager platform device.
> + */
> +struct dfl_fme_mgr_pdata {
> + void __iomem *ioaddr;
> +};
> +
> +#define DFL_FPGA_FME_MGR "dfl-fme-mgr"
> +#define DFL_FPGA_FME_BRIDGE "dfl-fme-bridge"
> +#define DFL_FPGA_FME_REGION "dfl-fme-region"
> +
> +#endif /* __DFL_FME_PR_H */
> diff --git a/drivers/fpga/dfl-fme.h b/drivers/fpga/dfl-fme.h
> new file mode 100644
> index 0000000..73b864d
> --- /dev/null
> +++ b/drivers/fpga/dfl-fme.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Header file for FPGA Management Engine (FME) Driver
> + *
> + * Copyright (C) 2017 Intel Corporation, Inc.
> + *
> + * Authors:
> + * Kang Luwei <luwei.kang@intel.com>
> + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
> + * Wu Hao <hao.wu@intel.com>
> + * Joseph Grecco <joe.grecco@intel.com>
> + * Enno Luebbers <enno.luebbers@intel.com>
> + * Tim Whisonant <tim.whisonant@intel.com>
> + * Ananda Ravuri <ananda.ravuri@intel.com>
> + * Henry Mitchel <henry.mitchel@intel.com>
> + */
> +
> +#ifndef __DFL_FME_H
> +#define __DFL_FME_H
> +
> +/**
> + * struct dfl_fme - dfl fme private data
> + *
> + * @mgr: FME's FPGA manager platform device.
> + * @region_list: link list of FME's FPGA regions.
> + * @bridge_list: link list of FME's FPGA bridges.
> + * @pdata: fme platform device's pdata.
> + */
> +struct dfl_fme {
> + struct platform_device *mgr;
> + struct list_head region_list;
> + struct list_head bridge_list;
> + struct dfl_feature_platform_data *pdata;
> +};
> +
> +extern const struct dfl_feature_ops pr_mgmt_ops;
> +
> +#endif /* __DFL_FME_H */
> diff --git a/include/uapi/linux/fpga-dfl.h b/include/uapi/linux/fpga-dfl.h
> index 7585de0..1c8bb2d 100644
> --- a/include/uapi/linux/fpga-dfl.h
> +++ b/include/uapi/linux/fpga-dfl.h
> @@ -14,6 +14,8 @@
> #ifndef _UAPI_LINUX_FPGA_DFL_H
> #define _UAPI_LINUX_FPGA_DFL_H
>
> +#include <linux/types.h>
> +
> #define DFL_FPGA_API_VERSION 0
>
> /*
> @@ -26,6 +28,7 @@
> #define DFL_FPGA_MAGIC 0xB6
>
> #define DFL_FPGA_BASE 0
> +#define DFL_FME_BASE 0x80
>
> /**
> * DFL_FPGA_GET_API_VERSION - _IO(DFL_FPGA_MAGIC, DFL_FPGA_BASE + 0)
> @@ -45,4 +48,29 @@
>
> #define DFL_FPGA_CHECK_EXTENSION _IO(DFL_FPGA_MAGIC, DFL_FPGA_BASE + 1)
>
> +/* IOCTLs for FME file descriptor */
> +
> +/**
> + * DFL_FPGA_FME_PORT_PR - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 0,
> + * struct dfl_fpga_fme_port_pr)
> + *
> + * Driver does Partial Reconfiguration based on Port ID and Buffer (Image)
> + * provided by caller.
> + * Return: 0 on success, -errno on failure.
> + * If DFL_FPGA_FME_PORT_PR returns -EIO, that indicates the HW has detected
> + * some errors during PR, under this case, the user can fetch HW error info
> + * from the status of FME's fpga manager.
> + */
> +
> +struct dfl_fpga_fme_port_pr {
> + /* Input */
> + __u32 argsz; /* Structure length */
> + __u32 flags; /* Zero for now */
> + __u32 port_id;
> + __u32 buffer_size;
> + __u64 buffer_address; /* Userspace address to the buffer for PR */
> +};
> +
> +#define DFL_FPGA_FME_PORT_PR _IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 0)
> +
> #endif /* _UAPI_LINUX_FPGA_DFL_H */
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH v5 14/28] fpga: dfl: add FPGA Management Engine driver basic framework
From: Alan Tull @ 2018-06-06 16:08 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <1525229431-3087-15-git-send-email-hao.wu@intel.com>
On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
> From: Kang Luwei <luwei.kang@intel.com>
>
> The FPGA Management Engine (FME) provides power, thermal management,
> performance counters, partial reconfiguration and other functions. For each
> function, it is packaged into a private feature linked to the FME feature
> device in the 'Device Feature List'. It's a platform device created by
> DFL framework.
>
> This patch adds the basic framework of FME platform driver. It defines
> sub feature drivers to handle the different sub features, including init,
> uinit and ioctl. It also registers the file operations for the device file.
>
> 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: Kang Luwei <luwei.kang@intel.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
> ---
> v3: rename driver from intel-fpga-fme to dfl-fme
> rename Kconfig from INTEL_FPGA_FME to FPGA_DFL_FME
> v4: fix SPDX license issue, use dfl-fme as module name
> v5: rebase, due to DFL framework naming changes on functions and data structures.
> fix uinit order in remove function.
> remove else block in fme_ioctl function per suggestion from Alan.
> ---
> drivers/fpga/Kconfig | 10 +++
> drivers/fpga/Makefile | 3 +
> drivers/fpga/dfl-fme-main.c | 158 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 171 insertions(+)
> create mode 100644 drivers/fpga/dfl-fme-main.c
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 87f3d44..103d5e2 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -140,6 +140,16 @@ config FPGA_DFL
> Gate Array (FPGA) solutions which implement Device Feature List.
> It provides enumeration APIs, and feature device infrastructure.
>
> +config FPGA_DFL_FME
> + tristate "FPGA DFL FME Driver"
> + depends on FPGA_DFL
> + help
> + The FPGA Management Engine (FME) is a feature device implemented
> + under Device Feature List (DFL) framework. Select this option to
> + enable the platform device driver for FME which implements all
> + FPGA platform level management features. There shall be 1 FME
> + per DFL based FPGA device.
> +
> config FPGA_DFL_PCI
> tristate "FPGA Device Feature List (DFL) PCIe Device Driver"
> depends on PCI && FPGA_DFL
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 4375630..fbd1c85 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -30,6 +30,9 @@ obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.o
>
> # FPGA Device Feature List Support
> obj-$(CONFIG_FPGA_DFL) += dfl.o
> +obj-$(CONFIG_FPGA_DFL_FME) += dfl-fme.o
> +
> +dfl-fme-objs := dfl-fme-main.o
>
> # Drivers for FPGAs which implement DFL
> obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
> diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
> new file mode 100644
> index 0000000..000651f
> --- /dev/null
> +++ b/drivers/fpga/dfl-fme-main.c
> @@ -0,0 +1,158 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for FPGA Management Engine (FME)
> + *
> + * Copyright (C) 2017 Intel Corporation, Inc.
> + *
> + * Authors:
> + * Kang Luwei <luwei.kang@intel.com>
> + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
> + * Joseph Grecco <joe.grecco@intel.com>
> + * Enno Luebbers <enno.luebbers@intel.com>
> + * Tim Whisonant <tim.whisonant@intel.com>
> + * Ananda Ravuri <ananda.ravuri@intel.com>
> + * Henry Mitchel <henry.mitchel@intel.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#include "dfl.h"
> +
> +static int
> +fme_hdr_init(struct platform_device *pdev, struct dfl_feature *feature)
Minor nit, where possible please stick with consistently having the
function name in the same line as the return type such as:
static int fme_hdr_init(struct platform_device *pdev, so on...
> +{
> + dev_dbg(&pdev->dev, "FME HDR Init.\n");
> +
> + return 0;
> +}
> +
> +static void
> +fme_hdr_uinit(struct platform_device *pdev, struct dfl_feature *feature)
> +{
> + dev_dbg(&pdev->dev, "FME HDR UInit.\n");
> +}
> +
> +static const struct dfl_feature_ops fme_hdr_ops = {
> + .init = fme_hdr_init,
> + .uinit = fme_hdr_uinit,
> +};
> +
> +static struct dfl_feature_driver fme_feature_drvs[] = {
> + {
> + .id = FME_FEATURE_ID_HEADER,
> + .ops = &fme_hdr_ops,
> + },
> + {
> + .ops = NULL,
> + },
> +};
> +
> +static int fme_open(struct inode *inode, struct file *filp)
> +{
> + struct platform_device *fdev = dfl_fpga_inode_to_feature_dev(inode);
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(&fdev->dev);
> + int ret;
> +
> + if (WARN_ON(!pdata))
> + return -ENODEV;
> +
> + ret = dfl_feature_dev_use_begin(pdata);
> + if (ret)
> + return ret;
> +
> + dev_dbg(&fdev->dev, "Device File Open\n");
> + filp->private_data = pdata;
> +
> + return 0;
> +}
> +
> +static int fme_release(struct inode *inode, struct file *filp)
> +{
> + struct dfl_feature_platform_data *pdata = filp->private_data;
> + struct platform_device *pdev = pdata->dev;
> +
> + dev_dbg(&pdev->dev, "Device File Release\n");
> + dfl_feature_dev_use_end(pdata);
> +
> + return 0;
> +}
> +
> +static long fme_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> + struct dfl_feature_platform_data *pdata = filp->private_data;
> + struct platform_device *pdev = pdata->dev;
> + struct dfl_feature *f;
> + long ret;
> +
> + dev_dbg(&pdev->dev, "%s cmd 0x%x\n", __func__, cmd);
> +
> + switch (cmd) {
> + default:
> + /*
> + * Let sub-feature's ioctl function to handle the cmd
> + * Sub-feature's ioctl returns -ENODEV when cmd is not
> + * handled in this sub feature, and returns 0 and other
> + * error code if cmd is handled.
> + */
> + dfl_fpga_dev_for_each_feature(pdata, f) {
> + if (f->ops && f->ops->ioctl) {
> + ret = f->ops->ioctl(pdev, f, cmd, arg);
> + if (ret != -ENODEV)
> + return ret;
> + }
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static const struct file_operations fme_fops = {
> + .owner = THIS_MODULE,
> + .open = fme_open,
> + .release = fme_release,
> + .unlocked_ioctl = fme_ioctl,
> +};
> +
> +static int fme_probe(struct platform_device *pdev)
> +{
> + int ret;
> +
> + ret = dfl_fpga_dev_feature_init(pdev, fme_feature_drvs);
> + if (ret)
> + goto exit;
> +
> + ret = dfl_fpga_register_dev_ops(pdev, &fme_fops, THIS_MODULE);
> + if (ret)
> + goto feature_uinit;
> +
> + return 0;
> +
> +feature_uinit:
> + dfl_fpga_dev_feature_uinit(pdev);
> +exit:
> + return ret;
> +}
> +
> +static int fme_remove(struct platform_device *pdev)
> +{
> + dfl_fpga_unregister_dev_ops(pdev);
> + dfl_fpga_dev_feature_uinit(pdev);
> +
> + return 0;
> +}
> +
> +static struct platform_driver fme_driver = {
> + .driver = {
> + .name = DFL_FPGA_FEATURE_DEV_FME,
> + },
> + .probe = fme_probe,
> + .remove = fme_remove,
> +};
> +
> +module_platform_driver(fme_driver);
> +
> +MODULE_DESCRIPTION("FPGA Management Engine driver");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:dfl-fme");
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH v5 27/28] fpga: dfl: afu: add afu sub feature support
From: Alan Tull @ 2018-06-06 16:04 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Xiao Guangrong, Tim Whisonant, Enno Luebbers,
Shiva Rao, Christopher Rauer
In-Reply-To: <1525229431-3087-28-git-send-email-hao.wu@intel.com>
On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
> From: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>
> User Accelerated Function Unit sub feature exposes the MMIO region of
> the AFU. After valid PR bitstream is programmed and the port is enabled,
> then this MMIO region could be accessed.
>
> This patch adds support to enumerate the AFU MMIO region and expose it
> to userspace via mmap file operation. Below interfaces are exposed to user:
>
> Sysfs interface:
> * /sys/class/fpga_region/<regionX>/<dfl-port.x>/afu_id
> Read-only. Indicate which PR bitstream is programmed to this AFU.
>
> Ioctl interfaces:
> * DFL_FPGA_PORT_GET_INFO
> Provide info to userspace on the number of supported region.
> Only UAFU region is supported now.
>
> * DFL_FPGA_PORT_GET_REGION_INFO
> Provide region information, including access permission, region size,
> offset from the start of device fd.
>
> 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: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
^ permalink raw reply
* Re: [PATCH v5 24/28] fpga: dfl: afu: add port ops support
From: Alan Tull @ 2018-06-06 15:57 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z
In-Reply-To: <1525229431-3087-25-git-send-email-hao.wu@intel.com>
On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
> This patch registers the port ops into the global list in the DFL
> framework, and it allows other modules to use the port ops. And
> This patch includes the implementation of the get_id and enable_set
> ops too.
>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
> ---
> drivers/fpga/dfl-afu-main.c | 122 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 121 insertions(+), 1 deletion(-)
^ permalink raw reply
* Re: [PATCH v5 18/28] fpga: dfl: add fpga manager platform driver for FME
From: Alan Tull @ 2018-06-06 15:52 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <1525229431-3087-19-git-send-email-hao.wu@intel.com>
On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
> This patch adds fpga manager driver for FPGA Management Engine (FME). It
> implements fpga_manager_ops for FPGA Partial Reconfiguration function.
>
> 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: Kang Luwei <luwei.kang@intel.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
> ---
> v3: rename driver to dfl-fpga-fme-mgr
> implemented status callback for fpga manager
> rebased due to fpga api changes
> v4: rename to dfl-fme-mgr, and fix SPDX license issue
> add pr_credit comments and improve dev_err message
> remove interface_id sysfs interface
> include dfl-fme-pr.h instead of dfl.h
> v5: move related register definition from dfl-fme-pr.h to dfl-fme-mgr.c
> use mapped ioaddr passed via pdata for register access.
> rebase due to fpga manager API changes.
> include header file for readq/writeq.
> ---
> drivers/fpga/Kconfig | 6 +
> drivers/fpga/Makefile | 1 +
> drivers/fpga/dfl-fme-mgr.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 341 insertions(+)
> create mode 100644 drivers/fpga/dfl-fme-mgr.c
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 103d5e2..89f76e8 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -150,6 +150,12 @@ config FPGA_DFL_FME
> FPGA platform level management features. There shall be 1 FME
> per DFL based FPGA device.
>
> +config FPGA_DFL_FME_MGR
> + tristate "FPGA DFL FME Manager Driver"
> + depends on FPGA_DFL_FME
> + help
> + Say Y to enable FPGA Manager driver for FPGA Management Engine.
> +
> config FPGA_DFL_PCI
> tristate "FPGA Device Feature List (DFL) PCIe Device Driver"
> depends on PCI && FPGA_DFL
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 3c44fc9..f82814a 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -31,6 +31,7 @@ obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.o
> # FPGA Device Feature List Support
> obj-$(CONFIG_FPGA_DFL) += dfl.o
> obj-$(CONFIG_FPGA_DFL_FME) += dfl-fme.o
> +obj-$(CONFIG_FPGA_DFL_FME_MGR) += dfl-fme-mgr.o
>
> dfl-fme-objs := dfl-fme-main.o dfl-fme-pr.o
>
> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
> new file mode 100644
> index 0000000..1c5bc5a
> --- /dev/null
> +++ b/drivers/fpga/dfl-fme-mgr.c
> @@ -0,0 +1,334 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * FPGA Manager Driver for FPGA Management Engine (FME)
> + *
> + * Copyright (C) 2017 Intel Corporation, Inc.
> + *
> + * Authors:
> + * Kang Luwei <luwei.kang@intel.com>
> + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
> + * Wu Hao <hao.wu@intel.com>
> + * Joseph Grecco <joe.grecco@intel.com>
> + * Enno Luebbers <enno.luebbers@intel.com>
> + * Tim Whisonant <tim.whisonant@intel.com>
> + * Ananda Ravuri <ananda.ravuri@intel.com>
> + * Christopher Rauer <christopher.rauer@intel.com>
> + * Henry Mitchel <henry.mitchel@intel.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/module.h>
> +#include <linux/iopoll.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/fpga/fpga-mgr.h>
> +
> +#include "dfl-fme-pr.h"
> +
> +/* FME Partial Reconfiguration Sub Feature Register Set */
> +#define FME_PR_DFH 0x0
> +#define FME_PR_CTRL 0x8
> +#define FME_PR_STS 0x10
> +#define FME_PR_DATA 0x18
> +#define FME_PR_ERR 0x20
> +#define FME_PR_INTFC_ID_H 0xA8
> +#define FME_PR_INTFC_ID_L 0xB0
> +
> +/* FME PR Control Register Bitfield */
> +#define FME_PR_CTRL_PR_RST BIT_ULL(0) /* Reset PR engine */
> +#define FME_PR_CTRL_PR_RSTACK BIT_ULL(4) /* Ack for PR engine reset */
> +#define FME_PR_CTRL_PR_RGN_ID GENMASK_ULL(9, 7) /* PR Region ID */
> +#define FME_PR_CTRL_PR_START BIT_ULL(12) /* Start to request PR service */
> +#define FME_PR_CTRL_PR_COMPLETE BIT_ULL(13) /* PR data push completion */
> +
> +/* FME PR Status Register Bitfield */
> +/* Number of available entries in HW queue inside the PR engine. */
> +#define FME_PR_STS_PR_CREDIT GENMASK_ULL(8, 0)
> +#define FME_PR_STS_PR_STS BIT_ULL(16) /* PR operation status */
> +#define FME_PR_STS_PR_STS_IDLE 0
> +#define FME_PR_STS_PR_CTRLR_STS GENMASK_ULL(22, 20) /* Controller status */
> +#define FME_PR_STS_PR_HOST_STS GENMASK_ULL(27, 24) /* PR host status */
> +
> +/* FME PR Data Register Bitfield */
> +/* PR data from the raw-binary file. */
> +#define FME_PR_DATA_PR_DATA_RAW GENMASK_ULL(32, 0)
> +
> +/* FME PR Error Register */
> +/* PR Operation errors detected. */
> +#define FME_PR_ERR_OPERATION_ERR BIT_ULL(0)
> +/* CRC error detected. */
> +#define FME_PR_ERR_CRC_ERR BIT_ULL(1)
> +/* Incompatible PR bitstream detected. */
> +#define FME_PR_ERR_INCOMPATIBLE_BS BIT_ULL(2)
> +/* PR data push protocol violated. */
> +#define FME_PR_ERR_PROTOCOL_ERR BIT_ULL(3)
> +/* PR data fifo overflow error detected */
> +#define FME_PR_ERR_FIFO_OVERFLOW BIT_ULL(4)
> +
> +#define PR_WAIT_TIMEOUT 8000000
> +#define PR_HOST_STATUS_IDLE 0
> +
> +struct fme_mgr_priv {
> + void __iomem *ioaddr;
> + u64 pr_error;
> +};
> +
> +static u64 pr_error_to_mgr_status(u64 err)
> +{
> + u64 status = 0;
> +
> + if (err & FME_PR_ERR_OPERATION_ERR)
> + status |= FPGA_MGR_STATUS_OPERATION_ERR;
> + if (err & FME_PR_ERR_CRC_ERR)
> + status |= FPGA_MGR_STATUS_CRC_ERR;
> + if (err & FME_PR_ERR_INCOMPATIBLE_BS)
> + status |= FPGA_MGR_STATUS_INCOMPATIBLE_IMAGE_ERR;
> + if (err & FME_PR_ERR_PROTOCOL_ERR)
> + status |= FPGA_MGR_STATUS_IP_PROTOCOL_ERR;
> + if (err & FME_PR_ERR_FIFO_OVERFLOW)
> + status |= FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR;
> +
> + return status;
> +}
> +
> +static u64 fme_mgr_pr_error_handle(void __iomem *fme_pr)
> +{
> + u64 pr_status, pr_error;
> +
> + pr_status = readq(fme_pr + FME_PR_STS);
> + if (!(pr_status & FME_PR_STS_PR_STS))
> + return 0;
> +
> + pr_error = readq(fme_pr + FME_PR_ERR);
> + writeq(pr_error, fme_pr + FME_PR_ERR);
> +
> + return pr_error;
> +}
> +
> +static int fme_mgr_write_init(struct fpga_manager *mgr,
> + struct fpga_image_info *info,
> + const char *buf, size_t count)
> +{
> + struct device *dev = &mgr->dev;
> + struct fme_mgr_priv *priv = mgr->priv;
> + void __iomem *fme_pr = priv->ioaddr;
> + u64 pr_ctrl, pr_status;
> +
> + if (!(info->flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> + dev_err(dev, "only supports partial reconfiguration.\n");
> + return -EINVAL;
> + }
> +
> + dev_dbg(dev, "resetting PR before initiated PR\n");
> +
> + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> + pr_ctrl |= FME_PR_CTRL_PR_RST;
> + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> +
> + if (readq_poll_timeout(fme_pr + FME_PR_CTRL, pr_ctrl,
> + pr_ctrl & FME_PR_CTRL_PR_RSTACK, 1,
> + PR_WAIT_TIMEOUT)) {
> + dev_err(dev, "PR Reset ACK timeout\n");
> + return -ETIMEDOUT;
> + }
> +
> + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> + pr_ctrl &= ~FME_PR_CTRL_PR_RST;
> + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> +
> + dev_dbg(dev,
> + "waiting for PR resource in HW to be initialized and ready\n");
> +
> + if (readq_poll_timeout(fme_pr + FME_PR_STS, pr_status,
> + (pr_status & FME_PR_STS_PR_STS) ==
> + FME_PR_STS_PR_STS_IDLE, 1, PR_WAIT_TIMEOUT)) {
> + dev_err(dev, "PR Status timeout\n");
> + priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
> + return -ETIMEDOUT;
> + }
> +
> + dev_dbg(dev, "check and clear previous PR error\n");
> + priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
> + if (priv->pr_error)
> + dev_dbg(dev, "previous PR error detected %llx\n",
> + (unsigned long long)priv->pr_error);
> +
> + dev_dbg(dev, "set PR port ID\n");
> +
> + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> + pr_ctrl &= ~FME_PR_CTRL_PR_RGN_ID;
> + pr_ctrl |= FIELD_PREP(FME_PR_CTRL_PR_RGN_ID, info->region_id);
> + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> +
> + return 0;
> +}
> +
> +static int fme_mgr_write(struct fpga_manager *mgr,
> + const char *buf, size_t count)
> +{
> + struct device *dev = &mgr->dev;
> + struct fme_mgr_priv *priv = mgr->priv;
> + void __iomem *fme_pr = priv->ioaddr;
> + u64 pr_ctrl, pr_status, pr_data;
> + int delay = 0, pr_credit, i = 0;
> +
> + dev_dbg(dev, "start request\n");
> +
> + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> + pr_ctrl |= FME_PR_CTRL_PR_START;
> + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> +
> + dev_dbg(dev, "pushing data from bitstream to HW\n");
> +
> + /*
> + * driver can push data to PR hardware using PR_DATA register once HW
> + * has enough pr_credit (> 1), pr_credit reduces one for every 32bit
> + * pr data write to PR_DATA register. If pr_credit <= 1, driver needs
> + * to wait for enough pr_credit from hardware by polling.
> + */
> + pr_status = readq(fme_pr + FME_PR_STS);
> + pr_credit = FIELD_GET(FME_PR_STS_PR_CREDIT, pr_status);
> +
> + while (count > 0) {
> + while (pr_credit <= 1) {
> + if (delay++ > PR_WAIT_TIMEOUT) {
> + dev_err(dev, "PR_CREDIT timeout\n");
> + return -ETIMEDOUT;
> + }
> + udelay(1);
> +
> + pr_status = readq(fme_pr + FME_PR_STS);
> + pr_credit = FIELD_GET(FME_PR_STS_PR_CREDIT, pr_status);
> + }
> +
> + if (count < 4) {
> + WARN_ON(1);
> + return -EINVAL;
> + }
> +
> + pr_data = 0;
> + pr_data |= FIELD_PREP(FME_PR_DATA_PR_DATA_RAW,
> + *(((u32 *)buf) + i));
> + writeq(pr_data, fme_pr + FME_PR_DATA);
> + count -= 4;
> + pr_credit--;
> + i++;
> + }
> +
> + return 0;
> +}
> +
> +static int fme_mgr_write_complete(struct fpga_manager *mgr,
> + struct fpga_image_info *info)
> +{
> + struct device *dev = &mgr->dev;
> + struct fme_mgr_priv *priv = mgr->priv;
> + void __iomem *fme_pr = priv->ioaddr;
> + u64 pr_ctrl;
> +
> + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> + pr_ctrl |= FME_PR_CTRL_PR_COMPLETE;
> + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> +
> + dev_dbg(dev, "green bitstream push complete\n");
> + dev_dbg(dev, "waiting for HW to release PR resource\n");
> +
> + if (readq_poll_timeout(fme_pr + FME_PR_CTRL, pr_ctrl,
> + !(pr_ctrl & FME_PR_CTRL_PR_START), 1,
> + PR_WAIT_TIMEOUT)) {
> + dev_err(dev, "PR Completion ACK timeout.\n");
> + return -ETIMEDOUT;
> + }
> +
> + dev_dbg(dev, "PR operation complete, checking status\n");
> + priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
> + if (priv->pr_error) {
> + dev_dbg(dev, "PR error detected %llx\n",
> + (unsigned long long)priv->pr_error);
> + return -EIO;
> + }
> +
> + dev_dbg(dev, "PR done successfully\n");
> +
> + return 0;
> +}
> +
> +static enum fpga_mgr_states fme_mgr_state(struct fpga_manager *mgr)
> +{
> + return FPGA_MGR_STATE_UNKNOWN;
> +}
> +
> +static u64 fme_mgr_status(struct fpga_manager *mgr)
> +{
> + struct fme_mgr_priv *priv = mgr->priv;
> +
> + return pr_error_to_mgr_status(priv->pr_error);
> +}
> +
> +static const struct fpga_manager_ops fme_mgr_ops = {
> + .write_init = fme_mgr_write_init,
> + .write = fme_mgr_write,
> + .write_complete = fme_mgr_write_complete,
> + .state = fme_mgr_state,
> + .status = fme_mgr_status,
> +};
> +
> +static int fme_mgr_probe(struct platform_device *pdev)
> +{
> + struct dfl_fme_mgr_pdata *pdata = dev_get_platdata(&pdev->dev);
> + struct device *dev = &pdev->dev;
> + struct fme_mgr_priv *priv;
> + struct fpga_manager *mgr;
> + struct resource *res;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + if (pdata->ioaddr)
> + priv->ioaddr = pdata->ioaddr;
> +
> + if (!priv->ioaddr) {
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv->ioaddr = devm_ioremap_resource(dev, res);
> + if (IS_ERR(priv->ioaddr))
> + return PTR_ERR(priv->ioaddr);
> + }
> +
> + mgr = fpga_mgr_create(dev, "DFL FME FPGA Manager",
> + &fme_mgr_ops, priv);
> + if (!mgr)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, mgr);
> +
> + ret = fpga_mgr_register(mgr);
> + if (ret)
> + fpga_mgr_free(mgr);
> +
> + return ret;
> +}
> +
> +static int fme_mgr_remove(struct platform_device *pdev)
> +{
> + struct fpga_manager *mgr = platform_get_drvdata(pdev);
> +
> + fpga_mgr_unregister(mgr);
> +
> + return 0;
> +}
> +
> +static struct platform_driver fme_mgr_driver = {
> + .driver = {
> + .name = DFL_FPGA_FME_MGR,
> + },
> + .probe = fme_mgr_probe,
> + .remove = fme_mgr_remove,
> +};
> +
> +module_platform_driver(fme_mgr_driver);
> +
> +MODULE_DESCRIPTION("FPGA Manager for DFL FPGA Management Engine");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:dfl-fme-mgr");
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH v5 09/28] fpga: dfl: add feature device infrastructure
From: Wu Hao @ 2018-06-06 12:33 UTC (permalink / raw)
To: Alan Tull
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Xiao Guangrong, Tim Whisonant, Enno Luebbers,
Shiva Rao, Christopher Rauer
In-Reply-To: <CANk1AXRyqn_Y26HZiYkFQUoWUVzc89F-znVNWQXGAk+arNB6XA@mail.gmail.com>
On Tue, Jun 05, 2018 at 04:14:43PM -0500, Alan Tull wrote:
> On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
>
> Hi Hao,
>
> Some minor things, otherwise looks fine.
>
> > From: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >
> > This patch abstracts the common operations of the sub features, and defines
> > the feature_ops data structure, including init, uinit and ioctl function
> > pointers. And this patch adds some common helper functions for FME and AFU
> > drivers, e.g dfl_feature_dev_use_begin/end which are used to ensure
> > exclusive usage of the feature device file.
> >
> > 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: Kang Luwei <luwei.kang@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>
> Acked-by: Alan Tull <atull@kernel.org>
>
> > ---
> > v2: rebased
> > v3: use const for feature_ops.
> > replace pci related function.
> > v4: rebase and add more comments in code.
> > v5: remove useless WARN_ON().
> > reorder declarations in functions per suggestion from Moritz.
> > add "dfl_" prefix to functions and data structure.
> > ---
> > drivers/fpga/dfl.c | 57 +++++++++++++++++++++++++++++++++++++
> > drivers/fpga/dfl.h | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 138 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index 1e06efb..c4c47d6 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -74,6 +74,63 @@ static enum dfl_id_type feature_dev_id_type(struct platform_device *pdev)
> > return DFL_ID_MAX;
> > }
> >
> > +void dfl_fpga_dev_feature_uinit(struct platform_device *pdev)
> > +{
> > + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > + struct dfl_feature *feature;
> > +
> > + dfl_fpga_dev_for_each_feature(pdata, feature)
> > + if (feature->ops) {
> > + feature->ops->uinit(pdev, feature);
> > + feature->ops = NULL;
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit);
>
> Please add kernel-doc for functions that are exported to recommend and
> guide their usage.
Sorry, I missed uinit and init functions. will add them in v6.
>
> > +
> > +static int dfl_feature_instance_init(struct platform_device *pdev,
> > + struct dfl_feature_platform_data *pdata,
> > + struct dfl_feature *feature,
> > + struct dfl_feature_driver *drv)
> > +{
> > + int ret;
> > +
> > + ret = drv->ops->init(pdev, feature);
> > + if (ret)
> > + return ret;
> > +
> > + feature->ops = drv->ops;
> > +
> > + return ret;
> > +}
> > +
> > +int dfl_fpga_dev_feature_init(struct platform_device *pdev,
> > + struct dfl_feature_driver *feature_drvs)
> > +{
> > + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > + struct dfl_feature_driver *drv = feature_drvs;
> > + struct dfl_feature *feature;
> > + int ret;
> > +
> > + while (drv->ops) {
> > + dfl_fpga_dev_for_each_feature(pdata, feature) {
> > + /* match feature and drv using id */
> > + if (feature->id == drv->id) {
> > + ret = dfl_feature_instance_init(pdev, pdata,
> > + feature, drv);
> > + if (ret)
> > + goto exit;
> > + }
> > + }
> > + drv++;
> > + }
> > +
> > + return 0;
> > +exit:
> > + dfl_fpga_dev_feature_uinit(pdev);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init);
> > +
> > struct dfl_chardev_info {
> > const char *name;
> > dev_t devt;
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index 2b6aaef..27f7a74 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -132,6 +132,17 @@
> > #define PORT_CTRL_SFTRST_ACK BIT_ULL(4) /* HW ack for reset */
> >
> > /**
> > + * struct dfl_feature_driver - sub feature's driver
> > + *
> > + * @id: sub feature id.
> > + * @ops: ops of this sub feature.
> > + */
> > +struct dfl_feature_driver {
> > + u64 id;
> > + const struct dfl_feature_ops *ops;
> > +};
> > +
> > +/**
> > * struct dfl_feature - sub feature of the feature devices
> > *
> > * @id: sub feature id.
> > @@ -139,13 +150,17 @@
> > * this index is used to find its mmio resource from the
> > * feature dev (platform device)'s reources.
> > * @ioaddr: mapped mmio resource address.
> > + * @ops: ops of this sub feature.
> > */
> > struct dfl_feature {
> > u64 id;
> > int resource_index;
> > void __iomem *ioaddr;
> > + const struct dfl_feature_ops *ops;
> > };
> >
> > +#define DEV_STATUS_IN_USE 0
> > +
> > /**
> > * struct dfl_feature_platform_data - platform data for feature devices
> > *
> > @@ -156,6 +171,8 @@ struct dfl_feature {
> > * @dfl_cdev: ptr to container device.
> > * @disable_count: count for port disable.
> > * @num: number for sub features.
> > + * @dev_status: dev status (e.g DEV_STATUS_IN_USE).
> > + * @private: ptr to feature dev private data.
> > * @features: sub features of this feature dev.
> > */
> > struct dfl_feature_platform_data {
> > @@ -165,11 +182,49 @@ struct dfl_feature_platform_data {
> > struct platform_device *dev;
> > struct dfl_fpga_cdev *dfl_cdev;
> > unsigned int disable_count;
> > -
> > + unsigned long dev_status;
> > + void *private;
> > int num;
> > struct dfl_feature features[0];
> > };
> >
> > +static inline
> > +int dfl_feature_dev_use_begin(struct dfl_feature_platform_data *pdata)
> > +{
> > + /* Test and set IN_USE flags to ensure file is exclusively used */
> > + if (test_and_set_bit_lock(DEV_STATUS_IN_USE, &pdata->dev_status))
> > + return -EBUSY;
> > +
> > + return 0;
> > +}
> > +
> > +static inline
> > +void dfl_feature_dev_use_end(struct dfl_feature_platform_data *pdata)
> > +{
> > + clear_bit_unlock(DEV_STATUS_IN_USE, &pdata->dev_status);
> > +}
> > +
> > +static inline
> > +void dfl_fpga_pdata_set_private(struct dfl_feature_platform_data *pdata,
> > + void *private)
> > +{
> > + pdata->private = private;
> > +}
> > +
> > +static inline
> > +void *dfl_fpga_pdata_get_private(struct dfl_feature_platform_data *pdata)
> > +{
> > + return pdata->private;
> > +}
> > +
> > +struct dfl_feature_ops {
> > + int (*init)(struct platform_device *pdev, struct dfl_feature *feature);
> > + void (*uinit)(struct platform_device *pdev,
> > + struct dfl_feature *feature);
> > + long (*ioctl)(struct platform_device *pdev, struct dfl_feature *feature,
> > + unsigned int cmd, unsigned long arg);
> > +};
> > +
> > #define DFL_FPGA_FEATURE_DEV_FME "dfl-fme"
> > #define DFL_FPGA_FEATURE_DEV_PORT "dfl-port"
>
> Please move these to the same place as other things that will need to
> be added to as feature devices are added as noted in the other reviews
> today.
as these two strings are used as platform device name, so I think we need to
keep them in the dfl.h file, because platform driver could reuse the same.
But I will add detailed comments to guide others to put name string for new
feature device (platform device) in the dfl.h file together with above ones.
Thanks a lot for the review.
Hao
^ permalink raw reply
* Re: [PATCH v5 07/28] fpga: dfl: add chardev support for feature devices
From: Wu Hao @ 2018-06-06 12:24 UTC (permalink / raw)
To: Alan Tull
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <CANk1AXRQx=QtwmSKuWiGprMcfjNnUWcBHNy-hTfP+JaQZsJLLA@mail.gmail.com>
On Tue, Jun 05, 2018 at 03:21:48PM -0500, Alan Tull wrote:
> On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
>
> Hi Hao,
>
> > For feature devices drivers, both the FPGA Management Engine (FME) and
> > Accelerated Function Unit (AFU) driver need to expose user interfaces via
> > the device file, for example, mmap and ioctls.
> >
> > This patch adds chardev support in the dfl driver for feature devices,
> > FME and AFU. It reserves the chardev regions for FME and AFU, and provide
> > interfaces for FME and AFU driver to register their device file operations.
> >
> > 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>
> > ---
> > v2: rebased
> > v3: move chardev support to fpga-dfl framework
> > v4: rebase, and add more comments in code.
> > v5: rebase, and add dfl_ prefix to APIs and data structures.
> > ---
> > drivers/fpga/dfl.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > drivers/fpga/dfl.h | 14 ++++++++
> > 2 files changed, 117 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index c1462e9..18aba02 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -74,6 +74,96 @@ static enum dfl_id_type feature_dev_id_type(struct platform_device *pdev)
> > return DFL_ID_MAX;
> > }
> >
> > +struct dfl_chardev_info {
> > + const char *name;
> > + dev_t devt;
> > +};
> > +
> > +/* indexed by enum dfl_fpga_devt_type */
> > +struct dfl_chardev_info dfl_chrdevs[] = {
> > + {.name = DFL_FPGA_FEATURE_DEV_FME}, /* DFL_FPGA_DEVT_FME */
> > + {.name = DFL_FPGA_FEATURE_DEV_PORT}, /* DFL_FPGA_DEVT_AFU */
> > +};
>
> If this were added in the initial dfl.c patch, it could be used by
> build_info_create_dev to get the name.
Sure, will fix this.
>
> > +
> > +static void dfl_chardev_uinit(void)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < DFL_FPGA_DEVT_MAX; i++)
> > + if (MAJOR(dfl_chrdevs[i].devt)) {
> > + unregister_chrdev_region(dfl_chrdevs[i].devt,
> > + MINORMASK);
> > + dfl_chrdevs[i].devt = MKDEV(0, 0);
> > + }
> > +}
> > +
> > +static int dfl_chardev_init(void)
> > +{
> > + int i, ret;
> > +
> > + for (i = 0; i < DFL_FPGA_DEVT_MAX; i++) {
> > + ret = alloc_chrdev_region(&dfl_chrdevs[i].devt, 0, MINORMASK,
> > + dfl_chrdevs[i].name);
> > + if (ret)
> > + goto exit;
> > + }
> > +
> > + return 0;
> > +
> > +exit:
> > + dfl_chardev_uinit();
> > + return ret;
> > +}
> > +
> > +static dev_t dfl_get_devt(enum dfl_fpga_devt_type type, int id)
> > +{
> > + WARN_ON(type >= DFL_FPGA_DEVT_MAX);
> > +
> > + return MKDEV(MAJOR(dfl_chrdevs[type].devt), id);
> > +}
> > +
> > +/**
> > + * dfl_fpga_register_dev_ops - register cdev ops for feature dev
> > + *
> > + * @pdev: feature dev.
> > + * @fops: file operations for feature dev's cdev.
> > + * @owner: owning module/driver.
> > + *
> > + * Return: 0 on success, negative error code otherwise.
> > + */
> > +int dfl_fpga_register_dev_ops(struct platform_device *pdev,
> > + const struct file_operations *fops,
> > + struct module *owner)
> > +{
> > + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > +
> > + cdev_init(&pdata->cdev, fops);
> > + pdata->cdev.owner = owner;
> > +
> > + /*
> > + * set parent to the feature device so that its refcount is
> > + * decreased after the last refcount of cdev is gone, that
> > + * makes sure the feature device is valid during device
> > + * file's life-cycle.
> > + */
> > + pdata->cdev.kobj.parent = &pdev->dev.kobj;
> > +
> > + return cdev_add(&pdata->cdev, pdev->dev.devt, 1);
> > +}
> > +EXPORT_SYMBOL_GPL(dfl_fpga_register_dev_ops);
> > +
> > +/**
> > + * dfl_fpga_unregister_dev_ops - unregister cdev ops for feature dev
> > + * @pdev: feature dev.
> > + */
> > +void dfl_fpga_unregister_dev_ops(struct platform_device *pdev)
> > +{
> > + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > +
> > + cdev_del(&pdata->cdev);
> > +}
> > +EXPORT_SYMBOL_GPL(dfl_fpga_unregister_dev_ops);
> > +
> > /**
> > * struct build_feature_devs_info - info collected during feature dev build.
> > *
> > @@ -208,9 +298,13 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> > enum dfl_id_type type, const char *name,
> > void __iomem *ioaddr)
> > {
> > + enum dfl_fpga_devt_type devt_type = DFL_FPGA_DEVT_FME;
> > struct platform_device *fdev;
> > int ret;
> >
> > + if (type == PORT_ID)
> > + devt_type = DFL_FPGA_DEVT_PORT;
> > +
> > /* we will create a new device, commit current device first */
> > ret = build_info_commit_dev(binfo);
> > if (ret)
> > @@ -234,6 +328,7 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> > return fdev->id;
> >
> > fdev->dev.parent = &binfo->cdev->region->dev;
> > + fdev->dev.devt = dfl_get_devt(devt_type, fdev->id);
> >
> > return 0;
> > }
> > @@ -702,13 +797,20 @@ void dfl_fpga_remove_feature_devs(struct dfl_fpga_cdev *cdev)
> >
> > static int __init dfl_fpga_init(void)
> > {
> > + int ret;
> > +
> > dfl_ids_init();
> >
> > - return 0;
> > + ret = dfl_chardev_init();
> > + if (ret)
> > + dfl_ids_destroy();
> > +
> > + return ret;
> > }
> >
> > static void __exit dfl_fpga_exit(void)
> > {
> > + dfl_chardev_uinit();
> > dfl_ids_destroy();
> > }
> >
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index 2ede915..5fcb1a1 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -15,6 +15,7 @@
> > #define __FPGA_DFL_H
> >
> > #include <linux/bitfield.h>
> > +#include <linux/cdev.h>
> > #include <linux/delay.h>
> > #include <linux/fs.h>
> > #include <linux/iopoll.h>
> > @@ -150,6 +151,7 @@ struct dfl_feature {
> > *
> > * @node: node to link feature devs to container device's port_dev_list.
> > * @lock: mutex to protect platform data.
> > + * @cdev: cdev of feature dev.
> > * @dev: ptr to platform device linked with this platform data.
> > * @dfl_cdev: ptr to container device.
> > * @disable_count: count for port disable.
> > @@ -159,6 +161,7 @@ struct dfl_feature {
> > struct dfl_feature_platform_data {
> > struct list_head node;
> > struct mutex lock;
> > + struct cdev cdev;
> > struct platform_device *dev;
> > struct dfl_fpga_cdev *dfl_cdev;
> > unsigned int disable_count;
> > @@ -176,6 +179,17 @@ static inline int dfl_feature_platform_data_size(const int num)
> > num * sizeof(struct dfl_feature);
> > }
> >
> > +enum dfl_fpga_devt_type {
> > + DFL_FPGA_DEVT_FME,
> > + DFL_FPGA_DEVT_PORT,
> > + DFL_FPGA_DEVT_MAX,
> > +};
>
> Could you move this enum to be close to other similar enums (like
> dfl_id_type)? The dfl code has a few enums that are similar, and may
> need updating (or not) as feature are added. Putting them close
> together with appropriate comments would be helpful to keep them all
> straight.
Agree, I will fix this in v6.
Thanks for the review.
Hao
^ permalink raw reply
* Re: [PATCH v5 06/28] fpga: add device feature list support
From: Wu Hao @ 2018-06-06 12:22 UTC (permalink / raw)
To: Alan Tull
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <CANk1AXQcq4ozNA6_Ad5S1_dJHsT7OLb73xmJ9Fhjhbx1qnwa4Q@mail.gmail.com>
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
^ permalink raw reply
* Re: [PATCH 2/3] mm: add find_alloc_contig_pages() interface
From: Michal Hocko @ 2018-06-06 9:32 UTC (permalink / raw)
To: Mike Kravetz
Cc: Vlastimil Babka, linux-mm, linux-kernel, linux-api,
Reinette Chatre, Christopher Lameter, Guy Shattah,
Anshuman Khandual, Michal Nazarewicz, David Nellans, Laura Abbott,
Pavel Machek, Dave Hansen, Andrew Morton
In-Reply-To: <af56d281-fea1-2bd7-aff1-108fa1d9f3be@oracle.com>
On Wed 02-05-18 14:13:32, Mike Kravetz wrote:
> On 04/21/2018 09:16 AM, Vlastimil Babka wrote:
> > On 04/17/2018 04:09 AM, Mike Kravetz wrote:
> >> find_alloc_contig_pages() is a new interface that attempts to locate
> >> and allocate a contiguous range of pages. It is provided as a more
> >> convenient interface than alloc_contig_range() which is currently
> >> used by CMA and gigantic huge pages.
> >>
> >> When attempting to allocate a range of pages, migration is employed
> >> if possible. There is no guarantee that the routine will succeed.
> >> So, the user must be prepared for failure and have a fall back plan.
> >>
> >> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> >
> > Hi, just two quick observations, maybe discussion pointers for the
> > LSF/MM session:
> > - it's weird that find_alloc_contig_pages() takes an order, and
> > free_contig_pages() takes a nr_pages. I suspect the interface would be
> > more future-proof with both using nr_pages? Perhaps also minimum
> > alignment for the allocation side? Order is fine for hugetlb, but what
> > about other potential users?
>
> Agreed, and I am changing this to nr_pages and adding alignment.
>
> > - contig_alloc_migratetype_ok() says that MIGRATE_CMA blocks are OK to
> > allocate from. This silently assumes that everything allocated by this
> > will be migratable itself, or it might eat CMA reserves. Is it the case?
> > Also you then call alloc_contig_range() with MIGRATE_MOVABLE, so it will
> > skip/fail on MIGRATE_CMA anyway IIRC.
>
> When looking closer at the code, alloc_contig_range currently has comments
> saying migratetype must be MIGRATE_MOVABLE or MIGRATE_CMA. However, this
> is not checked/enforced anywhere in the code (that I can see). The
> migratetype passed to alloc_contig_range() will be used to set the migrate
> type of all pageblocks in the range. If there is an error, one side effect
> is that some pageblocks may have their migrate type changed to migratetype.
> Depending on how far we got before hitting the error, the number of pageblocks
> changed is unknown. This actually can happen at the lower level routine
> start_isolate_page_range().
>
> My first thought was to make start_isolate_page_range/set_migratetype_isolate
> check that the migrate type of a pageblock was migratetype before isolating.
> This would work for CMA, and I could make it work for the new allocator.
> However, offline_pages also calls start_isolate_page_range and I believe we
> do not want to enforce such a rule (all pageblocks must be of the same migrate
> type) for memory hotplug/offline?
>
> Should we be concerned at all about this potential changing of migrate type
> on error? The only way I can think to avoid this is to save the original
> migrate type before isolation.
This is more a question to Vlastimil, Joonsoo. But my understanding is
that it doesn't matter. MIGRATE_MOVABLE will not block other
allocations. So we seem to need it only for MIGRATE_CMA. The later
should die sooner or later hopefully so this awful kludge should just
die with it.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v5 11/28] fpga: dfl: add dfl_fpga_check_port_id function.
From: Alan Tull @ 2018-06-05 21:25 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z
In-Reply-To: <1525229431-3087-12-git-send-email-hao.wu@intel.com>
On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
> This patch adds one common function in DFL framework. It uses
> port_ops get_id callback to get port id and compare it with given
> value. This function could be used as match function of the
> dfl_fpga_cdev_find_port function.
>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
^ permalink raw reply
* Re: [PATCH v5 10/28] fpga: dfl: add dfl_fpga_port_ops support.
From: Alan Tull @ 2018-06-05 21:24 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z
In-Reply-To: <1525229431-3087-11-git-send-email-hao.wu@intel.com>
On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
> In some cases, other DFL driver modules may need to access some port
> operations, e.g disable / enable port for partial reconfiguration in
> FME module. In order to avoid dependency between port and FME modules,
> this patch introduces the dfl_fpga_port_ops support in DFL framework.
> A global dfl_fpga_port_ops list is added in the DFL framework, and
> it allows other DFL modules to use these port operations registered
> to this list, even in virtualization case, the port platform device
> is turned into VF / guest VM and hidden in host, the registered
> port_ops is still usable. It resolves the dependency issues between
> modules, but once get port ops API returns a valid port ops, that
> means related port driver module has been module_get to prevent from
> unexpected unload, and put port ops API must be invoked after use.
>
> These APIs introduced by this patch is listed below:
> * dfl_fpga_add_port_ops
> add one port ops to the global list.
>
> * dfl_fpga_del_port_ops
> del one port ops from the global list.
>
> * dfl_fpga_get_port_ops / dfl_fpga_put_port_ops
> get/put the port ops before/after use.
>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
> ---
> drivers/fpga/dfl.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/fpga/dfl.h | 21 +++++++++++++++++
> 2 files changed, 88 insertions(+)
>
^ permalink raw reply
* Re: [PATCH v5 09/28] fpga: dfl: add feature device infrastructure
From: Alan Tull @ 2018-06-05 21:14 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Xiao Guangrong, Tim Whisonant, Enno Luebbers,
Shiva Rao, Christopher Rauer
In-Reply-To: <1525229431-3087-10-git-send-email-hao.wu@intel.com>
On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
Some minor things, otherwise looks fine.
> From: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>
> This patch abstracts the common operations of the sub features, and defines
> the feature_ops data structure, including init, uinit and ioctl function
> pointers. And this patch adds some common helper functions for FME and AFU
> drivers, e.g dfl_feature_dev_use_begin/end which are used to ensure
> exclusive usage of the feature device file.
>
> 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: Kang Luwei <luwei.kang@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>
Acked-by: Alan Tull <atull@kernel.org>
> ---
> v2: rebased
> v3: use const for feature_ops.
> replace pci related function.
> v4: rebase and add more comments in code.
> v5: remove useless WARN_ON().
> reorder declarations in functions per suggestion from Moritz.
> add "dfl_" prefix to functions and data structure.
> ---
> drivers/fpga/dfl.c | 57 +++++++++++++++++++++++++++++++++++++
> drivers/fpga/dfl.h | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 138 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 1e06efb..c4c47d6 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -74,6 +74,63 @@ static enum dfl_id_type feature_dev_id_type(struct platform_device *pdev)
> return DFL_ID_MAX;
> }
>
> +void dfl_fpga_dev_feature_uinit(struct platform_device *pdev)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> + struct dfl_feature *feature;
> +
> + dfl_fpga_dev_for_each_feature(pdata, feature)
> + if (feature->ops) {
> + feature->ops->uinit(pdev, feature);
> + feature->ops = NULL;
> + }
> +}
> +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit);
Please add kernel-doc for functions that are exported to recommend and
guide their usage.
> +
> +static int dfl_feature_instance_init(struct platform_device *pdev,
> + struct dfl_feature_platform_data *pdata,
> + struct dfl_feature *feature,
> + struct dfl_feature_driver *drv)
> +{
> + int ret;
> +
> + ret = drv->ops->init(pdev, feature);
> + if (ret)
> + return ret;
> +
> + feature->ops = drv->ops;
> +
> + return ret;
> +}
> +
> +int dfl_fpga_dev_feature_init(struct platform_device *pdev,
> + struct dfl_feature_driver *feature_drvs)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> + struct dfl_feature_driver *drv = feature_drvs;
> + struct dfl_feature *feature;
> + int ret;
> +
> + while (drv->ops) {
> + dfl_fpga_dev_for_each_feature(pdata, feature) {
> + /* match feature and drv using id */
> + if (feature->id == drv->id) {
> + ret = dfl_feature_instance_init(pdev, pdata,
> + feature, drv);
> + if (ret)
> + goto exit;
> + }
> + }
> + drv++;
> + }
> +
> + return 0;
> +exit:
> + dfl_fpga_dev_feature_uinit(pdev);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init);
> +
> struct dfl_chardev_info {
> const char *name;
> dev_t devt;
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 2b6aaef..27f7a74 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -132,6 +132,17 @@
> #define PORT_CTRL_SFTRST_ACK BIT_ULL(4) /* HW ack for reset */
>
> /**
> + * struct dfl_feature_driver - sub feature's driver
> + *
> + * @id: sub feature id.
> + * @ops: ops of this sub feature.
> + */
> +struct dfl_feature_driver {
> + u64 id;
> + const struct dfl_feature_ops *ops;
> +};
> +
> +/**
> * struct dfl_feature - sub feature of the feature devices
> *
> * @id: sub feature id.
> @@ -139,13 +150,17 @@
> * this index is used to find its mmio resource from the
> * feature dev (platform device)'s reources.
> * @ioaddr: mapped mmio resource address.
> + * @ops: ops of this sub feature.
> */
> struct dfl_feature {
> u64 id;
> int resource_index;
> void __iomem *ioaddr;
> + const struct dfl_feature_ops *ops;
> };
>
> +#define DEV_STATUS_IN_USE 0
> +
> /**
> * struct dfl_feature_platform_data - platform data for feature devices
> *
> @@ -156,6 +171,8 @@ struct dfl_feature {
> * @dfl_cdev: ptr to container device.
> * @disable_count: count for port disable.
> * @num: number for sub features.
> + * @dev_status: dev status (e.g DEV_STATUS_IN_USE).
> + * @private: ptr to feature dev private data.
> * @features: sub features of this feature dev.
> */
> struct dfl_feature_platform_data {
> @@ -165,11 +182,49 @@ struct dfl_feature_platform_data {
> struct platform_device *dev;
> struct dfl_fpga_cdev *dfl_cdev;
> unsigned int disable_count;
> -
> + unsigned long dev_status;
> + void *private;
> int num;
> struct dfl_feature features[0];
> };
>
> +static inline
> +int dfl_feature_dev_use_begin(struct dfl_feature_platform_data *pdata)
> +{
> + /* Test and set IN_USE flags to ensure file is exclusively used */
> + if (test_and_set_bit_lock(DEV_STATUS_IN_USE, &pdata->dev_status))
> + return -EBUSY;
> +
> + return 0;
> +}
> +
> +static inline
> +void dfl_feature_dev_use_end(struct dfl_feature_platform_data *pdata)
> +{
> + clear_bit_unlock(DEV_STATUS_IN_USE, &pdata->dev_status);
> +}
> +
> +static inline
> +void dfl_fpga_pdata_set_private(struct dfl_feature_platform_data *pdata,
> + void *private)
> +{
> + pdata->private = private;
> +}
> +
> +static inline
> +void *dfl_fpga_pdata_get_private(struct dfl_feature_platform_data *pdata)
> +{
> + return pdata->private;
> +}
> +
> +struct dfl_feature_ops {
> + int (*init)(struct platform_device *pdev, struct dfl_feature *feature);
> + void (*uinit)(struct platform_device *pdev,
> + struct dfl_feature *feature);
> + long (*ioctl)(struct platform_device *pdev, struct dfl_feature *feature,
> + unsigned int cmd, unsigned long arg);
> +};
> +
> #define DFL_FPGA_FEATURE_DEV_FME "dfl-fme"
> #define DFL_FPGA_FEATURE_DEV_PORT "dfl-port"
Please move these to the same place as other things that will need to
be added to as feature devices are added as noted in the other reviews
today.
>
> @@ -179,6 +234,10 @@ static inline int dfl_feature_platform_data_size(const int num)
> num * sizeof(struct dfl_feature);
> }
>
> +void dfl_fpga_dev_feature_uinit(struct platform_device *pdev);
> +int dfl_fpga_dev_feature_init(struct platform_device *pdev,
> + struct dfl_feature_driver *feature_drvs);
> +
> enum dfl_fpga_devt_type {
> DFL_FPGA_DEVT_FME,
> DFL_FPGA_DEVT_PORT,
> @@ -190,6 +249,16 @@ int dfl_fpga_register_dev_ops(struct platform_device *pdev,
> struct module *owner);
> void dfl_fpga_unregister_dev_ops(struct platform_device *pdev);
>
> +static inline
> +struct platform_device *dfl_fpga_inode_to_feature_dev(struct inode *inode)
> +{
> + struct dfl_feature_platform_data *pdata;
> +
> + pdata = container_of(inode->i_cdev, struct dfl_feature_platform_data,
> + cdev);
> + return pdata->dev;
> +}
> +
> #define dfl_fpga_dev_for_each_feature(pdata, feature) \
> for ((feature) = (pdata)->features; \
> (feature) < (pdata)->features + (pdata)->num; (feature)++)
> @@ -219,6 +288,17 @@ void __iomem *dfl_get_feature_ioaddr_by_id(struct device *dev, u64 id)
> return NULL;
> }
>
> +static inline bool is_dfl_feature_present(struct device *dev, u64 id)
> +{
> + return !!dfl_get_feature_ioaddr_by_id(dev, id);
> +}
> +
> +static inline
> +struct device *dfl_fpga_pdata_to_parent(struct dfl_feature_platform_data *pdata)
> +{
> + return pdata->dev->dev.parent->parent;
> +}
> +
> static inline bool dfl_feature_is_fme(void __iomem *base)
> {
> u64 v = readq(base + DFH);
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH v5 07/28] fpga: dfl: add chardev support for feature devices
From: Alan Tull @ 2018-06-05 20:21 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <1525229431-3087-8-git-send-email-hao.wu@intel.com>
On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
> For feature devices drivers, both the FPGA Management Engine (FME) and
> Accelerated Function Unit (AFU) driver need to expose user interfaces via
> the device file, for example, mmap and ioctls.
>
> This patch adds chardev support in the dfl driver for feature devices,
> FME and AFU. It reserves the chardev regions for FME and AFU, and provide
> interfaces for FME and AFU driver to register their device file operations.
>
> 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>
> ---
> v2: rebased
> v3: move chardev support to fpga-dfl framework
> v4: rebase, and add more comments in code.
> v5: rebase, and add dfl_ prefix to APIs and data structures.
> ---
> drivers/fpga/dfl.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> drivers/fpga/dfl.h | 14 ++++++++
> 2 files changed, 117 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index c1462e9..18aba02 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -74,6 +74,96 @@ static enum dfl_id_type feature_dev_id_type(struct platform_device *pdev)
> return DFL_ID_MAX;
> }
>
> +struct dfl_chardev_info {
> + const char *name;
> + dev_t devt;
> +};
> +
> +/* indexed by enum dfl_fpga_devt_type */
> +struct dfl_chardev_info dfl_chrdevs[] = {
> + {.name = DFL_FPGA_FEATURE_DEV_FME}, /* DFL_FPGA_DEVT_FME */
> + {.name = DFL_FPGA_FEATURE_DEV_PORT}, /* DFL_FPGA_DEVT_AFU */
> +};
If this were added in the initial dfl.c patch, it could be used by
build_info_create_dev to get the name.
> +
> +static void dfl_chardev_uinit(void)
> +{
> + int i;
> +
> + for (i = 0; i < DFL_FPGA_DEVT_MAX; i++)
> + if (MAJOR(dfl_chrdevs[i].devt)) {
> + unregister_chrdev_region(dfl_chrdevs[i].devt,
> + MINORMASK);
> + dfl_chrdevs[i].devt = MKDEV(0, 0);
> + }
> +}
> +
> +static int dfl_chardev_init(void)
> +{
> + int i, ret;
> +
> + for (i = 0; i < DFL_FPGA_DEVT_MAX; i++) {
> + ret = alloc_chrdev_region(&dfl_chrdevs[i].devt, 0, MINORMASK,
> + dfl_chrdevs[i].name);
> + if (ret)
> + goto exit;
> + }
> +
> + return 0;
> +
> +exit:
> + dfl_chardev_uinit();
> + return ret;
> +}
> +
> +static dev_t dfl_get_devt(enum dfl_fpga_devt_type type, int id)
> +{
> + WARN_ON(type >= DFL_FPGA_DEVT_MAX);
> +
> + return MKDEV(MAJOR(dfl_chrdevs[type].devt), id);
> +}
> +
> +/**
> + * dfl_fpga_register_dev_ops - register cdev ops for feature dev
> + *
> + * @pdev: feature dev.
> + * @fops: file operations for feature dev's cdev.
> + * @owner: owning module/driver.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +int dfl_fpga_register_dev_ops(struct platform_device *pdev,
> + const struct file_operations *fops,
> + struct module *owner)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +
> + cdev_init(&pdata->cdev, fops);
> + pdata->cdev.owner = owner;
> +
> + /*
> + * set parent to the feature device so that its refcount is
> + * decreased after the last refcount of cdev is gone, that
> + * makes sure the feature device is valid during device
> + * file's life-cycle.
> + */
> + pdata->cdev.kobj.parent = &pdev->dev.kobj;
> +
> + return cdev_add(&pdata->cdev, pdev->dev.devt, 1);
> +}
> +EXPORT_SYMBOL_GPL(dfl_fpga_register_dev_ops);
> +
> +/**
> + * dfl_fpga_unregister_dev_ops - unregister cdev ops for feature dev
> + * @pdev: feature dev.
> + */
> +void dfl_fpga_unregister_dev_ops(struct platform_device *pdev)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +
> + cdev_del(&pdata->cdev);
> +}
> +EXPORT_SYMBOL_GPL(dfl_fpga_unregister_dev_ops);
> +
> /**
> * struct build_feature_devs_info - info collected during feature dev build.
> *
> @@ -208,9 +298,13 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> enum dfl_id_type type, const char *name,
> void __iomem *ioaddr)
> {
> + enum dfl_fpga_devt_type devt_type = DFL_FPGA_DEVT_FME;
> struct platform_device *fdev;
> int ret;
>
> + if (type == PORT_ID)
> + devt_type = DFL_FPGA_DEVT_PORT;
> +
> /* we will create a new device, commit current device first */
> ret = build_info_commit_dev(binfo);
> if (ret)
> @@ -234,6 +328,7 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> return fdev->id;
>
> fdev->dev.parent = &binfo->cdev->region->dev;
> + fdev->dev.devt = dfl_get_devt(devt_type, fdev->id);
>
> return 0;
> }
> @@ -702,13 +797,20 @@ void dfl_fpga_remove_feature_devs(struct dfl_fpga_cdev *cdev)
>
> static int __init dfl_fpga_init(void)
> {
> + int ret;
> +
> dfl_ids_init();
>
> - return 0;
> + ret = dfl_chardev_init();
> + if (ret)
> + dfl_ids_destroy();
> +
> + return ret;
> }
>
> static void __exit dfl_fpga_exit(void)
> {
> + dfl_chardev_uinit();
> dfl_ids_destroy();
> }
>
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 2ede915..5fcb1a1 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -15,6 +15,7 @@
> #define __FPGA_DFL_H
>
> #include <linux/bitfield.h>
> +#include <linux/cdev.h>
> #include <linux/delay.h>
> #include <linux/fs.h>
> #include <linux/iopoll.h>
> @@ -150,6 +151,7 @@ struct dfl_feature {
> *
> * @node: node to link feature devs to container device's port_dev_list.
> * @lock: mutex to protect platform data.
> + * @cdev: cdev of feature dev.
> * @dev: ptr to platform device linked with this platform data.
> * @dfl_cdev: ptr to container device.
> * @disable_count: count for port disable.
> @@ -159,6 +161,7 @@ struct dfl_feature {
> struct dfl_feature_platform_data {
> struct list_head node;
> struct mutex lock;
> + struct cdev cdev;
> struct platform_device *dev;
> struct dfl_fpga_cdev *dfl_cdev;
> unsigned int disable_count;
> @@ -176,6 +179,17 @@ static inline int dfl_feature_platform_data_size(const int num)
> num * sizeof(struct dfl_feature);
> }
>
> +enum dfl_fpga_devt_type {
> + DFL_FPGA_DEVT_FME,
> + DFL_FPGA_DEVT_PORT,
> + DFL_FPGA_DEVT_MAX,
> +};
Could you move this enum to be close to other similar enums (like
dfl_id_type)? The dfl code has a few enums that are similar, and may
need updating (or not) as feature are added. Putting them close
together with appropriate comments would be helpful to keep them all
straight.
> +
> +int dfl_fpga_register_dev_ops(struct platform_device *pdev,
> + const struct file_operations *fops,
> + struct module *owner);
> +void dfl_fpga_unregister_dev_ops(struct platform_device *pdev);
> +
> #define dfl_fpga_dev_for_each_feature(pdata, feature) \
> for ((feature) = (pdata)->features; \
> (feature) < (pdata)->features + (pdata)->num; (feature)++)
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH v5 06/28] fpga: add device feature list support
From: Alan Tull @ 2018-06-05 20:21 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <1525229431-3087-7-git-send-email-hao.wu@intel.com>
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.
> 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?
>
> 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
> + *
> + * 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.
> +
> +/* 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.
> +
> +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).
> +
> + 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
> +
> + /*
> + * 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.
> + 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.
> +
> + if (ret)
> + return ret;
> +
> + /* Find and parse FIU's child AFU via its NEXT_AFU register */
> + v = readq(dfl->ioaddr + ofst + NEXT_AFU);
> +
> + offset = FIELD_GET(NEXT_AFU_NEXT_DFH_OFST, v);
> + if (offset)
> + return parse_feature_afu(binfo, dfl, ofst + offset);
> +
> + dev_dbg(binfo->dev, "No AFUs detected on FIU %d\n", id);
> +
> + return ret;
> +}
> +
> +static int parse_feature_private(struct build_feature_devs_info *binfo,
> + struct dfl_fpga_enum_dfl *dfl,
> + resource_size_t ofst)
> +{
> + if (!binfo->feature_dev) {
> + dev_err(binfo->dev, "the private feature %llx does not belong to any AFU.\n",
> + (unsigned long long)feature_id(dfl->ioaddr + ofst));
> + return -EINVAL;
> + }
> +
> + return create_feature_instance(binfo, dfl, ofst, 0, 0);
> +}
> +
> +/**
> + * parse_feature - parse a feature on given device feature list
> + *
> + * @binfo: build feature devices information.
> + * @dfl: device feature list to parse
> + * @ofst: offset to feature header on this device feature list
> + */
> +static int parse_feature(struct build_feature_devs_info *binfo,
> + struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst)
> +{
> + u64 v;
> + u32 type;
> +
> + v = readq(dfl->ioaddr + ofst + DFH);
> + type = FIELD_GET(DFH_TYPE, v);
> +
> + switch (type) {
> + case DFH_TYPE_AFU:
> + return parse_feature_afu(binfo, dfl, ofst);
> + case DFH_TYPE_PRIVATE:
> + return parse_feature_private(binfo, dfl, ofst);
> + case DFH_TYPE_FIU:
> + return parse_feature_fiu(binfo, dfl, ofst);
> + default:
> + dev_info(binfo->dev,
> + "Feature Type %x is not supported.\n", type);
> + }
> +
> + return 0;
> +}
> +
> +static int parse_feature_list(struct build_feature_devs_info *binfo,
> + struct dfl_fpga_enum_dfl *dfl)
> +{
> + void __iomem *start = dfl->ioaddr;
> + void __iomem *end = dfl->ioaddr + dfl->len;
> + int ret = 0;
> + u32 ofst = 0;
> + u64 v;
> +
> + /* walk through the device feature list via DFH's next DFH pointer. */
> + for (; start < end; start += ofst) {
> + if (end - start < DFH_SIZE) {
> + dev_err(binfo->dev, "The region is too small to contain a feature.\n");
> + return -EINVAL;
> + }
> +
> + ret = parse_feature(binfo, dfl, start - dfl->ioaddr);
> + if (ret)
> + return ret;
> +
> + v = readq(start + DFH);
> + ofst = FIELD_GET(DFH_NEXT_HDR_OFST, v);
> +
> + /* stop parsing if EOL(End of List) is set or offset is 0 */
> + if ((v & DFH_EOL) || !ofst)
> + break;
> + }
> +
> + /* commit current feature device when reach the end of list */
> + return build_info_commit_dev(binfo);
> +}
> +
> +struct dfl_fpga_enum_info *dfl_fpga_enum_info_alloc(struct device *dev)
> +{
> + struct dfl_fpga_enum_info *info;
> +
> + get_device(dev);
> +
> + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> + if (!info) {
> + put_device(dev);
> + return NULL;
> + }
> +
> + info->dev = dev;
> + INIT_LIST_HEAD(&info->dfls);
> +
> + return info;
> +}
> +EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_alloc);
> +
> +void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info)
> +{
> + struct dfl_fpga_enum_dfl *tmp, *dfl;
> + struct device *dev;
> +
> + if (!info)
> + return;
> +
> + dev = info->dev;
> +
> + /* remove all device feature lists in the list. */
> + list_for_each_entry_safe(dfl, tmp, &info->dfls, node) {
> + list_del(&dfl->node);
> + devm_kfree(dev, dfl);
> + }
> +
> + devm_kfree(dev, info);
> + put_device(dev);
> +}
> +EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_free);
> +
> +/**
> + * dfl_fpga_enum_info_add_dfl - add info of a device feature list to enum info
> + *
> + * @info: ptr to dfl_fpga_enum_info
> + * @start: mmio resource address of the device feature list.
> + * @len: mmio resource length of the device feature list.
> + * @ioaddr: mapped mmio resource address of the device feature list.
> + *
> + * One FPGA device may have 1 or more Device Feature Lists (DFLs), use this
> + * function to add information of each DFL to common data structure for next
> + * step enumeration.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
> + resource_size_t start, resource_size_t len,
> + void __iomem *ioaddr)
> +{
> + struct dfl_fpga_enum_dfl *dfl;
> +
> + dfl = devm_kzalloc(info->dev, sizeof(*dfl), GFP_KERNEL);
> + if (!dfl)
> + return -ENOMEM;
> +
> + dfl->start = start;
> + dfl->len = len;
> + dfl->ioaddr = ioaddr;
> +
> + list_add_tail(&dfl->node, &info->dfls);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_add_dfl);
> +
> +static int remove_feature_dev(struct device *dev, void *data)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + enum dfl_id_type type = feature_dev_id_type(pdev);
> + int id = pdev->id;
> +
> + platform_device_unregister(pdev);
> +
> + free_dfl_id(type, id);
> +
> + return 0;
> +}
> +
> +static void remove_feature_devs(struct dfl_fpga_cdev *cdev)
> +{
> + device_for_each_child(&cdev->region->dev, NULL, remove_feature_dev);
> +}
> +
> +/**
> + * dfl_fpga_enumerate_feature_devs - enumerate feature devices
> + * @info: information for enumeration.
> + *
> + * This function creates a container device (base FPGA region), enumerates
> + * feature devices based on the enumeration info and creates platform devices
> + * under the container device.
> + *
> + * Return: dfl_fpga_cdev struct on success, -errno on failure
> + */
> +struct dfl_fpga_cdev *
> +dfl_fpga_enumerate_feature_devs(struct dfl_fpga_enum_info *info)
> +{
> + struct build_feature_devs_info *binfo;
> + struct dfl_fpga_enum_dfl *dfl;
> + struct dfl_fpga_cdev *cdev;
> + int ret = 0;
> +
> + if (!info->dev)
> + return ERR_PTR(-ENODEV);
> +
> + cdev = devm_kzalloc(info->dev, sizeof(*cdev), GFP_KERNEL);
> + if (!cdev)
> + return ERR_PTR(-ENOMEM);
> +
> + cdev->region = fpga_region_create(info->dev, NULL, NULL);
> + if (!cdev->region) {
> + ret = -ENOMEM;
> + goto free_region_exit;
> + }
> +
> + cdev->parent = info->dev;
> + mutex_init(&cdev->lock);
> + INIT_LIST_HEAD(&cdev->port_dev_list);
> +
> + ret = fpga_region_register(cdev->region);
> + if (ret)
> + goto free_cdev_exit;
> +
> + /* create and init build info for enumeration */
> + binfo = devm_kzalloc(info->dev, sizeof(*binfo), GFP_KERNEL);
> + if (!binfo) {
> + ret = -ENOMEM;
> + goto unregister_region_exit;
> + }
> +
> + binfo->dev = info->dev;
> + binfo->cdev = cdev;
> +
> + /*
> + * start enumeration for all feature devices based on Device Feature
> + * Lists.
> + */
> + list_for_each_entry(dfl, &info->dfls, node) {
> + ret = parse_feature_list(binfo, dfl);
> + if (ret) {
> + remove_feature_devs(cdev);
> + build_info_free(binfo);
> + goto unregister_region_exit;
> + }
> + }
> +
> + build_info_free(binfo);
> +
> + return cdev;
> +
> +unregister_region_exit:
> + fpga_region_unregister(cdev->region);
> +free_region_exit:
> + fpga_region_free(cdev->region);
> +free_cdev_exit:
> + devm_kfree(info->dev, cdev);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(dfl_fpga_enumerate_feature_devs);
> +
> +/**
> + * dfl_fpga_remove_feature_devs - remove all feature devices
> + * @cdev: fpga container device.
> + *
> + * Remove the container device and all feature devices under given container
> + * devices.
> + */
> +void dfl_fpga_remove_feature_devs(struct dfl_fpga_cdev *cdev)
> +{
> + struct dfl_feature_platform_data *pdata, *ptmp;
> +
> + remove_feature_devs(cdev);
> +
> + mutex_lock(&cdev->lock);
> + if (cdev->fme_dev) {
> + /* the fme should be unregistered. */
> + WARN_ON(device_is_registered(cdev->fme_dev));
> + put_device(cdev->fme_dev);
> + }
> +
> + list_for_each_entry_safe(pdata, ptmp, &cdev->port_dev_list, node) {
> + struct platform_device *port_dev = pdata->dev;
> +
> + /* the port should be unregistered. */
> + WARN_ON(device_is_registered(&port_dev->dev));
> + list_del(&pdata->node);
> + put_device(&port_dev->dev);
> + }
> + mutex_unlock(&cdev->lock);
> +
> + fpga_region_unregister(cdev->region);
> + devm_kfree(cdev->parent, cdev);
> +}
> +EXPORT_SYMBOL_GPL(dfl_fpga_remove_feature_devs);
> +
> +static int __init dfl_fpga_init(void)
> +{
> + dfl_ids_init();
> +
> + return 0;
> +}
> +
> +static void __exit dfl_fpga_exit(void)
> +{
> + dfl_ids_destroy();
> +}
> +
> +module_init(dfl_fpga_init);
> +module_exit(dfl_fpga_exit);
> +
> +MODULE_DESCRIPTION("FPGA Device Feature List (DFL) Support");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> new file mode 100644
> index 0000000..2ede915
> --- /dev/null
> +++ b/drivers/fpga/dfl.h
> @@ -0,0 +1,279 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Driver Header File for FPGA Device Feature List (DFL) Support
> + *
> + * Copyright (C) 2017 Intel Corporation, Inc.
> + *
> + * 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>
> + */
> +
> +#ifndef __FPGA_DFL_H
> +#define __FPGA_DFL_H
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/iopoll.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/uuid.h>
> +#include <linux/fpga/fpga-region.h>
> +
> +/* maximum supported number of ports */
> +#define MAX_DFL_FPGA_PORT_NUM 4
> +/* plus one for fme device */
> +#define MAX_DFL_FEATURE_DEV_NUM (MAX_DFL_FPGA_PORT_NUM + 1)
> +
> +/* Reserved 0x0 for Header Group Register and 0xff for AFU */
> +#define FEATURE_ID_FIU_HEADER 0x0
> +#define FEATURE_ID_AFU 0xff
> +
> +#define FME_FEATURE_ID_HEADER FEATURE_ID_FIU_HEADER
> +#define FME_FEATURE_ID_THERMAL_MGMT 0x1
> +#define FME_FEATURE_ID_POWER_MGMT 0x2
> +#define FME_FEATURE_ID_GLOBAL_IPERF 0x3
> +#define FME_FEATURE_ID_GLOBAL_ERR 0x4
> +#define FME_FEATURE_ID_PR_MGMT 0x5
> +#define FME_FEATURE_ID_HSSI 0x6
> +#define FME_FEATURE_ID_GLOBAL_DPERF 0x7
> +
> +#define PORT_FEATURE_ID_HEADER FEATURE_ID_FIU_HEADER
> +#define PORT_FEATURE_ID_AFU FEATURE_ID_AFU
> +#define PORT_FEATURE_ID_ERROR 0x10
> +#define PORT_FEATURE_ID_UMSG 0x11
> +#define PORT_FEATURE_ID_UINT 0x12
> +#define PORT_FEATURE_ID_STP 0x13
> +
> +/*
> + * Device Feature Header Register Set
> + *
> + * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
> + * For AFUs, they have DFH + GUID as common header registers.
> + * For private features, they only have DFH register as common header.
> + */
> +#define DFH 0x0
> +#define GUID_L 0x8
> +#define GUID_H 0x10
> +#define NEXT_AFU 0x18
> +
> +#define DFH_SIZE 0x8
> +
> +/* Device Feature Header Register Bitfield */
> +#define DFH_ID GENMASK_ULL(11, 0) /* Feature ID */
> +#define DFH_ID_FIU_FME 0
> +#define DFH_ID_FIU_PORT 1
> +#define DFH_REVISION GENMASK_ULL(15, 12) /* Feature revision */
> +#define DFH_NEXT_HDR_OFST GENMASK_ULL(39, 16) /* Offset to next DFH */
> +#define DFH_EOL BIT_ULL(40) /* End of list */
> +#define DFH_TYPE GENMASK_ULL(63, 60) /* Feature type */
> +#define DFH_TYPE_AFU 1
> +#define DFH_TYPE_PRIVATE 3
> +#define DFH_TYPE_FIU 4
> +
> +/* Next AFU Register Bitfield */
> +#define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset to next AFU */
> +
> +/* FME Header Register Set */
> +#define FME_HDR_DFH DFH
> +#define FME_HDR_GUID_L GUID_L
> +#define FME_HDR_GUID_H GUID_H
> +#define FME_HDR_NEXT_AFU NEXT_AFU
> +#define FME_HDR_CAP 0x30
> +#define FME_HDR_PORT_OFST(n) (0x38 + ((n) * 0x8))
> +#define FME_HDR_BITSTREAM_ID 0x60
> +#define FME_HDR_BITSTREAM_MD 0x68
> +
> +/* FME Fab Capability Register Bitfield */
> +#define FME_CAP_FABRIC_VERID GENMASK_ULL(7, 0) /* Fabric version ID */
> +#define FME_CAP_SOCKET_ID BIT_ULL(8) /* Socket ID */
> +#define FME_CAP_PCIE0_LINK_AVL BIT_ULL(12) /* PCIE0 Link */
> +#define FME_CAP_PCIE1_LINK_AVL BIT_ULL(13) /* PCIE1 Link */
> +#define FME_CAP_COHR_LINK_AVL BIT_ULL(14) /* Coherent Link */
> +#define FME_CAP_IOMMU_AVL BIT_ULL(16) /* IOMMU available */
> +#define FME_CAP_NUM_PORTS GENMASK_ULL(19, 17) /* Number of ports */
> +#define FME_CAP_ADDR_WIDTH GENMASK_ULL(29, 24) /* Address bus width */
> +#define FME_CAP_CACHE_SIZE GENMASK_ULL(43, 32) /* cache size in KB */
> +#define FME_CAP_CACHE_ASSOC GENMASK_ULL(47, 44) /* Associativity */
> +
> +/* FME Port Offset Register Bitfield */
> +/* Offset to port device feature header */
> +#define FME_PORT_OFST_DFH_OFST GENMASK_ULL(23, 0)
> +/* PCI Bar ID for this port */
> +#define FME_PORT_OFST_BAR_ID GENMASK_ULL(34, 32)
> +/* AFU MMIO access permission. 1 - VF, 0 - PF. */
> +#define FME_PORT_OFST_ACC_CTRL BIT_ULL(55)
> +#define FME_PORT_OFST_ACC_PF 0
> +#define FME_PORT_OFST_ACC_VF 1
> +#define FME_PORT_OFST_IMP BIT_ULL(60)
> +
> +/* PORT Header Register Set */
> +#define PORT_HDR_DFH DFH
> +#define PORT_HDR_GUID_L GUID_L
> +#define PORT_HDR_GUID_H GUID_H
> +#define PORT_HDR_NEXT_AFU NEXT_AFU
> +#define PORT_HDR_CAP 0x30
> +#define PORT_HDR_CTRL 0x38
> +
> +/* Port Capability Register Bitfield */
> +#define PORT_CAP_PORT_NUM GENMASK_ULL(1, 0) /* ID of this port */
> +#define PORT_CAP_MMIO_SIZE GENMASK_ULL(23, 8) /* MMIO size in KB */
> +#define PORT_CAP_SUPP_INT_NUM GENMASK_ULL(35, 32) /* Interrupts num */
> +
> +/* Port Control Register Bitfield */
> +#define PORT_CTRL_SFTRST BIT_ULL(0) /* Port soft reset */
> +/* Latency tolerance reporting. '1' >= 40us, '0' < 40us.*/
> +#define PORT_CTRL_LATENCY BIT_ULL(2)
> +#define PORT_CTRL_SFTRST_ACK BIT_ULL(4) /* HW ack for reset */
> +
> +/**
> + * struct dfl_feature - sub feature of the feature devices
> + *
> + * @id: sub feature id.
> + * @resource_index: each sub feature has one mmio resource for its registers.
> + * this index is used to find its mmio resource from the
> + * feature dev (platform device)'s reources.
> + * @ioaddr: mapped mmio resource address.
> + */
> +struct dfl_feature {
> + u64 id;
> + int resource_index;
> + void __iomem *ioaddr;
> +};
> +
> +/**
> + * struct dfl_feature_platform_data - platform data for feature devices
> + *
> + * @node: node to link feature devs to container device's port_dev_list.
> + * @lock: mutex to protect platform data.
> + * @dev: ptr to platform device linked with this platform data.
> + * @dfl_cdev: ptr to container device.
> + * @disable_count: count for port disable.
> + * @num: number for sub features.
> + * @features: sub features of this feature dev.
> + */
> +struct dfl_feature_platform_data {
> + struct list_head node;
> + struct mutex lock;
> + struct platform_device *dev;
> + struct dfl_fpga_cdev *dfl_cdev;
> + unsigned int disable_count;
> +
> + int num;
> + struct dfl_feature features[0];
> +};
> +
> +#define DFL_FPGA_FEATURE_DEV_FME "dfl-fme"
> +#define DFL_FPGA_FEATURE_DEV_PORT "dfl-port"
> +
> +static inline int dfl_feature_platform_data_size(const int num)
> +{
> + return sizeof(struct dfl_feature_platform_data) +
> + num * sizeof(struct dfl_feature);
> +}
> +
> +#define dfl_fpga_dev_for_each_feature(pdata, feature) \
> + for ((feature) = (pdata)->features; \
> + (feature) < (pdata)->features + (pdata)->num; (feature)++)
> +
> +static inline
> +struct dfl_feature *dfl_get_feature_by_id(struct device *dev, u64 id)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> + struct dfl_feature *feature;
> +
> + dfl_fpga_dev_for_each_feature(pdata, feature)
> + if (feature->id == id)
> + return feature;
> +
> + return NULL;
> +}
> +
> +static inline
> +void __iomem *dfl_get_feature_ioaddr_by_id(struct device *dev, u64 id)
> +{
> + struct dfl_feature *feature = dfl_get_feature_by_id(dev, id);
> +
> + if (feature && feature->ioaddr)
> + return feature->ioaddr;
> +
> + WARN_ON(1);
> + return NULL;
> +}
> +
> +static inline bool dfl_feature_is_fme(void __iomem *base)
> +{
> + u64 v = readq(base + DFH);
> +
> + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
> +}
> +
> +static inline bool dfl_feature_is_port(void __iomem *base)
> +{
> + u64 v = readq(base + DFH);
> +
> + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
> +}
> +
> +/**
> + * struct dfl_fpga_enum_info - DFL FPGA enumeration information
> + *
> + * @dev: parent device.
> + * @dfls: list of device feature lists.
> + */
> +struct dfl_fpga_enum_info {
> + struct device *dev;
> + struct list_head dfls;
> +};
> +
> +/**
> + * struct dfl_fpga_enum_dfl - DFL FPGA enumeration device feature list info
> + *
> + * @start: base address of this device feature list.
> + * @len: size of this device feature list.
> + * @ioaddr: mapped base address of this device feature list.
> + * @node: node in list of device feature lists.
> + */
> +struct dfl_fpga_enum_dfl {
> + resource_size_t start;
> + resource_size_t len;
> +
> + void __iomem *ioaddr;
> +
> + struct list_head node;
> +};
> +
> +struct dfl_fpga_enum_info *dfl_fpga_enum_info_alloc(struct device *dev);
> +int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
> + resource_size_t start, resource_size_t len,
> + void __iomem *ioaddr);
> +void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info);
> +
> +/**
> + * struct dfl_fpga_cdev - container device of DFL based FPGA
> + *
> + * @parent: parent device of this container device.
> + * @region: base fpga region.
> + * @fme_dev: FME feature device under this container device.
> + * @lock: mutex lock to protect the port device list.
> + * @port_dev_list: list of all port feature devices under this container device.
> + */
> +struct dfl_fpga_cdev {
> + struct device *parent;
> + struct fpga_region *region;
> + struct device *fme_dev;
> + struct mutex lock;
> + struct list_head port_dev_list;
> +};
> +
> +struct dfl_fpga_cdev *
> +dfl_fpga_enumerate_feature_devs(struct dfl_fpga_enum_info *info);
> +void dfl_fpga_remove_feature_devs(struct dfl_fpga_cdev *cdev);
> +
> +#endif /* __FPGA_DFL_H */
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [RFC PATCH for 4.18 10/16] powerpc: Wire up restartable sequences system call
From: Mathieu Desnoyers @ 2018-06-05 12:51 UTC (permalink / raw)
To: Michael Ellerman
Cc: Peter Zijlstra, Paul E. McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, linux-kernel, linux-api, Paul Turner, Andrew Morton,
Russell King, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Andrew Hunter, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas <cata>
In-Reply-To: <874lihbykr.fsf@concordia.ellerman.id.au>
----- On Jun 5, 2018, at 1:18 AM, Michael Ellerman mpe@ellerman.id.au wrote:
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:
>
>> From: Boqun Feng <boqun.feng@gmail.com>
>>
>> Wire up the rseq system call on powerpc.
>>
>> This provides an ABI improving the speed of a user-space getcpu
>> operation on powerpc by skipping the getcpu system call on the fast
>> path, as well as improving the speed of user-space operations on per-cpu
>> data compared to using load-reservation/store-conditional atomics.
>>
>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> CC: Paul Mackerras <paulus@samba.org>
>> CC: Michael Ellerman <mpe@ellerman.id.au>
>> CC: Peter Zijlstra <peterz@infradead.org>
>> CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
>> CC: linuxppc-dev@lists.ozlabs.org
>> ---
>> arch/powerpc/include/asm/systbl.h | 1 +
>> arch/powerpc/include/asm/unistd.h | 2 +-
>> arch/powerpc/include/uapi/asm/unistd.h | 1 +
>> 3 files changed, 3 insertions(+), 1 deletion(-)
>
> Looks fine to me.
>
> I don't have any other new syscalls in my next, so this should not
> conflict with anything for 4.18.
>
> Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
Added your ack to the series, thanks!
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18 09/16] powerpc: Add syscall detection for restartable sequences
From: Mathieu Desnoyers @ 2018-06-05 12:50 UTC (permalink / raw)
To: Michael Ellerman
Cc: Peter Zijlstra, Paul E. McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, linux-kernel, linux-api, Paul Turner, Andrew Morton,
Russell King, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Andrew Hunter, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas <cata>
In-Reply-To: <871sdlbyg7.fsf@concordia.ellerman.id.au>
----- On Jun 5, 2018, at 1:21 AM, Michael Ellerman mpe@ellerman.id.au wrote:
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:
>> From: Boqun Feng <boqun.feng@gmail.com>
>>
>> Syscalls are not allowed inside restartable sequences, so add a call to
>> rseq_syscall() at the very beginning of system call exiting path for
>> CONFIG_DEBUG_RSEQ=y kernel. This could help us to detect whether there
>> is a syscall issued inside restartable sequences.
>>
>> [ Tested on 64-bit powerpc kernel by Mathieu Desnoyers. Still needs to
>> be tested on 32-bit powerpc kernel. ]
>>
>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> CC: Paul Mackerras <paulus@samba.org>
>> CC: Michael Ellerman <mpe@ellerman.id.au>
>> CC: Peter Zijlstra <peterz@infradead.org>
>> CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
>> CC: linuxppc-dev@lists.ozlabs.org
>> ---
>> arch/powerpc/kernel/entry_32.S | 7 +++++++
>> arch/powerpc/kernel/entry_64.S | 8 ++++++++
>> 2 files changed, 15 insertions(+)
>
> I don't _love_ the #ifdefs in here, but they look correct and there's
> not really a better option until we rewrite the syscall handler in C.
>
> The rseq selftests passed for me with this applied and enabled. So if
> you like here's some tags:
>
> Tested-by: Michael Ellerman <mpe@ellerman.id.au>
> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
>
Adding you ack to the series.
Thanks!
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18 09/16] powerpc: Add syscall detection for restartable sequences
From: Michael Ellerman @ 2018-06-05 5:21 UTC (permalink / raw)
To: Peter Zijlstra, Paul E . McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson
Cc: linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H . Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon,
Michael Kerrisk, Joel Fernandes, Mathieu Desnoyers,
Benjamin Herrenschmidt <benh>
In-Reply-To: <20180602124408.8430-10-mathieu.desnoyers@efficios.com>
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:
> From: Boqun Feng <boqun.feng@gmail.com>
>
> Syscalls are not allowed inside restartable sequences, so add a call to
> rseq_syscall() at the very beginning of system call exiting path for
> CONFIG_DEBUG_RSEQ=y kernel. This could help us to detect whether there
> is a syscall issued inside restartable sequences.
>
> [ Tested on 64-bit powerpc kernel by Mathieu Desnoyers. Still needs to
> be tested on 32-bit powerpc kernel. ]
>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: Paul Mackerras <paulus@samba.org>
> CC: Michael Ellerman <mpe@ellerman.id.au>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> CC: linuxppc-dev@lists.ozlabs.org
> ---
> arch/powerpc/kernel/entry_32.S | 7 +++++++
> arch/powerpc/kernel/entry_64.S | 8 ++++++++
> 2 files changed, 15 insertions(+)
I don't _love_ the #ifdefs in here, but they look correct and there's
not really a better option until we rewrite the syscall handler in C.
The rseq selftests passed for me with this applied and enabled. So if
you like here's some tags:
Tested-by: Michael Ellerman <mpe@ellerman.id.au>
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
cheers
^ permalink raw reply
* Re: [RFC PATCH for 4.18 10/16] powerpc: Wire up restartable sequences system call
From: Michael Ellerman @ 2018-06-05 5:18 UTC (permalink / raw)
To: Peter Zijlstra, Paul E . McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson
Cc: linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H . Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon,
Michael Kerrisk, Joel Fernandes, Mathieu Desnoyers,
Benjamin Herrenschmidt <benh>
In-Reply-To: <20180602124408.8430-11-mathieu.desnoyers@efficios.com>
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:
> From: Boqun Feng <boqun.feng@gmail.com>
>
> Wire up the rseq system call on powerpc.
>
> This provides an ABI improving the speed of a user-space getcpu
> operation on powerpc by skipping the getcpu system call on the fast
> path, as well as improving the speed of user-space operations on per-cpu
> data compared to using load-reservation/store-conditional atomics.
>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: Paul Mackerras <paulus@samba.org>
> CC: Michael Ellerman <mpe@ellerman.id.au>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> CC: linuxppc-dev@lists.ozlabs.org
> ---
> arch/powerpc/include/asm/systbl.h | 1 +
> arch/powerpc/include/asm/unistd.h | 2 +-
> arch/powerpc/include/uapi/asm/unistd.h | 1 +
> 3 files changed, 3 insertions(+), 1 deletion(-)
Looks fine to me.
I don't have any other new syscalls in my next, so this should not
conflict with anything for 4.18.
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
cheers
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
From: Richard Guy Briggs @ 2018-06-04 20:30 UTC (permalink / raw)
To: Steve Grubb
Cc: linux-api, containers, LKML, eparis, Linux-Audit Mailing List,
linux-fsdevel, viro
In-Reply-To: <20180604202301.2id5xpjqqotqq5j4@madcap2.tricolour.ca>
On 2018-06-04 16:23, Richard Guy Briggs wrote:
> On 2018-06-04 12:09, Steve Grubb wrote:
> > On Friday, June 1, 2018 5:04:55 PM EDT Richard Guy Briggs wrote:
> > > Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
> > >
> > > From: Richard Guy Briggs <rgb@redhat.com>
> > > To: Me
> > > CC: linux-api@vger.kernel.org, containers@lists.linux-foundation.org, LKML
> > > <linux-kernel@vger.kernel.org>, eparis@parisplace.org, ... Date: 6/1/18
> > > 5:04 PM
> > >
> > > On 2018-05-17 17:00, Steve Grubb wrote:
> > > > On Fri, 16 Mar 2018 05:00:28 -0400
> > > >
> > > > Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > Implement the proc fs write to set the audit container ID of a
> > > > > process, emitting an AUDIT_CONTAINER record to document the event.
> > > > >
> > > > > This is a write from the container orchestrator task to a proc entry
> > > > > of the form /proc/PID/containerid where PID is the process ID of the
> > > > > newly created task that is to become the first task in a container,
> > > > > or an additional task added to a container.
> > > > >
> > > > > The write expects up to a u64 value (unset: 18446744073709551615).
> > > > >
> > > > > This will produce a record such as this:
> > > > > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0
> > > > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0
> > > > > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455
> > > > > res=0
> > > >
> > > > The was one thing I was wondering about. Currently when we set the
> > > > loginuid, the record is AUDIT_LOGINUID. The corollary is that when we
> > > > set the container id, the event should be AUDIT_CONTAINERID or
> > > > AUDIT_CONTAINER_ID.
> > > >
> > > > During syscall events, the path info is returned in a a record simply
> > > > called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So, rather than
> > > > calling the record that gets attached to everything
> > > > AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.
> > > >
> > > > > The "op" field indicates an initial set. The "pid" to "ses" fields
> > > > > are the orchestrator while the "opid" field is the object's PID, the
> > > > > process being "contained". Old and new container ID values are given
> > > > > in the "contid" fields, while res indicates its success.
> > > > >
> > > > > It is not permitted to self-set, unset or re-set the container ID. A
> > > > > child inherits its parent's container ID, but then can be set only
> > > > > once after.
> > > > >
> > > > > See: https://github.com/linux-audit/audit-kernel/issues/32
> > > > >
> > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > ---
> > > > > fs/proc/base.c | 37 ++++++++++++++++++++
> > > > > include/linux/audit.h | 16 +++++++++
> > > > > include/linux/init_task.h | 4 ++-
> > > > > include/linux/sched.h | 1 +
> > > > > include/uapi/linux/audit.h | 2 ++
> > > > > kernel/auditsc.c | 84
> > > > > ++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 143
> > > > > insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > > > index 60316b5..6ce4fbe 100644
> > > > > --- a/fs/proc/base.c
> > > > > +++ b/fs/proc/base.c
> > > > > @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file
> > > > > * file, char __user * buf, .read = proc_sessionid_read,
> > > > > .llseek = generic_file_llseek,
> > > > > };
> > > > > +
> > > > > +static ssize_t proc_containerid_write(struct file *file, const char
> > > > > __user *buf,
> > > > > + size_t count, loff_t *ppos)
> > > > > +{
> > > > > + struct inode *inode = file_inode(file);
> > > > > + u64 containerid;
> > > > > + int rv;
> > > > > + struct task_struct *task = get_proc_task(inode);
> > > > > +
> > > > > + if (!task)
> > > > > + return -ESRCH;
> > > > > + if (*ppos != 0) {
> > > > > + /* No partial writes. */
> > > > > + put_task_struct(task);
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + rv = kstrtou64_from_user(buf, count, 10, &containerid);
> > > > > + if (rv < 0) {
> > > > > + put_task_struct(task);
> > > > > + return rv;
> > > > > + }
> > > > > +
> > > > > + rv = audit_set_containerid(task, containerid);
> > > > > + put_task_struct(task);
> > > > > + if (rv < 0)
> > > > > + return rv;
> > > > > + return count;
> > > > > +}
> > > > > +
> > > > > +static const struct file_operations proc_containerid_operations = {
> > > > > + .write = proc_containerid_write,
> > > > > + .llseek = generic_file_llseek,
> > > > > +};
> > > > > +
> > > > > #endif
> > > > >
> > > > > #ifdef CONFIG_FAULT_INJECTION
> > > > > @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file
> > > > > *m, struct pid_namespace *ns, #ifdef CONFIG_AUDITSYSCALL
> > > > > REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > > > > REG("sessionid", S_IRUGO, proc_sessionid_operations),
> > > > > + REG("containerid", S_IWUSR, proc_containerid_operations),
> > > > > #endif
> > > > > #ifdef CONFIG_FAULT_INJECTION
> > > > > REG("make-it-fail", S_IRUGO|S_IWUSR,
> > > > > proc_fault_inject_operations), @@ -3355,6 +3391,7 @@ static int
> > > > > proc_tid_comm_permission(struct inode *inode, int mask) #ifdef
> > > > > CONFIG_AUDITSYSCALL REG("loginuid", S_IWUSR|S_IRUGO,
> > > > > proc_loginuid_operations), REG("sessionid", S_IRUGO,
> > > > > proc_sessionid_operations),
> > > > > + REG("containerid", S_IWUSR, proc_containerid_operations),
> > > > > #endif
> > > > > #ifdef CONFIG_FAULT_INJECTION
> > > > > REG("make-it-fail", S_IRUGO|S_IWUSR,
> > > > > proc_fault_inject_operations), diff --git a/include/linux/audit.h
> > > > > b/include/linux/audit.h index af410d9..fe4ba3f 100644
> > > > > --- a/include/linux/audit.h
> > > > > +++ b/include/linux/audit.h
> > > > > @@ -29,6 +29,7 @@
> > > > >
> > > > > #define AUDIT_INO_UNSET ((unsigned long)-1)
> > > > > #define AUDIT_DEV_UNSET ((dev_t)-1)
> > > > > +#define INVALID_CID AUDIT_CID_UNSET
> > > > >
> > > > > struct audit_sig_info {
> > > > > uid_t uid;
> > > > > @@ -321,6 +322,7 @@ static inline void audit_ptrace(struct
> > > > > task_struct *t) extern int auditsc_get_stamp(struct audit_context
> > > > > *ctx, struct timespec64 *t, unsigned int *serial);
> > > > > extern int audit_set_loginuid(kuid_t loginuid);
> > > > > +extern int audit_set_containerid(struct task_struct *tsk, u64
> > > > > containerid);
> > > > > static inline kuid_t audit_get_loginuid(struct task_struct *tsk)
> > > > > {
> > > > > @@ -332,6 +334,11 @@ static inline unsigned int
> > > > > audit_get_sessionid(struct task_struct *tsk) return tsk->sessionid;
> > > > > }
> > > > >
> > > > > +static inline u64 audit_get_containerid(struct task_struct *tsk)
> > > > > +{
> > > > > + return tsk->containerid;
> > > > > +}
> > > > > +
> > > > > extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
> > > > > extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid,
> > > > > gid_t gid, umode_t mode); extern void __audit_bprm(struct
> > > > > linux_binprm *bprm); @@ -517,6 +524,10 @@ static inline unsigned int
> > > > > audit_get_sessionid(struct task_struct *tsk) {
> > > > > return -1;
> > > > > }
> > > > > +static inline kuid_t audit_get_containerid(struct task_struct *tsk)
> > > > > +{
> > > > > + return INVALID_CID;
> > > > > +}
> > > > > static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> > > > > { }
> > > > > static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t
> > > > > uid, @@ -581,6 +592,11 @@ static inline bool
> > > > > audit_loginuid_set(struct task_struct *tsk) return
> > > > > uid_valid(audit_get_loginuid(tsk)); }
> > > > >
> > > > > +static inline bool audit_containerid_set(struct task_struct *tsk)
> > > > > +{
> > > > > + return audit_get_containerid(tsk) != INVALID_CID;
> > > > > +}
> > > > > +
> > > > > static inline void audit_log_string(struct audit_buffer *ab, const
> > > > > char *buf) {
> > > > > audit_log_n_string(ab, buf, strlen(buf));
> > > > > diff --git a/include/linux/init_task.h b/include/linux/init_task.h
> > > > > index 6a53262..046bd0a 100644
> > > > > --- a/include/linux/init_task.h
> > > > > +++ b/include/linux/init_task.h
> > > > > @@ -18,6 +18,7 @@
> > > > > #include <linux/sched/rt.h>
> > > > > #include <linux/livepatch.h>
> > > > > #include <linux/mm_types.h>
> > > > > +#include <linux/audit.h>
> > > > >
> > > > > #include <asm/thread_info.h>
> > > > >
> > > > > @@ -120,7 +121,8 @@
> > > > > #ifdef CONFIG_AUDITSYSCALL
> > > > > #define INIT_IDS \
> > > > > .loginuid = INVALID_UID, \
> > > > > - .sessionid = (unsigned int)-1,
> > > > > + .sessionid = (unsigned int)-1, \
> > > > > + .containerid = INVALID_CID,
> > > > > #else
> > > > > #define INIT_IDS
> > > > > #endif
> > > > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > > > index d258826..1b82191 100644
> > > > > --- a/include/linux/sched.h
> > > > > +++ b/include/linux/sched.h
> > > > > @@ -796,6 +796,7 @@ struct task_struct {
> > > > > #ifdef CONFIG_AUDITSYSCALL
> > > > > kuid_t loginuid;
> > > > > unsigned int sessionid;
> > > > > + u64 containerid;
> > > > > #endif
> > > > > struct seccomp seccomp;
> > > > >
> > > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > > > index 4e61a9e..921a71f 100644
> > > > > --- a/include/uapi/linux/audit.h
> > > > > +++ b/include/uapi/linux/audit.h
> > > > > @@ -71,6 +71,7 @@
> > > > > #define AUDIT_TTY_SET 1017 /* Set TTY auditing
> > > > > status */ #define AUDIT_SET_FEATURE 1018 /* Turn an
> > > > > audit feature on or off */ #define AUDIT_GET_FEATURE
> > > > > 1019 /* Get which features are enabled */ +#define
> > > > > AUDIT_CONTAINER 1020 /* Define the container id
> > > > > and information */ #define AUDIT_FIRST_USER_MSG 1100 /*
> > > > > Userspace messages mostly uninteresting to kernel */ #define
> > > > > AUDIT_USER_AVC 1107 /* We filter this
> > > > > differently */ @@ -465,6 +466,7 @@ struct audit_tty_status { };
> > > > >
> > > > > #define AUDIT_UID_UNSET (unsigned int)-1
> > > > > +#define AUDIT_CID_UNSET ((u64)-1)
> > > > >
> > > > > /* audit_rule_data supports filter rules with both integer and string
> > > > > * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
> > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > index 4e0a4ac..29c8482 100644
> > > > > --- a/kernel/auditsc.c
> > > > > +++ b/kernel/auditsc.c
> > > > > @@ -2073,6 +2073,90 @@ int audit_set_loginuid(kuid_t loginuid)
> > > > > return rc;
> > > > > }
> > > > >
> > > > > +static int audit_set_containerid_perm(struct task_struct *task, u64
> > > > > containerid) +{
> > > > > + struct task_struct *parent;
> > > > > + u64 pcontainerid, ccontainerid;
> > > > > +
> > > > > + /* Don't allow to set our own containerid */
> > > > > + if (current == task)
> > > > > + return -EPERM;
> > > > > + /* Don't allow the containerid to be unset */
> > > > > + if (!cid_valid(containerid))
> > > > > + return -EINVAL;
> > > > > + /* if we don't have caps, reject */
> > > > > + if (!capable(CAP_AUDIT_CONTROL))
> > > > > + return -EPERM;
> > > > > + /* if containerid is unset, allow */
> > > > > + if (!audit_containerid_set(task))
> > > > > + return 0;
> > > > > + /* it is already set, and not inherited from the parent,
> > > > > reject */
> > > > > + ccontainerid = audit_get_containerid(task);
> > > > > + rcu_read_lock();
> > > > > + parent = rcu_dereference(task->real_parent);
> > > > > + rcu_read_unlock();
> > > > > + task_lock(parent);
> > > > > + pcontainerid = audit_get_containerid(parent);
> > > > > + task_unlock(parent);
> > > > > + if (ccontainerid != pcontainerid)
> > > > > + return -EPERM;
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static void audit_log_set_containerid(struct task_struct *task, u64
> > > > > oldcontainerid,
> > > > > + u64 containerid, int rc)
> > > > > +{
> > > > > + struct audit_buffer *ab;
> > > > > + uid_t uid;
> > > > > + struct tty_struct *tty;
> > > > > +
> > > > > + if (!audit_enabled)
> > > > > + return;
> > > > > +
> > > > > + ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONTAINER);
> > > > > + if (!ab)
> > > > > + return;
> > > > > +
> > > > > + uid = from_kuid(&init_user_ns, task_uid(current));
> > > > > + tty = audit_get_tty(current);
> > > > > +
> > > > > + audit_log_format(ab, "op=set pid=%d uid=%u",
> > > > > task_tgid_nr(current), uid);
> > > > > + audit_log_task_context(ab);
> > > > > + audit_log_format(ab, " auid=%u tty=%s ses=%u opid=%d
> > > > > old-contid=%llu contid=%llu res=%d",
> > > >
> > > > The preferred ordering would be: op, opid, old-contid, contid, pid, uid,
> > > > tty, ses, subj, comm, exe, res. This groups the searchable fields
> > > > together using the most common ordering so that parsing is simple.
> > >
> > > Where would you like auid? It appears that just before uid would be the
> > > right place, if not in place of uid, but this is just a guess since it
> > > isn't consistent.
> >
> >
> > Just after the uid is the proper place. The most common sequence is:
> > pid, uid, auid, tty, session, subject context, comm, exe.
>
> Not according to:
> http://people.redhat.com/sgrubb/audit/record-fields.html
Ok, maybe I spoke a bit quickly... Most of the inital ones I saw were
in the reverse order, but there is a clump about 3/4 of the way down
which includes VIRT and USER events.
> > -Steve
>
> - RGB
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
From: Richard Guy Briggs @ 2018-06-04 20:23 UTC (permalink / raw)
To: Steve Grubb
Cc: linux-api, containers, LKML, eparis, Linux-Audit Mailing List,
linux-fsdevel, viro
In-Reply-To: <36399691.WpcOE4jDTJ@x2>
On 2018-06-04 12:09, Steve Grubb wrote:
> On Friday, June 1, 2018 5:04:55 PM EDT Richard Guy Briggs wrote:
> > Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
> >
> > From: Richard Guy Briggs <rgb@redhat.com>
> > To: Me
> > CC: linux-api@vger.kernel.org, containers@lists.linux-foundation.org, LKML
> > <linux-kernel@vger.kernel.org>, eparis@parisplace.org, ... Date: 6/1/18
> > 5:04 PM
> >
> > On 2018-05-17 17:00, Steve Grubb wrote:
> > > On Fri, 16 Mar 2018 05:00:28 -0400
> > >
> > > Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > Implement the proc fs write to set the audit container ID of a
> > > > process, emitting an AUDIT_CONTAINER record to document the event.
> > > >
> > > > This is a write from the container orchestrator task to a proc entry
> > > > of the form /proc/PID/containerid where PID is the process ID of the
> > > > newly created task that is to become the first task in a container,
> > > > or an additional task added to a container.
> > > >
> > > > The write expects up to a u64 value (unset: 18446744073709551615).
> > > >
> > > > This will produce a record such as this:
> > > > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0
> > > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0
> > > > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455
> > > > res=0
> > >
> > > The was one thing I was wondering about. Currently when we set the
> > > loginuid, the record is AUDIT_LOGINUID. The corollary is that when we
> > > set the container id, the event should be AUDIT_CONTAINERID or
> > > AUDIT_CONTAINER_ID.
> > >
> > > During syscall events, the path info is returned in a a record simply
> > > called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So, rather than
> > > calling the record that gets attached to everything
> > > AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.
> > >
> > > > The "op" field indicates an initial set. The "pid" to "ses" fields
> > > > are the orchestrator while the "opid" field is the object's PID, the
> > > > process being "contained". Old and new container ID values are given
> > > > in the "contid" fields, while res indicates its success.
> > > >
> > > > It is not permitted to self-set, unset or re-set the container ID. A
> > > > child inherits its parent's container ID, but then can be set only
> > > > once after.
> > > >
> > > > See: https://github.com/linux-audit/audit-kernel/issues/32
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > > fs/proc/base.c | 37 ++++++++++++++++++++
> > > > include/linux/audit.h | 16 +++++++++
> > > > include/linux/init_task.h | 4 ++-
> > > > include/linux/sched.h | 1 +
> > > > include/uapi/linux/audit.h | 2 ++
> > > > kernel/auditsc.c | 84
> > > > ++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 143
> > > > insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > > index 60316b5..6ce4fbe 100644
> > > > --- a/fs/proc/base.c
> > > > +++ b/fs/proc/base.c
> > > > @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file
> > > > * file, char __user * buf, .read = proc_sessionid_read,
> > > > .llseek = generic_file_llseek,
> > > > };
> > > > +
> > > > +static ssize_t proc_containerid_write(struct file *file, const char
> > > > __user *buf,
> > > > + size_t count, loff_t *ppos)
> > > > +{
> > > > + struct inode *inode = file_inode(file);
> > > > + u64 containerid;
> > > > + int rv;
> > > > + struct task_struct *task = get_proc_task(inode);
> > > > +
> > > > + if (!task)
> > > > + return -ESRCH;
> > > > + if (*ppos != 0) {
> > > > + /* No partial writes. */
> > > > + put_task_struct(task);
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + rv = kstrtou64_from_user(buf, count, 10, &containerid);
> > > > + if (rv < 0) {
> > > > + put_task_struct(task);
> > > > + return rv;
> > > > + }
> > > > +
> > > > + rv = audit_set_containerid(task, containerid);
> > > > + put_task_struct(task);
> > > > + if (rv < 0)
> > > > + return rv;
> > > > + return count;
> > > > +}
> > > > +
> > > > +static const struct file_operations proc_containerid_operations = {
> > > > + .write = proc_containerid_write,
> > > > + .llseek = generic_file_llseek,
> > > > +};
> > > > +
> > > > #endif
> > > >
> > > > #ifdef CONFIG_FAULT_INJECTION
> > > > @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file
> > > > *m, struct pid_namespace *ns, #ifdef CONFIG_AUDITSYSCALL
> > > > REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > > > REG("sessionid", S_IRUGO, proc_sessionid_operations),
> > > > + REG("containerid", S_IWUSR, proc_containerid_operations),
> > > > #endif
> > > > #ifdef CONFIG_FAULT_INJECTION
> > > > REG("make-it-fail", S_IRUGO|S_IWUSR,
> > > > proc_fault_inject_operations), @@ -3355,6 +3391,7 @@ static int
> > > > proc_tid_comm_permission(struct inode *inode, int mask) #ifdef
> > > > CONFIG_AUDITSYSCALL REG("loginuid", S_IWUSR|S_IRUGO,
> > > > proc_loginuid_operations), REG("sessionid", S_IRUGO,
> > > > proc_sessionid_operations),
> > > > + REG("containerid", S_IWUSR, proc_containerid_operations),
> > > > #endif
> > > > #ifdef CONFIG_FAULT_INJECTION
> > > > REG("make-it-fail", S_IRUGO|S_IWUSR,
> > > > proc_fault_inject_operations), diff --git a/include/linux/audit.h
> > > > b/include/linux/audit.h index af410d9..fe4ba3f 100644
> > > > --- a/include/linux/audit.h
> > > > +++ b/include/linux/audit.h
> > > > @@ -29,6 +29,7 @@
> > > >
> > > > #define AUDIT_INO_UNSET ((unsigned long)-1)
> > > > #define AUDIT_DEV_UNSET ((dev_t)-1)
> > > > +#define INVALID_CID AUDIT_CID_UNSET
> > > >
> > > > struct audit_sig_info {
> > > > uid_t uid;
> > > > @@ -321,6 +322,7 @@ static inline void audit_ptrace(struct
> > > > task_struct *t) extern int auditsc_get_stamp(struct audit_context
> > > > *ctx, struct timespec64 *t, unsigned int *serial);
> > > > extern int audit_set_loginuid(kuid_t loginuid);
> > > > +extern int audit_set_containerid(struct task_struct *tsk, u64
> > > > containerid);
> > > > static inline kuid_t audit_get_loginuid(struct task_struct *tsk)
> > > > {
> > > > @@ -332,6 +334,11 @@ static inline unsigned int
> > > > audit_get_sessionid(struct task_struct *tsk) return tsk->sessionid;
> > > > }
> > > >
> > > > +static inline u64 audit_get_containerid(struct task_struct *tsk)
> > > > +{
> > > > + return tsk->containerid;
> > > > +}
> > > > +
> > > > extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
> > > > extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid,
> > > > gid_t gid, umode_t mode); extern void __audit_bprm(struct
> > > > linux_binprm *bprm); @@ -517,6 +524,10 @@ static inline unsigned int
> > > > audit_get_sessionid(struct task_struct *tsk) {
> > > > return -1;
> > > > }
> > > > +static inline kuid_t audit_get_containerid(struct task_struct *tsk)
> > > > +{
> > > > + return INVALID_CID;
> > > > +}
> > > > static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> > > > { }
> > > > static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t
> > > > uid, @@ -581,6 +592,11 @@ static inline bool
> > > > audit_loginuid_set(struct task_struct *tsk) return
> > > > uid_valid(audit_get_loginuid(tsk)); }
> > > >
> > > > +static inline bool audit_containerid_set(struct task_struct *tsk)
> > > > +{
> > > > + return audit_get_containerid(tsk) != INVALID_CID;
> > > > +}
> > > > +
> > > > static inline void audit_log_string(struct audit_buffer *ab, const
> > > > char *buf) {
> > > > audit_log_n_string(ab, buf, strlen(buf));
> > > > diff --git a/include/linux/init_task.h b/include/linux/init_task.h
> > > > index 6a53262..046bd0a 100644
> > > > --- a/include/linux/init_task.h
> > > > +++ b/include/linux/init_task.h
> > > > @@ -18,6 +18,7 @@
> > > > #include <linux/sched/rt.h>
> > > > #include <linux/livepatch.h>
> > > > #include <linux/mm_types.h>
> > > > +#include <linux/audit.h>
> > > >
> > > > #include <asm/thread_info.h>
> > > >
> > > > @@ -120,7 +121,8 @@
> > > > #ifdef CONFIG_AUDITSYSCALL
> > > > #define INIT_IDS \
> > > > .loginuid = INVALID_UID, \
> > > > - .sessionid = (unsigned int)-1,
> > > > + .sessionid = (unsigned int)-1, \
> > > > + .containerid = INVALID_CID,
> > > > #else
> > > > #define INIT_IDS
> > > > #endif
> > > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > > index d258826..1b82191 100644
> > > > --- a/include/linux/sched.h
> > > > +++ b/include/linux/sched.h
> > > > @@ -796,6 +796,7 @@ struct task_struct {
> > > > #ifdef CONFIG_AUDITSYSCALL
> > > > kuid_t loginuid;
> > > > unsigned int sessionid;
> > > > + u64 containerid;
> > > > #endif
> > > > struct seccomp seccomp;
> > > >
> > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > > index 4e61a9e..921a71f 100644
> > > > --- a/include/uapi/linux/audit.h
> > > > +++ b/include/uapi/linux/audit.h
> > > > @@ -71,6 +71,7 @@
> > > > #define AUDIT_TTY_SET 1017 /* Set TTY auditing
> > > > status */ #define AUDIT_SET_FEATURE 1018 /* Turn an
> > > > audit feature on or off */ #define AUDIT_GET_FEATURE
> > > > 1019 /* Get which features are enabled */ +#define
> > > > AUDIT_CONTAINER 1020 /* Define the container id
> > > > and information */ #define AUDIT_FIRST_USER_MSG 1100 /*
> > > > Userspace messages mostly uninteresting to kernel */ #define
> > > > AUDIT_USER_AVC 1107 /* We filter this
> > > > differently */ @@ -465,6 +466,7 @@ struct audit_tty_status { };
> > > >
> > > > #define AUDIT_UID_UNSET (unsigned int)-1
> > > > +#define AUDIT_CID_UNSET ((u64)-1)
> > > >
> > > > /* audit_rule_data supports filter rules with both integer and string
> > > > * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index 4e0a4ac..29c8482 100644
> > > > --- a/kernel/auditsc.c
> > > > +++ b/kernel/auditsc.c
> > > > @@ -2073,6 +2073,90 @@ int audit_set_loginuid(kuid_t loginuid)
> > > > return rc;
> > > > }
> > > >
> > > > +static int audit_set_containerid_perm(struct task_struct *task, u64
> > > > containerid) +{
> > > > + struct task_struct *parent;
> > > > + u64 pcontainerid, ccontainerid;
> > > > +
> > > > + /* Don't allow to set our own containerid */
> > > > + if (current == task)
> > > > + return -EPERM;
> > > > + /* Don't allow the containerid to be unset */
> > > > + if (!cid_valid(containerid))
> > > > + return -EINVAL;
> > > > + /* if we don't have caps, reject */
> > > > + if (!capable(CAP_AUDIT_CONTROL))
> > > > + return -EPERM;
> > > > + /* if containerid is unset, allow */
> > > > + if (!audit_containerid_set(task))
> > > > + return 0;
> > > > + /* it is already set, and not inherited from the parent,
> > > > reject */
> > > > + ccontainerid = audit_get_containerid(task);
> > > > + rcu_read_lock();
> > > > + parent = rcu_dereference(task->real_parent);
> > > > + rcu_read_unlock();
> > > > + task_lock(parent);
> > > > + pcontainerid = audit_get_containerid(parent);
> > > > + task_unlock(parent);
> > > > + if (ccontainerid != pcontainerid)
> > > > + return -EPERM;
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void audit_log_set_containerid(struct task_struct *task, u64
> > > > oldcontainerid,
> > > > + u64 containerid, int rc)
> > > > +{
> > > > + struct audit_buffer *ab;
> > > > + uid_t uid;
> > > > + struct tty_struct *tty;
> > > > +
> > > > + if (!audit_enabled)
> > > > + return;
> > > > +
> > > > + ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONTAINER);
> > > > + if (!ab)
> > > > + return;
> > > > +
> > > > + uid = from_kuid(&init_user_ns, task_uid(current));
> > > > + tty = audit_get_tty(current);
> > > > +
> > > > + audit_log_format(ab, "op=set pid=%d uid=%u",
> > > > task_tgid_nr(current), uid);
> > > > + audit_log_task_context(ab);
> > > > + audit_log_format(ab, " auid=%u tty=%s ses=%u opid=%d
> > > > old-contid=%llu contid=%llu res=%d",
> > >
> > > The preferred ordering would be: op, opid, old-contid, contid, pid, uid,
> > > tty, ses, subj, comm, exe, res. This groups the searchable fields
> > > together using the most common ordering so that parsing is simple.
> >
> > Where would you like auid? It appears that just before uid would be the
> > right place, if not in place of uid, but this is just a guess since it
> > isn't consistent.
>
>
> Just after the uid is the proper place. The most common sequence is:
> pid, uid, auid, tty, session, subject context, comm, exe.
Not according to:
http://people.redhat.com/sgrubb/audit/record-fields.html
> -Steve
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])
From: Miklos Szeredi @ 2018-06-04 19:38 UTC (permalink / raw)
To: Al Viro
Cc: Matthew Wilcox, David Howells, Christoph Hellwig, linux-fsdevel,
linux-afs, LKML, Linux API
In-Reply-To: <20180604173508.GJ30522@ZenIV.linux.org.uk>
On Mon, Jun 4, 2018 at 7:35 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Jun 04, 2018 at 10:16:30AM -0700, Matthew Wilcox wrote:
>> On Sun, Jun 03, 2018 at 01:55:37AM +0100, Al Viro wrote:
>> > +SYSCALL_DEFINE3(open_tree, int, dfd, const char *, filename, unsigned, flags)
>> > +{
>> > + struct file *file;
>> > + struct path path;
>> > + int lookup_flags = LOOKUP_AUTOMOUNT | LOOKUP_FOLLOW;
>> > + bool detached = flags & OPEN_TREE_CLONE;
>> > + int error;
>> > + int fd;
>> > +
>> > + BUILD_BUG_ON(OPEN_TREE_CLOEXEC != O_CLOEXEC);
>>
>> Why do we need OPEN_TREE_CLOEXEC? Wouldn't we be better off just making
>> the fd returned by open_tree implicitly close-on-exec? I can think of
>> no good reason for these file descriptors to be inherited across exec()
>
> How are they different from any file descriptor? It's not as if it was
> something usable only for mounting stuff - again, you can use them
> with any ...at() syscalls.
Defaulting to close on exec helps keep out clutter from the API.
Is there a disadvantage to needing an explicit fcntl(F_SETFD) call to
disable close on exec?
Thanks,
Miklos
^ permalink raw reply
* Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])
From: Miklos Szeredi @ 2018-06-04 19:27 UTC (permalink / raw)
To: Al Viro
Cc: David Howells, Christoph Hellwig, linux-fsdevel, linux-afs, LKML,
Linux API
In-Reply-To: <20180604155205.GH30522@ZenIV.linux.org.uk>
On Mon, Jun 4, 2018 at 5:52 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Jun 04, 2018 at 12:34:44PM +0200, Miklos Szeredi wrote:
>
>> fsopen = create fsfd
>> fsmount = fsfd -> mountfd & set attr on mountfd & attach mountfd
>> fspick = path -> fsfd
>> move_mount = attach mountfd or move existing
>> fsinfo = info from path
>> open_tree = new mountfd from path or clone
>> mount_setattr = set attr on mountfd
>>
>> Notice that fsmount() encompasses mount_setattr() + move_mount()
>> functionality. Split those out and leave fsmount() to actually do
>> the "fsfd ->mountfd" translation?
>
> Might make sense.
> FWIW, to make it clear: fsmount(2) in this series actually does *NOT*
> attach it to the tree.
Ah, that leaves the mount_setattr() functionality to split out. I'd
be more happy to rid this new API of all the old MS_* crap and have
have a new set of attributes, that just apply to mounts. It will
also need two args: a bitmap of new attributes and a mask to tell us
which attributes to change.
> Commit message definitely needs updating - as it
> is, it's
>
> +SYSCALL_DEFINE5(fsmount, int, fs_fd, unsigned int, flags, unsigned int, ms_flags,
> + void *, reserved4, void *, reserved5)
>
> PS: IMO these reserved... arguments are in bad taste; if anyone has good reasons
> for that practice in ABI design, I'd like to hear those.
Agreed. A flags argument is often wise to add even if currently
unused (and should be checked for undefined flags), but adding a
random number of pointers doesn't seem to make a lot of sense.
>
>> fsinfo() name suggests it's in the same class as
>> fsopen/fsmount/fspick, operating on fsfd object, but's it's not and I
>> think that's slightly confusing.
>>
>> Rename move_mount() -> mount_move()?
>
> mount_move_bikeshed_bikeshed_bikeshed(), surely?
Consistent naming for related functions... not unheard of in API
design. The above set definitely does not qualify.
>> Also does it make sense to make the cloning behavior of open_tree()
>> optional? Without cloning it's just a plain open(O_PATH). That way
>> it could be renamed mount_clone().
>
> Umm... I'm not sure about that one. If nothing else, OPEN_TREE_DETACH
> might be a good idea, in which case cloning is not the primary effect;
> hell knows.
So conceptually we have the following distinct mount tree operations:
treefd = clone(path);
treefd = detach(path);
attach(treefd, path);
move(path1, path2);
The detach/move/attach trio are more related in functionality, while
clone and detach have the same signature. I'm not sure either.
Thanks,
Miklos
^ permalink raw reply
* [PATCH 2/2] fs: aio ioprio use ioprio_check_cap ret val
From: adam.manzanares @ 2018-06-04 17:59 UTC (permalink / raw)
To: viro, linux-fsdevel
Cc: axboe, bcrl, mingo, tglx, kstewart, peterz, pombredanne, gregkh,
bigeasy, rgoldwyn, linux-block, linux-kernel, linux-aio,
linux-api, hch, jmoyer, willy, smfrench, Adam Manzanares
In-Reply-To: <20180604175957.2647-1-adam.manzanares@wdc.com>
From: Adam Manzanares <adam.manzanares@wdc.com>
Previously the value was ignored.
Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
---
fs/aio.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/aio.c b/fs/aio.c
index 9527ededa669..134e5b635d64 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1443,8 +1443,8 @@ static int aio_prep_rw(struct kiocb *req, struct iocb *iocb)
*/
ret = ioprio_check_cap(iocb->aio_reqprio);
if (ret) {
- pr_debug("aio ioprio check cap error\n");
- return -EINVAL;
+ pr_debug("aio ioprio check cap error: %d\n", ret);
+ return ret;
}
req->ki_ioprio = iocb->aio_reqprio;
--
2.17.0
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox