Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752619AbdHNViT (ORCPT ); Mon, 14 Aug 2017 17:38:19 -0400 Received: from mail.kernel.org ([198.145.29.99]:55044 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752576AbdHNViQ (ORCPT ); Mon, 14 Aug 2017 17:38:16 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D8E8B22C91 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=atull@kernel.org MIME-Version: 1.0 In-Reply-To: <1498441938-14046-20-git-send-email-hao.wu@intel.com> References: <1498441938-14046-1-git-send-email-hao.wu@intel.com> <1498441938-14046-20-git-send-email-hao.wu@intel.com> From: Alan Tull Date: Mon, 14 Aug 2017 16:37:34 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2 19/22] fpga: intel: afu: add header sub feature support To: Wu Hao Cc: Moritz Fischer , linux-fpga@vger.kernel.org, linux-kernel , linux-api@vger.kernel.org, "Kang, Luwei" , "Zhang, Yi Z" , Tim Whisonant , Enno Luebbers , Shiva Rao , Christopher Rauer Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7396 Lines: 216 On Sun, Jun 25, 2017 at 8:52 PM, Wu Hao wrote: Hi Hao, > The header register set is always present for the Port/AFU, it is mainly > for capability, control and status of the ports that AFU connected to. So just to be clear, the reset function is acting on the Port, not the AFU, right? > > This patch implements header sub feature support. Please add a brief reminder here what the 'header' is. It's defined in patch 7 as being part of the feature list, but hardly mentioned when I grep intel-fpga.txt. > Below user interfaces > are created by this patch. > > Sysfs interface: > * /sys/class/fpga///id > Read-only. Port ID. > > Ioctl interface: > * FPGA_PORT_RESET > Reset the FPGA AFU Port. > > Signed-off-by: Tim Whisonant > Signed-off-by: Enno Luebbers > Signed-off-by: Shiva Rao > Signed-off-by: Christopher Rauer > Signed-off-by: Xiao Guangrong > Signed-off-by: Wu Hao > --- > v2: add sysfs documentation. > --- > .../ABI/testing/sysfs-platform-intel-fpga-afu | 7 ++++ > drivers/fpga/intel-afu-main.c | 44 +++++++++++++++++++++- > include/uapi/linux/intel-fpga.h | 14 +++++++ > 3 files changed, 64 insertions(+), 1 deletion(-) > create mode 100644 Documentation/ABI/testing/sysfs-platform-intel-fpga-afu > > diff --git a/Documentation/ABI/testing/sysfs-platform-intel-fpga-afu b/Documentation/ABI/testing/sysfs-platform-intel-fpga-afu > new file mode 100644 > index 0000000..8ad22c9 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-platform-intel-fpga-afu > @@ -0,0 +1,7 @@ > +What: /sys/bus/platform/devices/intel-fpga-port.0/id > +Date: June 2017 > +KernelVersion: 4.12 > +Contact: Wu Hao > +Description: Read-only. It returns id of this port. One Intel FPGA device > + may have more than one port. Userspace could use this id to > + distinguish different ports under same FPGA device. > diff --git a/drivers/fpga/intel-afu-main.c b/drivers/fpga/intel-afu-main.c > index 96d0367..2a17cde 100644 > --- a/drivers/fpga/intel-afu-main.c > +++ b/drivers/fpga/intel-afu-main.c > @@ -18,25 +18,66 @@ > > #include > #include > +#include > > #include "intel-feature-dev.h" > > +static ssize_t > +id_show(struct device *dev, struct device_attribute *attr, char *buf) > +{ > + int id = fpga_port_id(to_platform_device(dev)); > + > + return scnprintf(buf, PAGE_SIZE, "%d\n", id); > +} > +static DEVICE_ATTR_RO(id); > + > +static const struct attribute *port_hdr_attrs[] = { > + &dev_attr_id.attr, > + NULL, > +}; > + > static int port_hdr_init(struct platform_device *pdev, struct feature *feature) > { > dev_dbg(&pdev->dev, "PORT HDR Init.\n"); > > - return 0; > + fpga_port_reset(pdev); So the port will be reset here, which happens during fme_probe(). IIUC the PR region is empty then, there is just the static region, right? > + > + return sysfs_create_files(&pdev->dev.kobj, port_hdr_attrs); Greg wrote an article that there could be a race condition caused by creating sysfs files this late [1] and I see sysfs_create_files() used very sparingly in the kernel. I'm thinking that fpga-bridge should provide a place to create sysfs files earlier by adding an attribute_group to fpga_bridge_ops (same for fpga-mgr and fpga-region) and then fpga_bridge_register could do bridge->dev.groups = br_ops->groups. I'll put a patch for that out soon. > } > > static void port_hdr_uinit(struct platform_device *pdev, > struct feature *feature) > { > dev_dbg(&pdev->dev, "PORT HDR UInit.\n"); > + > + sysfs_remove_files(&pdev->dev.kobj, port_hdr_attrs); > +} > + > +static long > +port_hdr_ioctl(struct platform_device *pdev, struct feature *feature, > + unsigned int cmd, unsigned long arg) > +{ > + long ret; > + > + switch (cmd) { > + case FPGA_PORT_RESET: > + if (!arg) > + ret = fpga_port_reset(pdev); fpga_port_reset() disables and reenables traffic on the port. Is there ever a time when that would be unsafe to do? Like while DMA is happening? When I see a function called 'reset' exposed to userspace, I become concerned that hitting that reset at the wrong time could cause problems. We've discussed this some, but could you please remind me when userspace would need to reset the port? Please add documentation of what the intended use of this ioctl would be, when it is valid to request a reset from userspace and when userspace should never do that. The pcie code, the AFU file interface, and the bridge code all do port reset. This code is spread out over a few patches, but I'll comment here for now. I'm trying to keep track of everything that resets the port. The port gets reset in afu_probe, fme_probe, the AFU file release, and intel-pcie.c after parsing the features. Also the port is esentially reset after doing reprogramming, since that involves a bridge disable/enable. In the v1 review, the issue raised that the port functionality could be an expansion of fpga-bridge. If reset were added to the fpga_bridge_ops and a fpga_bridge_reset API added to fpga-bridge, then anything in the kernel that owns the bridge could reset it. That is of course assuming that this code doesn't need to reset the port before the fpga-bridge is created. Thanks, Alan Tull [1] http://www.kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/ > + else > + ret = -EINVAL; > + break; > + default: > + dev_dbg(&pdev->dev, "%x cmd not handled", cmd); > + ret = -ENODEV; > + } > + > + return ret; > } > > struct feature_ops port_hdr_ops = { > .init = port_hdr_init, > .uinit = port_hdr_uinit, > + .ioctl = port_hdr_ioctl, > }; > > static struct feature_driver port_feature_drvs[] = { > @@ -76,6 +117,7 @@ static int afu_release(struct inode *inode, struct file *filp) > > dev_dbg(&pdev->dev, "Device File Release\n"); > > + fpga_port_reset(pdev); > feature_dev_use_end(pdata); > return 0; > } > diff --git a/include/uapi/linux/intel-fpga.h b/include/uapi/linux/intel-fpga.h > index be295ae..be5f813 100644 > --- a/include/uapi/linux/intel-fpga.h > +++ b/include/uapi/linux/intel-fpga.h > @@ -30,8 +30,11 @@ > #define FPGA_MAGIC 0xB6 > > #define FPGA_BASE 0 > +#define PORT_BASE 0x40 > #define FME_BASE 0x80 > > +/* Common IOCTLs for both FME and AFU file descriptor */ > + > /** > * FPGA_GET_API_VERSION - _IO(FPGA_MAGIC, FPGA_BASE + 0) > * > @@ -50,6 +53,17 @@ > > #define FPGA_CHECK_EXTENSION _IO(FPGA_MAGIC, FPGA_BASE + 1) > > +/* IOCTLs for AFU file descriptor */ > + > +/** > + * FPGA_PORT_RESET - _IO(FPGA_MAGIC, PORT_BASE + 0) > + * > + * Reset the FPGA AFU Port. No parameters are supported. > + * Return: 0 on success, -errno of failure > + */ > + > +#define FPGA_PORT_RESET _IO(FPGA_MAGIC, PORT_BASE + 0) > + > /* IOCTLs for FME file descriptor */ > > /** > -- > 1.8.3.1 >