Date: Fri, 31 Mar 2017 21:31:09 +0800
From: Wu Hao <hao.wu@intel.com>
To: greg@kroah.com
Cc: atull@kernel.org, moritz.fischer@ettus.com, linux-fpga@vger.kernel.org,
        linux-kernel@vger.kernel.org, luwei.kang@intel.com,
        yi.z.zhang@intel.com, tim.whisonant@intel.com,
        Enno Luebbers <enno.luebbers@intel.com>,
        Shiva Rao <shiva.rao@intel.com>,
        Christopher Rauer <christopher.rauer@intel.com>,
        guangrong.xiao@linux.intel.com
Subject: Re: [PATCH 02/16] fpga: add FPGA device framework
Message-ID: <20170331133109.GB1312@hao-dev>
References: <1490875696-15145-1-git-send-email-hao.wu@intel.com>
 <1490875696-15145-3-git-send-email-hao.wu@intel.com>
 <20170331061322.GB7621@kroah.com>
 <82D7661F83C1A047AF7DC287873BF1E167C90F1B@SHSMSX101.ccr.corp.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <82D7661F83C1A047AF7DC287873BF1E167C90F1B@SHSMSX101.ccr.corp.intel.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 9638
Lines: 301

> On Thu, Mar 30, 2017 at 08:08:02PM +0800, Wu Hao wrote:
> > During FPGA device (e.g PCI-based) discovery, platform devices are 
> > registered for different FPGA function units. But the device node path 
> > isn't quite friendly to applications.
> > 
> > Consider this case, applications want to access child device's sysfs 
> > file for some information.
> > 
> > 1) Access using bus-based path (e.g PCI)
> > 
> >   /sys/bus/pci/devices/xxxxx/fpga_func_a.0/sysfs_file
> > 
> >   From the path, it's clear which PCI device is the parent, but not perfect
> >   solution for applications. PCI device BDF is not fixed, application may
> >   need to search all PCI device to find the actual FPGA Device.
> > 
> > 2) Or access using platform device path
> > 
> >   /sys/bus/platform/devices/fpga_func_a.0/sysfs_file
> > 
> >   Applications find the actual function by name easily, but no information
> >   about which fpga device it belongs to. It's quite confusing if multiple
> >   FPGA devices are in one system.
> > 
> > 'FPGA Device' class is introduced to resolve this problem. Each node 
> > under this class represents a fpga device, which may have one or more 
> > child devices. Applications only need to search under this FPGA Device 
> > class folder to find the child device node it needs.
> > 
> > For example, for the platform has 2 fpga devices, each fpga device has
> > 3 child devices, the hierarchy looks like this.
> > 
> > Two nodes are under /sys/class/fpga/:
> > /sys/class/fpga/fpga.0
> > /sys/class/fpga/fpga.1
> > 
> > Each node has 1 function A device and 2 function B devices:
> > /sys/class/fpga/fpga.0/func_a.0
> > /sys/class/fpga/fpga.0/func_b.0
> > /sys/class/fpga/fpga.0/func_b.1
> > 
> > /sys/class/fpga/fpga.1/func_a.1
> > /sys/class/fpga/fpga.1/func_b.2
> > /sys/class/fpga/fpga.1/func_b.3
> > 
> > This following APIs are provided by FPGA device framework:
> > * fpga_dev_create
> >   Create fpga device under the given parent device.
> > * fpga_dev_destroy
> >   Destroy fpga device
> > 
> > The following sysfs files are created:
> > * /sys/class/fpga/<fpga.x>/name
> >   Name of the fpga device.
> > 
> > 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>
> > ---
> >  drivers/fpga/Kconfig          |   6 +++
> >  drivers/fpga/Makefile         |   3 ++
> >  drivers/fpga/fpga-dev.c       | 120 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/fpga/fpga-dev.h |  34 ++++++++++++
> >  4 files changed, 163 insertions(+)
> >  create mode 100644 drivers/fpga/fpga-dev.c  create mode 100644 
> > include/linux/fpga/fpga-dev.h
> > 
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index 
> > ce861a2..d99b640 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -12,6 +12,12 @@ config FPGA
> >  	  manager drivers.
> >  
> >  if FPGA
> > +config FPGA_DEVICE
> > +	tristate "FPGA Device Framework"
> > +	help
> > +	  Say Y here if you want support for FPGA Devices from the kernel.
> > +	  The FPGA Device Framework adds a FPGA device class and provide
> > +	  interfaces to create FPGA devices.
> >  
> >  config FPGA_REGION
> >  	tristate "FPGA Region"
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index 
> > 8df07bc..53a41d2 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -5,6 +5,9 @@
> >  # Core FPGA Manager Framework
> >  obj-$(CONFIG_FPGA)			+= fpga-mgr.o
> >  
> > +# FPGA Device Framework
> > +obj-$(CONFIG_FPGA_DEVICE)		+= fpga-dev.o
> > +
> >  # FPGA Manager Drivers
> >  obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
> >  obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
> > diff --git a/drivers/fpga/fpga-dev.c b/drivers/fpga/fpga-dev.c new 
> > file mode 100644 index 0000000..0f4c0ed
> > --- /dev/null
> > +++ b/drivers/fpga/fpga-dev.c
> > @@ -0,0 +1,120 @@
> > +/*
> > + * FPGA Device Framework Driver
> > + *
> > + * Copyright (C) 2017 Intel Corporation, Inc.
> > + *
> > + * This work is licensed under a dual BSD/GPLv2 license. When using 
> > +or
> > + * redistributing this file, you may do so under either license. See 
> > +the
> > + * LICENSE.BSD file under drivers/fpga/intel for the BSD license and 
> > +see
> > + * the COPYING file in the top-level directory for the GPLv2 license.
> 
> Really?  A BSD licened file that does EXPORT_SYMBOL_GPL and interacts directly with the driver core?  Please go talk to some of your lawyers about this before you resubmit this correctly...

Sorry, will check and fix this in next version.

> 
> 
> > + */
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/fpga/fpga-dev.h>
> > +
> > +static DEFINE_IDA(fpga_dev_ida);
> > +static struct class *fpga_dev_class;
> > +
> > +static ssize_t name_show(struct device *dev,
> > +			 struct device_attribute *attr, char *buf) {
> > +	struct fpga_dev *fdev = to_fpga_dev(dev);
> > +
> > +	return sprintf(buf, "%s\n", fdev->name); } static 
> > +DEVICE_ATTR_RO(name);
> 
> There already is a name for the device, it's the directory name.

For current implementation, the directory will have a common name like

/sys/class/fpga/fpga.0
/sys/class/fpga/fpga.1
/sys/class/fpga/fpga.2
...

For the 'name' sysfs interface, driver can put more device specific info
into this 'name', e.g intel-fpga-dev. Userspace can use this information
to know which kind of FPGA device it is. e.g if applications read the
/sys/class/fpga/fpga.5/name as intel-fpga-dev, it means the 5th fpga
device on the system is a Intel FPGA device, and then application applies
related method to enumerate the accelerators for it.
And other existing fpga class has similar sysfs interface too, so I would
like to keep it aligned with others.

> 
> > +
> > +static struct attribute *fpga_dev_attrs[] = {
> > +	&dev_attr_name.attr,
> > +	NULL,
> > +};
> > +ATTRIBUTE_GROUPS(fpga_dev);
> > +
> > +/**
> > + * fpga_dev_create - create a fpga device
> > + * @parent: parent device
> > + * @name: fpga device name
> > + *
> > + * Return fpga_dev struct for success, error code otherwise.
> > + */
> > +struct fpga_dev *fpga_dev_create(struct device *parent, const char 
> > +*name) {
> > +	struct fpga_dev *fdev;
> > +	int id, ret = 0;
> > +
> > +	if (!name || !strlen(name)) {
> > +		dev_err(parent, "Attempt to register with no name!\n");
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	fdev = kzalloc(sizeof(*fdev), GFP_KERNEL);
> > +	if (!fdev)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	id = ida_simple_get(&fpga_dev_ida, 0, 0, GFP_KERNEL);
> > +	if (id < 0) {
> > +		ret = id;
> > +		goto error_kfree;
> > +	}
> > +
> > +	fdev->name = name;
> > +
> > +	device_initialize(&fdev->dev);
> > +	fdev->dev.class = fpga_dev_class;
> > +	fdev->dev.parent = parent;
> > +	fdev->dev.id = id;
> > +
> > +	ret = dev_set_name(&fdev->dev, "fpga.%d", id);
> > +	if (ret)
> > +		goto error_device;
> > +
> > +	ret = device_add(&fdev->dev);
> > +	if (ret)
> > +		goto error_device;
> > +
> > +	dev_dbg(fdev->dev.parent, "fpga device [%s] created\n", fdev->name);
> > +
> > +	return fdev;
> > +
> > +error_device:
> > +	ida_simple_remove(&fpga_dev_ida, id);
> > +error_kfree:
> > +	kfree(fdev);
> > +
> > +	return ERR_PTR(ret);
> > +}
> > +EXPORT_SYMBOL_GPL(fpga_dev_create);
> > +
> > +static void fpga_dev_release(struct device *dev) {
> > +	struct fpga_dev *fdev = to_fpga_dev(dev);
> > +
> > +	ida_simple_remove(&fpga_dev_ida, fdev->dev.id);
> > +	kfree(fdev);
> > +}
> > +
> > +static int __init fpga_dev_class_init(void) {
> > +	pr_info("FPGA Device framework\n");
> > +
> > +	fpga_dev_class = class_create(THIS_MODULE, "fpga");
> > +	if (IS_ERR(fpga_dev_class))
> > +		return PTR_ERR(fpga_dev_class);
> > +
> > +	fpga_dev_class->dev_groups = fpga_dev_groups;
> > +	fpga_dev_class->dev_release = fpga_dev_release;
> > +
> > +	return 0;
> > +}
> > +
> > +static void __exit fpga_dev_class_exit(void) {
> > +	class_destroy(fpga_dev_class);
> > +}
> > +
> > +MODULE_DESCRIPTION("FPGA Device framework"); MODULE_LICENSE("Dual 
> > +BSD/GPL");
> > +
> > +subsys_initcall(fpga_dev_class_init);
> > +module_exit(fpga_dev_class_exit);
> > diff --git a/include/linux/fpga/fpga-dev.h 
> > b/include/linux/fpga/fpga-dev.h new file mode 100644 index 
> > 0000000..7b58356
> > --- /dev/null
> > +++ b/include/linux/fpga/fpga-dev.h
> > @@ -0,0 +1,34 @@
> > +/*
> > + * FPGA Device Driver Header
> > + *
> > + * Copyright (C) 2017 Intel Corporation, Inc.
> > + *
> > + * This work is licensed under a dual BSD/GPLv2 license. When using 
> > +or
> > + * redistributing this file, you may do so under either license. See 
> > +the
> > + * LICENSE.BSD file under drivers/fpga/intel for the BSD license and 
> > +see
> > + * the COPYING file in the top-level directory for the GPLv2 license.
> 
> Again with the dual license, please fix up.
>

Sorry, will fix this in the next version.
 
> > + *
> > + */
> > +#ifndef _LINUX_FPGA_DEV_H
> > +#define _LINUX_FPGA_DEV_H
> > +
> > +/**
> > + * struct fpga_dev - fpga device structure
> > + * @name: name of fpga device
> > + * @dev: fpga device
> > + */
> > +struct fpga_dev {
> > +	const char *name;
> > +	struct device dev;
> 
> struct device already has a name, why duplicate it here?

As mentioned above, dev has common name as fpga.x, but 'name' could have
more meaningful information, e.g venodr and device information.

Thanks
Hao

> 
> thanks,
> 
> greg k-h