| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 6 Jun 2018 20:24:57 +0800
From: Wu Hao <hao.wu@...el.com>
To: Alan Tull <atull@...nel.org>
Cc: Moritz Fischer <mdf@...nel.org>, linux-fpga@...r.kernel.org,
linux-kernel <linux-kernel@...r.kernel.org>,
linux-api@...r.kernel.org, "Kang, Luwei" <luwei.kang@...el.com>,
"Zhang, Yi Z" <yi.z.zhang@...el.com>,
Tim Whisonant <tim.whisonant@...el.com>,
Enno Luebbers <enno.luebbers@...el.com>,
Shiva Rao <shiva.rao@...el.com>,
Christopher Rauer <christopher.rauer@...el.com>,
Xiao Guangrong <guangrong.xiao@...ux.intel.com>
Subject: Re: [PATCH v5 07/28] fpga: dfl: add chardev support for feature
devices
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@...el.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@...el.com>
> > Signed-off-by: Enno Luebbers <enno.luebbers@...el.com>
> > Signed-off-by: Shiva Rao <shiva.rao@...el.com>
> > Signed-off-by: Christopher Rauer <christopher.rauer@...el.com>
> > Signed-off-by: Zhang Yi <yi.z.zhang@...el.com>
> > Signed-off-by: Xiao Guangrong <guangrong.xiao@...ux.intel.com>
> > Signed-off-by: Wu Hao <hao.wu@...el.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
Powered by blists - more mailing lists