2019-08-04 10:40:07

by Wu Hao

Subject: [PATCH v4 00/12] FPGA DFL updates

Hi Greg,

This is v4 patchset which adds more features to FPGA DFL. The avx512
partial reconfiguration support patch is dropped for now as need
to address some opens first.

Main changes from v3:
- drop avx512 partail reconfiguration patch for now.
- split dfl_fpga_cdev_config_port to 2 functions *_release/assign_port
(#1).
- split __dfl_fpga_cdev_config_port_vf into 2 functions with locking
added (#2).
- improve description in sysfs doc to avoid misunderstanding (#3).
- switch to boolean in sysfs entry store function (#3).
- remove dev_dbg in init/uinit callback function (#7, #9, #11).
- remove uinit callback which does does nothing (#8, #9)

Main changes from v2:
- update kernel version/date in sysfs doc (patch #4, #5, #8, #10, #11).
- add back Documentation patch (patch #12).

Main changes from v1:
- remove DRV/MODULE_VERSION modifications. (patch #1, #3, #4, #6)
- remove argsz from new ioctls. (patch #2)
- replace sysfs_create/remove_* with device_add/remove_* for sysfs entries.
(patch #5, #8, #11)

Wu Hao (12):
fpga: dfl: fme: add DFL_FPGA_FME_PORT_RELEASE/ASSIGN ioctl support.
fpga: dfl: pci: enable SRIOV support.
fpga: dfl: afu: add AFU state related sysfs interfaces
fpga: dfl: afu: add userclock sysfs interfaces.
fpga: dfl: add id_table for dfl private feature driver
fpga: dfl: afu: export __port_enable/disable function.
fpga: dfl: afu: add error reporting support.
fpga: dfl: make uinit callback optional
fpga: dfl: afu: add STP (SignalTap) support
fpga: dfl: fme: add capability sysfs interfaces
fpga: dfl: fme: add global error reporting support
Documentation: fpga: dfl: add descriptions for virtualization and new
interfaces.

Documentation/ABI/testing/sysfs-platform-dfl-fme | 98 ++++++
Documentation/ABI/testing/sysfs-platform-dfl-port | 106 ++++++
Documentation/fpga/dfl.rst | 105 ++++++
drivers/fpga/Makefile | 3 +-
drivers/fpga/dfl-afu-error.c | 221 +++++++++++++
drivers/fpga/dfl-afu-main.c | 319 +++++++++++++++++-
drivers/fpga/dfl-afu.h | 7 +
drivers/fpga/dfl-fme-error.c | 381 ++++++++++++++++++++++
drivers/fpga/dfl-fme-main.c | 105 +++++-
drivers/fpga/dfl-fme-pr.c | 7 +-
drivers/fpga/dfl-fme.h | 5 +-
drivers/fpga/dfl-pci.c | 36 ++
drivers/fpga/dfl.c | 216 +++++++++++-
drivers/fpga/dfl.h | 54 ++-
include/uapi/linux/fpga-dfl.h | 18 +
15 files changed, 1640 insertions(+), 41 deletions(-)
create mode 100644 drivers/fpga/dfl-afu-error.c
create mode 100644 drivers/fpga/dfl-fme-error.c

--
1.8.3.1

2019-08-04 10:40:07

by Wu Hao

Subject: [PATCH v4 02/12] fpga: dfl: pci: enable SRIOV support.

This patch enables the standard sriov support. It allows user to
enable SRIOV (and VFs), then user could pass through accelerators
(VFs) into virtual machine or use VFs directly in host.

Signed-off-by: Zhang Yi Z <[email protected]>
Signed-off-by: Xu Yilun <[email protected]>
Signed-off-by: Wu Hao <[email protected]>
Acked-by: Alan Tull <[email protected]>
Acked-by: Moritz Fischer <[email protected]>
Signed-off-by: Moritz Fischer <[email protected]>
---
v2: remove DRV/MODULE_VERSION modifications.
v4: split __dfl_fpga_cdev_config_port_vf into 2 functions with
locking added.
---
drivers/fpga/dfl-pci.c | 36 ++++++++++++++++++++++
drivers/fpga/dfl.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++
drivers/fpga/dfl.h | 3 +-
3 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
index 66b5720..89ca292 100644
--- a/drivers/fpga/dfl-pci.c
+++ b/drivers/fpga/dfl-pci.c
@@ -223,8 +223,43 @@ int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
return ret;
}

+static int cci_pci_sriov_configure(struct pci_dev *pcidev, int num_vfs)
+{
+ struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
+ struct dfl_fpga_cdev *cdev = drvdata->cdev;
+ int ret = 0;
+
+ if (!num_vfs) {
+ /*
+ * disable SRIOV and then put released ports back to default
+ * PF access mode.
+ */
+ pci_disable_sriov(pcidev);
+
+ dfl_fpga_cdev_config_ports_pf(cdev);
+
+ } else {
+ /*
+ * before enable SRIOV, put released ports into VF access mode
+ * first of all.
+ */
+ ret = dfl_fpga_cdev_config_ports_vf(cdev, num_vfs);
+ if (ret)
+ return ret;
+
+ ret = pci_enable_sriov(pcidev, num_vfs);
+ if (ret)
+ dfl_fpga_cdev_config_ports_pf(cdev);
+ }
+
+ return ret;
+}
+
static void cci_pci_remove(struct pci_dev *pcidev)
{
+ if (dev_is_pf(&pcidev->dev))
+ cci_pci_sriov_configure(pcidev, 0);
+
cci_remove_feature_devs(pcidev);
pci_disable_pcie_error_reporting(pcidev);
}
@@ -234,6 +269,7 @@ static void cci_pci_remove(struct pci_dev *pcidev)
.id_table = cci_pcie_id_tbl,
.probe = cci_pci_probe,
.remove = cci_pci_remove,
+ .sriov_configure = cci_pci_sriov_configure,
};

module_pci_driver(cci_pci_driver);
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 70ffe8b..b913704 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -1131,6 +1131,88 @@ int dfl_fpga_cdev_assign_port(struct dfl_fpga_cdev *cdev, int port_id)
}
EXPORT_SYMBOL_GPL(dfl_fpga_cdev_assign_port);

+static void config_port_access_mode(struct device *fme_dev, int port_id,
+ bool is_vf)
+{
+ void __iomem *base;
+ u64 v;
+
+ base = dfl_get_feature_ioaddr_by_id(fme_dev, FME_FEATURE_ID_HEADER);
+
+ v = readq(base + FME_HDR_PORT_OFST(port_id));
+
+ v &= ~FME_PORT_OFST_ACC_CTRL;
+ v |= FIELD_PREP(FME_PORT_OFST_ACC_CTRL,
+ is_vf ? FME_PORT_OFST_ACC_VF : FME_PORT_OFST_ACC_PF);
+
+ writeq(v, base + FME_HDR_PORT_OFST(port_id));
+}
+
+#define config_port_vf_mode(dev, id) config_port_access_mode(dev, id, true)
+#define config_port_pf_mode(dev, id) config_port_access_mode(dev, id, false)
+
+/**
+ * dfl_fpga_cdev_config_ports_pf - configure ports to PF access mode
+ *
+ * @cdev: parent container device.
+ *
+ * This function is needed in sriov configuration routine. It could be used to
+ * configure the all released ports from VF access mode to PF.
+ */
+void dfl_fpga_cdev_config_ports_pf(struct dfl_fpga_cdev *cdev)
+{
+ struct dfl_feature_platform_data *pdata;
+
+ mutex_lock(&cdev->lock);
+ list_for_each_entry(pdata, &cdev->port_dev_list, node) {
+ if (device_is_registered(&pdata->dev->dev))
+ continue;
+
+ config_port_pf_mode(cdev->fme_dev, pdata->id);
+ }
+ mutex_unlock(&cdev->lock);
+}
+EXPORT_SYMBOL_GPL(dfl_fpga_cdev_config_ports_pf);
+
+/**
+ * dfl_fpga_cdev_config_ports_vf - configure ports to VF access mode
+ *
+ * @cdev: parent container device.
+ * @num_vfs: VF device number.
+ *
+ * This function is needed in sriov configuration routine. It could be used to
+ * configure the released ports from PF access mode to VF.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int dfl_fpga_cdev_config_ports_vf(struct dfl_fpga_cdev *cdev, int num_vfs)
+{
+ struct dfl_feature_platform_data *pdata;
+ int ret = 0;
+
+ mutex_lock(&cdev->lock);
+ /*
+ * can't turn multiple ports into 1 VF device, only 1 port for 1 VF
+ * device, so if released port number doesn't match VF device number,
+ * then reject the request with -EINVAL error code.
+ */
+ if (cdev->released_port_num != num_vfs) {
+ ret = -EINVAL;
+ goto done;
+ }
+
+ list_for_each_entry(pdata, &cdev->port_dev_list, node) {
+ if (device_is_registered(&pdata->dev->dev))
+ continue;
+
+ config_port_vf_mode(cdev->fme_dev, pdata->id);
+ }
+done:
+ mutex_unlock(&cdev->lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dfl_fpga_cdev_config_ports_vf);
+
static void __exit dfl_fpga_exit(void)
{
dfl_chardev_uinit();
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 6f7855e..b3f2f53 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -416,5 +416,6 @@ struct platform_device *

int dfl_fpga_cdev_release_port(struct dfl_fpga_cdev *cdev, int port_id);
int dfl_fpga_cdev_assign_port(struct dfl_fpga_cdev *cdev, int port_id);
-
+void dfl_fpga_cdev_config_ports_pf(struct dfl_fpga_cdev *cdev);
+int dfl_fpga_cdev_config_ports_vf(struct dfl_fpga_cdev *cdev, int num_vf);
#endif /* __FPGA_DFL_H */
--
1.8.3.1

2019-08-04 10:40:07

by Wu Hao

Subject: [PATCH v4 12/12] Documentation: fpga: dfl: add descriptions for virtualization and new interfaces.

This patch adds virtualization support description for DFL based
FPGA devices (based on PCIe SRIOV), and introductions to new
interfaces added by new dfl private feature drivers.

[[email protected]: Fixed up to make it work with new reStructuredText docs]
Signed-off-by: Xu Yilun <[email protected]>
Signed-off-by: Wu Hao <[email protected]>
Acked-by: Alan Tull <[email protected]>
Signed-off-by: Moritz Fischer <[email protected]>
---
Documentation/fpga/dfl.rst | 105 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 105 insertions(+)

diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 2f125ab..6fa483f 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -87,6 +87,8 @@ The following functions are exposed through ioctls:
- Get driver API version (DFL_FPGA_GET_API_VERSION)
- Check for extensions (DFL_FPGA_CHECK_EXTENSION)
- Program bitstream (DFL_FPGA_FME_PORT_PR)
+- Assign port to PF (DFL_FPGA_FME_PORT_ASSIGN)
+- Release port from PF (DFL_FPGA_FME_PORT_RELEASE)

More functions are exposed through sysfs
(/sys/class/fpga_region/regionX/dfl-fme.n/):
@@ -102,6 +104,10 @@ More functions are exposed through sysfs
one FPGA device may have more than one port, this sysfs interface indicates
how many ports the FPGA device has.

+ Global error reporting management (errors/)
+ error reporting sysfs interfaces allow user to read errors detected by the
+ hardware, and clear the logged errors.
+

FIU - PORT
==========
@@ -143,6 +149,10 @@ More functions are exposed through sysfs:
Read Accelerator GUID (afu_id)
afu_id indicates which PR bitstream is programmed to this AFU.

+ Error reporting (errors/)
+ error reporting sysfs interfaces allow user to read port/afu errors
+ detected by the hardware, and clear the logged errors.
+

DFL Framework Overview
======================
@@ -218,6 +228,101 @@ the compat_id exposed by the target FPGA region. This check is usually done by
userspace before calling the reconfiguration IOCTL.


+FPGA virtualization - PCIe SRIOV
+================================
+This section describes the virtualization support on DFL based FPGA device to
+enable accessing an accelerator from applications running in a virtual machine
+(VM). This section only describes the PCIe based FPGA device with SRIOV support.
+
+Features supported by the particular FPGA device are exposed through Device
+Feature Lists, as illustrated below:
+
+::
+
+ +-------------------------------+ +-------------+
+ | PF | | VF |
+ +-------------------------------+ +-------------+
+ ^ ^ ^ ^
+ | | | |
+ +-----|------------|---------|--------------|-------+
+ | | | | | |
+ | +-----+ +-------+ +-------+ +-------+ |
+ | | FME | | Port0 | | Port1 | | Port2 | |
+ | +-----+ +-------+ +-------+ +-------+ |
+ | ^ ^ ^ |
+ | | | | |
+ | +-------+ +------+ +-------+ |
+ | | AFU | | AFU | | AFU | |
+ | +-------+ +------+ +-------+ |
+ | |
+ | DFL based FPGA PCIe Device |
+ +---------------------------------------------------+
+
+FME is always accessed through the physical function (PF).
+
+Ports (and related AFUs) are accessed via PF by default, but could be exposed
+through virtual function (VF) devices via PCIe SRIOV. Each VF only contains
+1 Port and 1 AFU for isolation. Users could assign individual VFs (accelerators)
+created via PCIe SRIOV interface, to virtual machines.
+
+The driver organization in virtualization case is illustrated below:
+::
+
+ +-------++------++------+ |
+ | FME || FME || FME | |
+ | FPGA || FPGA || FPGA | |
+ |Manager||Bridge||Region| |
+ +-------++------++------+ |
+ +-----------------------+ +--------+ | +--------+
+ | FME | | AFU | | | AFU |
+ | Module | | Module | | | Module |
+ +-----------------------+ +--------+ | +--------+
+ +-----------------------+ | +-----------------------+
+ | FPGA Container Device | | | FPGA Container Device |
+ | (FPGA Base Region) | | | (FPGA Base Region) |
+ +-----------------------+ | +-----------------------+
+ +------------------+ | +------------------+
+ | FPGA PCIE Module | | Virtual | FPGA PCIE Module |
+ +------------------+ Host | Machine +------------------+
+ -------------------------------------- | ------------------------------
+ +---------------+ | +---------------+
+ | PCI PF Device | | | PCI VF Device |
+ +---------------+ | +---------------+
+
+FPGA PCIe device driver is always loaded first once a FPGA PCIe PF or VF device
+is detected. It:
+
+* Finishes enumeration on both FPGA PCIe PF and VF device using common
+ interfaces from DFL framework.
+* Supports SRIOV.
+
+The FME device driver plays a management role in this driver architecture, it
+provides ioctls to release Port from PF and assign Port to PF. After release
+a port from PF, then it's safe to expose this port through a VF via PCIe SRIOV
+sysfs interface.
+
+To enable accessing an accelerator from applications running in a VM, the
+respective AFU's port needs to be assigned to a VF using the following steps:
+
+#. The PF owns all AFU ports by default. Any port that needs to be
+ reassigned to a VF must first be released through the
+ DFL_FPGA_FME_PORT_RELEASE ioctl on the FME device.
+
+#. Once N ports are released from PF, then user can use command below
+ to enable SRIOV and VFs. Each VF owns only one Port with AFU.
+
+ ::
+
+ echo N > $PCI_DEVICE_PATH/sriov_numvfs
+
+#. Pass through the VFs to VMs
+
+#. The AFU under VF is accessible from applications in VM (using the
+ same driver inside the VF).
+
+Note that an FME can't be assigned to a VF, thus PR and other management
+functions are only available via the PF.
+
Device enumeration
==================
This section introduces how applications enumerate the fpga device from
--
1.8.3.1

2019-08-04 10:40:07

by Wu Hao

Subject: [PATCH v4 11/12] fpga: dfl: fme: add global error reporting support

This patch adds support for global error reporting for FPGA
Management Engine (FME), it introduces sysfs interfaces to
report different error detected by the hardware, and allow
user to clear errors or inject error for testing purpose.

Signed-off-by: Luwei Kang <[email protected]>
Signed-off-by: Ananda Ravuri <[email protected]>
Signed-off-by: Xu Yilun <[email protected]>
Signed-off-by: Wu Hao <[email protected]>
Acked-by: Alan Tull <[email protected]>
Signed-off-by: Moritz Fischer <[email protected]>
---
v2: switch to device_add/remove_groups for sysfs.
v3: update kernel version and date in sysfs doc
v4: rebase, remove dev_dbg in init/uinit callback.
---
Documentation/ABI/testing/sysfs-platform-dfl-fme | 75 +++++
drivers/fpga/Makefile | 2 +-
drivers/fpga/dfl-fme-error.c | 381 +++++++++++++++++++++++
drivers/fpga/dfl-fme-main.c | 4 +
drivers/fpga/dfl-fme.h | 2 +
drivers/fpga/dfl.h | 2 +
6 files changed, 465 insertions(+), 1 deletion(-)
create mode 100644 drivers/fpga/dfl-fme-error.c

diff --git a/Documentation/ABI/testing/sysfs-platform-dfl-fme b/Documentation/ABI/testing/sysfs-platform-dfl-fme
index 65372aa..6e9e7d4 100644
--- a/Documentation/ABI/testing/sysfs-platform-dfl-fme
+++ b/Documentation/ABI/testing/sysfs-platform-dfl-fme
@@ -44,3 +44,78 @@ Description: Read-only. It returns socket_id to indicate which socket
this FPGA belongs to, only valid for integrated solution.
User only needs this information, in case standard numa node
can't provide correct information.
+
+What: /sys/bus/platform/devices/dfl-fme.0/errors/revision
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-only. Read this file to get the revision of this global
+ error reporting private feature.
+
+What: /sys/bus/platform/devices/dfl-fme.0/errors/pcie0_errors
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-Write. Read this file for errors detected on pcie0 link.
+ Write this file to clear errors logged in pcie0_errors. Write
+ fails with -EINVAL if input parsing fails or input error code
+ doesn't match.
+
+What: /sys/bus/platform/devices/dfl-fme.0/errors/pcie1_errors
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-Write. Read this file for errors detected on pcie1 link.
+ Write this file to clear errors logged in pcie1_errors. Write
+ fails with -EINVAL if input parsing fails or input error code
+ doesn't match.
+
+What: /sys/bus/platform/devices/dfl-fme.0/errors/nonfatal_errors
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-only. It returns non-fatal errors detected.
+
+What: /sys/bus/platform/devices/dfl-fme.0/errors/catfatal_errors
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-only. It returns catastrophic and fatal errors detected.
+
+What: /sys/bus/platform/devices/dfl-fme.0/errors/inject_error
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-Write. Read this file to check errors injected. Write this
+ file to inject errors for testing purpose. Write fails with
+ -EINVAL if input parsing fails or input inject error code isn't
+ supported.
+
+What: /sys/bus/platform/devices/dfl-fme.0/errors/fme-errors/errors
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-only. Read this file to get errors detected by hardware.
+
+What: /sys/bus/platform/devices/dfl-fme.0/errors/fme-errors/first_error
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-only. Read this file to get the first error detected by
+ hardware.
+
+What: /sys/bus/platform/devices/dfl-fme.0/errors/fme-errors/next_error
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-only. Read this file to get the second error detected by
+ hardware.
+
+What: /sys/bus/platform/devices/dfl-fme.0/errors/fme-errors/clear
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Write-only. Write error code to this file to clear all errors
+ logged in errors, first_error and next_error. Write fails with
+ -EINVAL if input parsing fails or input error code doesn't
+ match.
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 7255891..4865b74 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -39,7 +39,7 @@ obj-$(CONFIG_FPGA_DFL_FME_BRIDGE) += dfl-fme-br.o
obj-$(CONFIG_FPGA_DFL_FME_REGION) += dfl-fme-region.o
obj-$(CONFIG_FPGA_DFL_AFU) += dfl-afu.o

-dfl-fme-objs := dfl-fme-main.o dfl-fme-pr.o
+dfl-fme-objs := dfl-fme-main.o dfl-fme-pr.o dfl-fme-error.o
dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
dfl-afu-objs += dfl-afu-error.o

diff --git a/drivers/fpga/dfl-fme-error.c b/drivers/fpga/dfl-fme-error.c
new file mode 100644
index 0000000..8b2df80
--- /dev/null
+++ b/drivers/fpga/dfl-fme-error.c
@@ -0,0 +1,381 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for FPGA Management Engine Error Management
+ *
+ * Copyright 2019 Intel Corporation, Inc.
+ *
+ * Authors:
+ * Kang Luwei <[email protected]>
+ * Xiao Guangrong <[email protected]>
+ * Wu Hao <[email protected]>
+ * Joseph Grecco <[email protected]>
+ * Enno Luebbers <[email protected]>
+ * Tim Whisonant <[email protected]>
+ * Ananda Ravuri <[email protected]>
+ * Mitchel, Henry <[email protected]>
+ */
+
+#include <linux/uaccess.h>
+
+#include "dfl.h"
+#include "dfl-fme.h"
+
+#define FME_ERROR_MASK 0x8
+#define FME_ERROR 0x10
+#define MBP_ERROR BIT_ULL(6)
+#define PCIE0_ERROR_MASK 0x18
+#define PCIE0_ERROR 0x20
+#define PCIE1_ERROR_MASK 0x28
+#define PCIE1_ERROR 0x30
+#define FME_FIRST_ERROR 0x38
+#define FME_NEXT_ERROR 0x40
+#define RAS_NONFAT_ERROR_MASK 0x48
+#define RAS_NONFAT_ERROR 0x50
+#define RAS_CATFAT_ERROR_MASK 0x58
+#define RAS_CATFAT_ERROR 0x60
+#define RAS_ERROR_INJECT 0x68
+#define INJECT_ERROR_MASK GENMASK_ULL(2, 0)
+
+static ssize_t revision_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct device *err_dev = dev->parent;
+ void __iomem *base;
+
+ base = dfl_get_feature_ioaddr_by_id(err_dev, FME_FEATURE_ID_GLOBAL_ERR);
+
+ return sprintf(buf, "%u\n", dfl_feature_revision(base));
+}
+static DEVICE_ATTR_RO(revision);
+
+static ssize_t pcie0_errors_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct device *err_dev = dev->parent;
+ void __iomem *base;
+
+ base = dfl_get_feature_ioaddr_by_id(err_dev, FME_FEATURE_ID_GLOBAL_ERR);
+
+ return sprintf(buf, "0x%llx\n",
+ (unsigned long long)readq(base + PCIE0_ERROR));
+}
+
+static ssize_t pcie0_errors_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(dev->parent);
+ struct device *err_dev = dev->parent;
+ void __iomem *base;
+ int ret = 0;
+ u64 v, val;
+
+ if (kstrtou64(buf, 0, &val))
+ return -EINVAL;
+
+ base = dfl_get_feature_ioaddr_by_id(err_dev, FME_FEATURE_ID_GLOBAL_ERR);
+
+ mutex_lock(&pdata->lock);
+ writeq(GENMASK_ULL(63, 0), base + PCIE0_ERROR_MASK);
+
+ v = readq(base + PCIE0_ERROR);
+ if (val == v)
+ writeq(v, base + PCIE0_ERROR);
+ else
+ ret = -EINVAL;
+
+ writeq(0ULL, base + PCIE0_ERROR_MASK);
+ mutex_unlock(&pdata->lock);
+ return ret ? ret : count;
+}
+static DEVICE_ATTR_RW(pcie0_errors);
+
+static ssize_t pcie1_errors_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct device *err_dev = dev->parent;
+ void __iomem *base;
+
+ base = dfl_get_feature_ioaddr_by_id(err_dev, FME_FEATURE_ID_GLOBAL_ERR);
+
+ return sprintf(buf, "0x%llx\n",
+ (unsigned long long)readq(base + PCIE1_ERROR));
+}
+
+static ssize_t pcie1_errors_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(dev->parent);
+ struct device *err_dev = dev->parent;
+ void __iomem *base;
+ int ret = 0;
+ u64 v, val;
+
+ if (kstrtou64(buf, 0, &val))
+ return -EINVAL;
+
+ base = dfl_get_feature_ioaddr_by_id(err_dev, FME_FEATURE_ID_GLOBAL_ERR);
+
+ mutex_lock(&pdata->lock);
+ writeq(GENMASK_ULL(63, 0), base + PCIE1_ERROR_MASK);
+
+ v = readq(base + PCIE1_ERROR);
+ if (val == v)
+ writeq(v, base + PCIE1_ERROR);
+ else
+ ret = -EINVAL;
+
+ writeq(0ULL, base + PCIE1_ERROR_MASK);
+ mutex_unlock(&pdata->lock);
+ return ret ? ret : count;
+}
+static DEVICE_ATTR_RW(pcie1_errors);
+
+static ssize_t nonfatal_errors_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct device *err_dev = dev->parent;
+ void __iomem *base;
+
+ base = dfl_get_feature_ioaddr_by_id(err_dev, FME_FEATURE_ID_GLOBAL_ERR);
+
+ return sprintf(buf, "0x%llx\n",
+ (unsigned long long)readq(base + RAS_NONFAT_ERROR));
+}
+static DEVICE_ATTR_RO(nonfatal_errors);
+
+static ssize_t catfatal_errors_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct device *err_dev = dev->parent;
+ void __iomem *base;
+
+ base = dfl_get_feature_ioaddr_by_id(err_dev, FME_FEATURE_ID_GLOBAL_ERR);
+
+ return sprintf(buf, "0x%llx\n",
+ (unsigned long long)readq(base + RAS_CATFAT_ERROR));
+}
+static DEVICE_ATTR_RO(catfatal_errors);
+
+static ssize_t inject_error_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct device *err_dev = dev->parent;
+ void __iomem *base;
+ u64 v;
+
+ base = dfl_get_feature_ioaddr_by_id(err_dev, FME_FEATURE_ID_GLOBAL_ERR);
+
+ v = readq(base + RAS_ERROR_INJECT);
+
+ return sprintf(buf, "0x%llx\n",
+ (unsigned long long)FIELD_GET(INJECT_ERROR_MASK, v));
+}
+
+static ssize_t inject_error_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(dev->parent);
+ struct device *err_dev = dev->parent;
+ void __iomem *base;
+ u8 inject_error;
+ u64 v;
+
+ if (kstrtou8(buf, 0, &inject_error))
+ return -EINVAL;
+
+ if (inject_error & ~INJECT_ERROR_MASK)
+ return -EINVAL;
+
+ base = dfl_get_feature_ioaddr_by_id(err_dev, FME_FEATURE_ID_GLOBAL_ERR);
+
+ mutex_lock(&pdata->lock);
+ v = readq(base + RAS_ERROR_INJECT);
+ v &= ~INJECT_ERROR_MASK;
+ v |= FIELD_PREP(INJECT_ERROR_MASK, inject_error);
+ writeq(v, base + RAS_ERROR_INJECT);
+ mutex_unlock(&pdata->lock);
+
+ return count;
+}
+static DEVICE_ATTR_RW(inject_error);
+
+static struct attribute *errors_attrs[] = {
+ &dev_attr_revision.attr,
+ &dev_attr_pcie0_errors.attr,
+ &dev_attr_pcie1_errors.attr,
+ &dev_attr_nonfatal_errors.attr,
+ &dev_attr_catfatal_errors.attr,
+ &dev_attr_inject_error.attr,
+ NULL,
+};
+
+static struct attribute_group errors_attr_group = {
+ .attrs = errors_attrs,
+};
+
+static ssize_t errors_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct device *err_dev = dev->parent;
+ void __iomem *base;
+
+ base = dfl_get_feature_ioaddr_by_id(err_dev, FME_FEATURE_ID_GLOBAL_ERR);
+
+ return sprintf(buf, "0x%llx\n",
+ (unsigned long long)readq(base + FME_ERROR));
+}
+static DEVICE_ATTR_RO(errors);
+
+static ssize_t first_error_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct device *err_dev = dev->parent;
+ void __iomem *base;
+
+ base = dfl_get_feature_ioaddr_by_id(err_dev, FME_FEATURE_ID_GLOBAL_ERR);
+
+ return sprintf(buf, "0x%llx\n",
+ (unsigned long long)readq(base + FME_FIRST_ERROR));
+}
+static DEVICE_ATTR_RO(first_error);
+
+static ssize_t next_error_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct device *err_dev = dev->parent;
+ void __iomem *base;
+
+ base = dfl_get_feature_ioaddr_by_id(err_dev, FME_FEATURE_ID_GLOBAL_ERR);
+
+ return sprintf(buf, "0x%llx\n",
+ (unsigned long long)readq(base + FME_NEXT_ERROR));
+}
+static DEVICE_ATTR_RO(next_error);
+
+static ssize_t clear_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(dev->parent);
+ struct device *err_dev = dev->parent;
+ void __iomem *base;
+ u64 v, val;
+ int ret = 0;
+
+ if (kstrtou64(buf, 0, &val))
+ return -EINVAL;
+
+ base = dfl_get_feature_ioaddr_by_id(err_dev, FME_FEATURE_ID_GLOBAL_ERR);
+
+ mutex_lock(&pdata->lock);
+ writeq(GENMASK_ULL(63, 0), base + FME_ERROR_MASK);
+
+ v = readq(base + FME_ERROR);
+ if (val == v) {
+ writeq(v, base + FME_ERROR);
+ v = readq(base + FME_FIRST_ERROR);
+ writeq(v, base + FME_FIRST_ERROR);
+ v = readq(base + FME_NEXT_ERROR);
+ writeq(v, base + FME_NEXT_ERROR);
+ } else {
+ ret = -EINVAL;
+ }
+
+ /* Workaround: disable MBP_ERROR if feature revision is 0 */
+ writeq(dfl_feature_revision(base) ? 0ULL : MBP_ERROR,
+ base + FME_ERROR_MASK);
+ mutex_unlock(&pdata->lock);
+ return ret ? ret : count;
+}
+static DEVICE_ATTR_WO(clear);
+
+static struct attribute *fme_errors_attrs[] = {
+ &dev_attr_errors.attr,
+ &dev_attr_first_error.attr,
+ &dev_attr_next_error.attr,
+ &dev_attr_clear.attr,
+ NULL,
+};
+
+static struct attribute_group fme_errors_attr_group = {
+ .attrs = fme_errors_attrs,
+ .name = "fme-errors",
+};
+
+static const struct attribute_group *error_groups[] = {
+ &fme_errors_attr_group,
+ &errors_attr_group,
+ NULL
+};
+
+static void fme_error_enable(struct dfl_feature *feature)
+{
+ void __iomem *base = feature->ioaddr;
+
+ /* Workaround: disable MBP_ERROR if revision is 0 */
+ writeq(dfl_feature_revision(feature->ioaddr) ? 0ULL : MBP_ERROR,
+ base + FME_ERROR_MASK);
+ writeq(0ULL, base + PCIE0_ERROR_MASK);
+ writeq(0ULL, base + PCIE1_ERROR_MASK);
+ writeq(0ULL, base + RAS_NONFAT_ERROR_MASK);
+ writeq(0ULL, base + RAS_CATFAT_ERROR_MASK);
+}
+
+static void err_dev_release(struct device *dev)
+{
+ kfree(dev);
+}
+
+static int fme_global_err_init(struct platform_device *pdev,
+ struct dfl_feature *feature)
+{
+ struct device *dev;
+ int ret = 0;
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev)
+ return -ENOMEM;
+
+ dev->parent = &pdev->dev;
+ dev->release = err_dev_release;
+ dev_set_name(dev, "errors");
+
+ fme_error_enable(feature);
+
+ ret = device_register(dev);
+ if (ret) {
+ put_device(dev);
+ return ret;
+ }
+
+ ret = device_add_groups(dev, error_groups);
+ if (ret) {
+ device_unregister(dev);
+ return ret;
+ }
+
+ feature->priv = dev;
+
+ return ret;
+}
+
+static void fme_global_err_uinit(struct platform_device *pdev,
+ struct dfl_feature *feature)
+{
+ struct device *dev = feature->priv;
+
+ device_remove_groups(dev, error_groups);
+ device_unregister(dev);
+}
+
+const struct dfl_feature_id fme_global_err_id_table[] = {
+ {.id = FME_FEATURE_ID_GLOBAL_ERR,},
+ {0,}
+};
+
+const struct dfl_feature_ops fme_global_err_ops = {
+ .init = fme_global_err_init,
+ .uinit = fme_global_err_uinit,
+};
diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
index f033f1c..59bc2cc 100644
--- a/drivers/fpga/dfl-fme-main.c
+++ b/drivers/fpga/dfl-fme-main.c
@@ -214,6 +214,10 @@ static long fme_hdr_ioctl(struct platform_device *pdev,
.ops = &fme_pr_mgmt_ops,
},
{
+ .id_table = fme_global_err_id_table,
+ .ops = &fme_global_err_ops,
+ },
+ {
.ops = NULL,
},
};
diff --git a/drivers/fpga/dfl-fme.h b/drivers/fpga/dfl-fme.h
index e4131e8..a3a48c8 100644
--- a/drivers/fpga/dfl-fme.h
+++ b/drivers/fpga/dfl-fme.h
@@ -35,5 +35,7 @@ struct dfl_fme {

extern const struct dfl_feature_ops fme_pr_mgmt_ops;
extern const struct dfl_feature_id fme_pr_mgmt_id_table[];
+extern const struct dfl_feature_ops fme_global_err_ops;
+extern const struct dfl_feature_id fme_global_err_id_table[];

#endif /* __DFL_FME_H */
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 9f0e656..d312678 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -197,12 +197,14 @@ struct dfl_feature_driver {
* feature dev (platform device)'s reources.
* @ioaddr: mapped mmio resource address.
* @ops: ops of this sub feature.
+ * @priv: priv data of this feature.
*/
struct dfl_feature {
u64 id;
int resource_index;
void __iomem *ioaddr;
const struct dfl_feature_ops *ops;
+ void *priv;
};

#define DEV_STATUS_IN_USE 0
--
1.8.3.1

2019-08-04 10:40:07

by Wu Hao

Subject: [PATCH v4 04/12] fpga: dfl: afu: add userclock sysfs interfaces.

This patch introduces userclock sysfs interfaces for AFU, user
could use these interfaces for clock setting to AFU.

Please note that, this is only working for port header feature
with revision 0, for later revisions, userclock setting is moved
to a separated private feature, so one revision sysfs interface
is exposed to userspace application for this purpose too.

Signed-off-by: Ananda Ravuri <[email protected]>
Signed-off-by: Russ Weight <[email protected]>
Signed-off-by: Xu Yilun <[email protected]>
Signed-off-by: Wu Hao <[email protected]>
Acked-by: Alan Tull <[email protected]>
Signed-off-by: Moritz Fischer <[email protected]>
---
v2: rebased, and switched to use device_add/remove_groups for sysfs
v3: update kernel version and date in sysfs doc
v4: rebased.
---
Documentation/ABI/testing/sysfs-platform-dfl-port | 35 +++++++
drivers/fpga/dfl-afu-main.c | 114 +++++++++++++++++++++-
drivers/fpga/dfl.h | 9 ++
3 files changed, 157 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-platform-dfl-port b/Documentation/ABI/testing/sysfs-platform-dfl-port
index 1ab3e6f..5663441 100644
--- a/Documentation/ABI/testing/sysfs-platform-dfl-port
+++ b/Documentation/ABI/testing/sysfs-platform-dfl-port
@@ -46,3 +46,38 @@ Contact: Wu Hao <[email protected]>
Description: Read-write. Read or set AFU latency tolerance reporting value.
Set ltr to 1 if the AFU can tolerate latency >= 40us or set it
to 0 if it is latency sensitive.
+
+What: /sys/bus/platform/devices/dfl-port.0/revision
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-only. Read this file to get the revision of port header
+ feature.
+
+What: /sys/bus/platform/devices/dfl-port.0/userclk_freqcmd
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Write-only. User writes command to this interface to set
+ userclock to AFU.
+
+What: /sys/bus/platform/devices/dfl-port.0/userclk_freqsts
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-only. Read this file to get the status of issued command
+ to userclck_freqcmd.
+
+What: /sys/bus/platform/devices/dfl-port.0/userclk_freqcntrcmd
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Write-only. User writes command to this interface to set
+ userclock counter.
+
+What: /sys/bus/platform/devices/dfl-port.0/userclk_freqcntrsts
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-only. Read this file to get the status of issued command
+ to userclck_freqcntrcmd.
diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
index 12175bb..407c97d 100644
--- a/drivers/fpga/dfl-afu-main.c
+++ b/drivers/fpga/dfl-afu-main.c
@@ -142,6 +142,17 @@ static int port_get_id(struct platform_device *pdev)
static DEVICE_ATTR_RO(id);

static ssize_t
+revision_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ void __iomem *base;
+
+ base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
+
+ return sprintf(buf, "%x\n", dfl_feature_revision(base));
+}
+static DEVICE_ATTR_RO(revision);
+
+static ssize_t
ltr_show(struct device *dev, struct device_attribute *attr, char *buf)
{
struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
@@ -276,6 +287,7 @@ static int port_get_id(struct platform_device *pdev)

static struct attribute *port_hdr_attrs[] = {
&dev_attr_id.attr,
+ &dev_attr_revision.attr,
&dev_attr_ltr.attr,
&dev_attr_ap1_event.attr,
&dev_attr_ap2_event.attr,
@@ -284,14 +296,113 @@ static int port_get_id(struct platform_device *pdev)
};
ATTRIBUTE_GROUPS(port_hdr);

+static ssize_t
+userclk_freqcmd_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
+ u64 userclk_freq_cmd;
+ void __iomem *base;
+
+ if (kstrtou64(buf, 0, &userclk_freq_cmd))
+ return -EINVAL;
+
+ base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
+
+ mutex_lock(&pdata->lock);
+ writeq(userclk_freq_cmd, base + PORT_HDR_USRCLK_CMD0);
+ mutex_unlock(&pdata->lock);
+
+ return count;
+}
+static DEVICE_ATTR_WO(userclk_freqcmd);
+
+static ssize_t
+userclk_freqcntrcmd_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
+ u64 userclk_freqcntr_cmd;
+ void __iomem *base;
+
+ if (kstrtou64(buf, 0, &userclk_freqcntr_cmd))
+ return -EINVAL;
+
+ base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
+
+ mutex_lock(&pdata->lock);
+ writeq(userclk_freqcntr_cmd, base + PORT_HDR_USRCLK_CMD1);
+ mutex_unlock(&pdata->lock);
+
+ return count;
+}
+static DEVICE_ATTR_WO(userclk_freqcntrcmd);
+
+static ssize_t
+userclk_freqsts_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ u64 userclk_freqsts;
+ void __iomem *base;
+
+ base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
+
+ userclk_freqsts = readq(base + PORT_HDR_USRCLK_STS0);
+
+ return sprintf(buf, "0x%llx\n", (unsigned long long)userclk_freqsts);
+}
+static DEVICE_ATTR_RO(userclk_freqsts);
+
+static ssize_t
+userclk_freqcntrsts_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ u64 userclk_freqcntrsts;
+ void __iomem *base;
+
+ base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
+
+ userclk_freqcntrsts = readq(base + PORT_HDR_USRCLK_STS1);
+
+ return sprintf(buf, "0x%llx\n",
+ (unsigned long long)userclk_freqcntrsts);
+}
+static DEVICE_ATTR_RO(userclk_freqcntrsts);
+
+static struct attribute *port_hdr_userclk_attrs[] = {
+ &dev_attr_userclk_freqcmd.attr,
+ &dev_attr_userclk_freqcntrcmd.attr,
+ &dev_attr_userclk_freqsts.attr,
+ &dev_attr_userclk_freqcntrsts.attr,
+ NULL,
+};
+ATTRIBUTE_GROUPS(port_hdr_userclk);
+
static int port_hdr_init(struct platform_device *pdev,
struct dfl_feature *feature)
{
+ int ret;
+
dev_dbg(&pdev->dev, "PORT HDR Init.\n");

port_reset(pdev);

- return device_add_groups(&pdev->dev, port_hdr_groups);
+ ret = device_add_groups(&pdev->dev, port_hdr_groups);
+ if (ret)
+ return ret;
+
+ /*
+ * if revision > 0, the userclock will be moved from port hdr register
+ * region to a separated private feature.
+ */
+ if (dfl_feature_revision(feature->ioaddr) > 0)
+ return 0;
+
+ ret = device_add_groups(&pdev->dev, port_hdr_userclk_groups);
+ if (ret)
+ device_remove_groups(&pdev->dev, port_hdr_groups);
+
+ return ret;
}

static void port_hdr_uinit(struct platform_device *pdev,
@@ -299,6 +410,7 @@ static void port_hdr_uinit(struct platform_device *pdev,
{
dev_dbg(&pdev->dev, "PORT HDR UInit.\n");

+ device_remove_groups(&pdev->dev, port_hdr_userclk_groups);
device_remove_groups(&pdev->dev, port_hdr_groups);
}

diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 6625d73..c65ab29 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -120,6 +120,10 @@
#define PORT_HDR_CAP 0x30
#define PORT_HDR_CTRL 0x38
#define PORT_HDR_STS 0x40
+#define PORT_HDR_USRCLK_CMD0 0x50
+#define PORT_HDR_USRCLK_CMD1 0x58
+#define PORT_HDR_USRCLK_STS0 0x60
+#define PORT_HDR_USRCLK_STS1 0x68

/* Port Capability Register Bitfield */
#define PORT_CAP_PORT_NUM GENMASK_ULL(1, 0) /* ID of this port */
@@ -346,6 +350,11 @@ static inline bool dfl_feature_is_port(void __iomem *base)
(FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
}

+static inline u8 dfl_feature_revision(void __iomem *base)
+{
+ return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
+}
+
/**
* struct dfl_fpga_enum_info - DFL FPGA enumeration information
*
--
1.8.3.1

2019-08-04 10:40:07

by Wu Hao

Subject: [PATCH v4 01/12] fpga: dfl: fme: add DFL_FPGA_FME_PORT_RELEASE/ASSIGN ioctl support.

In order to support virtualization usage via PCIe SRIOV, this patch
adds two ioctls under FPGA Management Engine (FME) to release and
assign back the port device. In order to safely turn Port from PF
into VF and enable PCIe SRIOV, it requires user to invoke this
PORT_RELEASE ioctl to release port firstly to remove userspace
interfaces, and then configure the PF/VF access register in FME.
After disable SRIOV, it requires user to invoke this PORT_ASSIGN
ioctl to attach the port back to PF.

Ioctl interfaces:
* DFL_FPGA_FME_PORT_RELEASE
Release platform device of given port, it deletes port platform
device to remove related userspace interfaces on PF. After this
function, then it's safe to configure PF/VF access mode to VF,
and enable VFs via SRIOV.

* DFL_FPGA_FME_PORT_ASSIGN
Assign platform device of given port back to PF. After configure
PF/VF access mode to PF, this ioctl adds port platform device
back to re-enable related userspace interfaces on PF.

Signed-off-by: Zhang Yi Z <[email protected]>
Signed-off-by: Xu Yilun <[email protected]>
Signed-off-by: Wu Hao <[email protected]>
Acked-by: Alan Tull <[email protected]>
Acked-by: Moritz Fischer <[email protected]>
Signed-off-by: Moritz Fischer <[email protected]>
---
v2: remove argsz from ioctls.
v4: split dfl_fpga_cdev_config_port to 2 functions *release/assign_port.
---
drivers/fpga/dfl-fme-main.c | 42 ++++++++++++++++
drivers/fpga/dfl.c | 113 +++++++++++++++++++++++++++++++++++++-----
drivers/fpga/dfl.h | 10 ++++
include/uapi/linux/fpga-dfl.h | 18 +++++++
4 files changed, 171 insertions(+), 12 deletions(-)

diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
index 0be4635..dfea2de 100644
--- a/drivers/fpga/dfl-fme-main.c
+++ b/drivers/fpga/dfl-fme-main.c
@@ -16,6 +16,7 @@

#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/uaccess.h>
#include <linux/fpga-dfl.h>

#include "dfl.h"
@@ -104,9 +105,50 @@ static void fme_hdr_uinit(struct platform_device *pdev,
device_remove_groups(&pdev->dev, fme_hdr_groups);
}

+static long fme_hdr_ioctl_release_port(struct dfl_feature_platform_data *pdata,
+ unsigned long arg)
+{
+ struct dfl_fpga_cdev *cdev = pdata->dfl_cdev;
+ int port_id;
+
+ if (get_user(port_id, (int __user *)arg))
+ return -EFAULT;
+
+ return dfl_fpga_cdev_release_port(cdev, port_id);
+}
+
+static long fme_hdr_ioctl_assign_port(struct dfl_feature_platform_data *pdata,
+ unsigned long arg)
+{
+ struct dfl_fpga_cdev *cdev = pdata->dfl_cdev;
+ int port_id;
+
+ if (get_user(port_id, (int __user *)arg))
+ return -EFAULT;
+
+ return dfl_fpga_cdev_assign_port(cdev, port_id);
+}
+
+static long fme_hdr_ioctl(struct platform_device *pdev,
+ struct dfl_feature *feature,
+ unsigned int cmd, unsigned long arg)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
+
+ switch (cmd) {
+ case DFL_FPGA_FME_PORT_RELEASE:
+ return fme_hdr_ioctl_release_port(pdata, arg);
+ case DFL_FPGA_FME_PORT_ASSIGN:
+ return fme_hdr_ioctl_assign_port(pdata, arg);
+ }
+
+ return -ENODEV;
+}
+
static const struct dfl_feature_ops fme_hdr_ops = {
.init = fme_hdr_init,
.uinit = fme_hdr_uinit,
+ .ioctl = fme_hdr_ioctl,
};

static struct dfl_feature_driver fme_feature_drvs[] = {
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 4b66aaa..70ffe8b 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -231,16 +231,20 @@ void dfl_fpga_port_ops_del(struct dfl_fpga_port_ops *ops)
*/
int dfl_fpga_check_port_id(struct platform_device *pdev, void *pport_id)
{
- struct dfl_fpga_port_ops *port_ops = dfl_fpga_port_ops_get(pdev);
- int port_id;
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
+ struct dfl_fpga_port_ops *port_ops;
+
+ if (pdata->id != FEATURE_DEV_ID_UNUSED)
+ return pdata->id == *(int *)pport_id;

+ port_ops = dfl_fpga_port_ops_get(pdev);
if (!port_ops || !port_ops->get_id)
return 0;

- port_id = port_ops->get_id(pdev);
+ pdata->id = port_ops->get_id(pdev);
dfl_fpga_port_ops_put(port_ops);

- return port_id == *(int *)pport_id;
+ return pdata->id == *(int *)pport_id;
}
EXPORT_SYMBOL_GPL(dfl_fpga_check_port_id);

@@ -474,6 +478,7 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
pdata->dev = fdev;
pdata->num = binfo->feature_num;
pdata->dfl_cdev = binfo->cdev;
+ pdata->id = FEATURE_DEV_ID_UNUSED;
mutex_init(&pdata->lock);
lockdep_set_class_and_name(&pdata->lock, &dfl_pdata_keys[type],
dfl_pdata_key_strings[type]);
@@ -973,25 +978,27 @@ void dfl_fpga_feature_devs_remove(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));
+ if (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));
+ /* remove released ports */
+ if (!device_is_registered(&port_dev->dev)) {
+ dfl_id_free(feature_dev_id_type(port_dev),
+ port_dev->id);
+ platform_device_put(port_dev);
+ }
+
list_del(&pdata->node);
put_device(&port_dev->dev);
}
mutex_unlock(&cdev->lock);

+ remove_feature_devs(cdev);
+
fpga_region_unregister(cdev->region);
devm_kfree(cdev->parent, cdev);
}
@@ -1042,6 +1049,88 @@ static int __init dfl_fpga_init(void)
return ret;
}

+/**
+ * dfl_fpga_cdev_release_port - release a port platform device
+ *
+ * @cdev: parent container device.
+ * @port_id: id of the port platform device.
+ *
+ * This function allows user to release a port platform device. This is a
+ * mandatory step before turn a port from PF into VF for SRIOV support.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int dfl_fpga_cdev_release_port(struct dfl_fpga_cdev *cdev, int port_id)
+{
+ struct platform_device *port_pdev;
+ int ret = -ENODEV;
+
+ mutex_lock(&cdev->lock);
+ port_pdev = __dfl_fpga_cdev_find_port(cdev, &port_id,
+ dfl_fpga_check_port_id);
+ if (!port_pdev)
+ goto unlock_exit;
+
+ if (!device_is_registered(&port_pdev->dev)) {
+ ret = -EBUSY;
+ goto put_dev_exit;
+ }
+
+ ret = dfl_feature_dev_use_begin(dev_get_platdata(&port_pdev->dev));
+ if (ret)
+ goto put_dev_exit;
+
+ platform_device_del(port_pdev);
+ cdev->released_port_num++;
+put_dev_exit:
+ put_device(&port_pdev->dev);
+unlock_exit:
+ mutex_unlock(&cdev->lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dfl_fpga_cdev_release_port);
+
+/**
+ * dfl_fpga_cdev_assign_port - assign a port platform device back
+ *
+ * @cdev: parent container device.
+ * @port_id: id of the port platform device.
+ *
+ * This function allows user to assign a port platform device back. This is
+ * a mandatory step after disable SRIOV support.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int dfl_fpga_cdev_assign_port(struct dfl_fpga_cdev *cdev, int port_id)
+{
+ struct platform_device *port_pdev;
+ int ret = -ENODEV;
+
+ mutex_lock(&cdev->lock);
+ port_pdev = __dfl_fpga_cdev_find_port(cdev, &port_id,
+ dfl_fpga_check_port_id);
+ if (!port_pdev)
+ goto unlock_exit;
+
+ if (device_is_registered(&port_pdev->dev)) {
+ ret = -EBUSY;
+ goto put_dev_exit;
+ }
+
+ ret = platform_device_add(port_pdev);
+ if (ret)
+ goto put_dev_exit;
+
+ dfl_feature_dev_use_end(dev_get_platdata(&port_pdev->dev));
+ cdev->released_port_num--;
+put_dev_exit:
+ put_device(&port_pdev->dev);
+unlock_exit:
+ mutex_unlock(&cdev->lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dfl_fpga_cdev_assign_port);
+
static void __exit dfl_fpga_exit(void)
{
dfl_chardev_uinit();
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index a8b869e..6f7855e 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -183,6 +183,8 @@ struct dfl_feature {

#define DEV_STATUS_IN_USE 0

+#define FEATURE_DEV_ID_UNUSED (-1)
+
/**
* struct dfl_feature_platform_data - platform data for feature devices
*
@@ -191,6 +193,7 @@ struct dfl_feature {
* @cdev: cdev of feature dev.
* @dev: ptr to platform device linked with this platform data.
* @dfl_cdev: ptr to container device.
+ * @id: id used for this feature device.
* @disable_count: count for port disable.
* @num: number for sub features.
* @dev_status: dev status (e.g. DEV_STATUS_IN_USE).
@@ -203,6 +206,7 @@ struct dfl_feature_platform_data {
struct cdev cdev;
struct platform_device *dev;
struct dfl_fpga_cdev *dfl_cdev;
+ int id;
unsigned int disable_count;
unsigned long dev_status;
void *private;
@@ -373,6 +377,7 @@ int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
* @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.
+ * @released_port_num: released port number under this container device.
*/
struct dfl_fpga_cdev {
struct device *parent;
@@ -380,6 +385,7 @@ struct dfl_fpga_cdev {
struct device *fme_dev;
struct mutex lock;
struct list_head port_dev_list;
+ int released_port_num;
};

struct dfl_fpga_cdev *
@@ -407,4 +413,8 @@ struct platform_device *

return pdev;
}
+
+int dfl_fpga_cdev_release_port(struct dfl_fpga_cdev *cdev, int port_id);
+int dfl_fpga_cdev_assign_port(struct dfl_fpga_cdev *cdev, int port_id);
+
#endif /* __FPGA_DFL_H */
diff --git a/include/uapi/linux/fpga-dfl.h b/include/uapi/linux/fpga-dfl.h
index 2e324e5..ec70a0746 100644
--- a/include/uapi/linux/fpga-dfl.h
+++ b/include/uapi/linux/fpga-dfl.h
@@ -176,4 +176,22 @@ struct dfl_fpga_fme_port_pr {

#define DFL_FPGA_FME_PORT_PR _IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 0)

+/**
+ * DFL_FPGA_FME_PORT_RELEASE - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 1,
+ * int port_id)
+ *
+ * Driver releases the port per Port ID provided by caller.
+ * Return: 0 on success, -errno on failure.
+ */
+#define DFL_FPGA_FME_PORT_RELEASE _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 1, int)
+
+/**
+ * DFL_FPGA_FME_PORT_ASSIGN - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 2,
+ * int port_id)
+ *
+ * Driver assigns the port back per Port ID provided by caller.
+ * Return: 0 on success, -errno on failure.
+ */
+#define DFL_FPGA_FME_PORT_ASSIGN _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 2, int)
+
#endif /* _UAPI_LINUX_FPGA_DFL_H */
--
1.8.3.1

2019-08-04 10:40:07

by Wu Hao

Subject: [PATCH v4 07/12] fpga: dfl: afu: add error reporting support.

Error reporting is one important private feature, it reports error
detected on port and accelerated function unit (AFU). It introduces
several sysfs interfaces to allow userspace to check and clear
errors detected by hardware.

Signed-off-by: Xu Yilun <[email protected]>
Signed-off-by: Wu Hao <[email protected]>
Acked-by: Alan Tull <[email protected]>
Signed-off-by: Moritz Fischer <[email protected]>
---
v2: switch to device_add/remove_group for sysfs.
v3: update kernel version and date in sysfs doc
v4: remove dev_dbg in init/uinit callback function.
---
Documentation/ABI/testing/sysfs-platform-dfl-port | 39 ++++
drivers/fpga/Makefile | 1 +
drivers/fpga/dfl-afu-error.c | 221 ++++++++++++++++++++++
drivers/fpga/dfl-afu-main.c | 4 +
drivers/fpga/dfl-afu.h | 4 +
5 files changed, 269 insertions(+)
create mode 100644 drivers/fpga/dfl-afu-error.c

diff --git a/Documentation/ABI/testing/sysfs-platform-dfl-port b/Documentation/ABI/testing/sysfs-platform-dfl-port
index 5663441..3b6580b 100644
--- a/Documentation/ABI/testing/sysfs-platform-dfl-port
+++ b/Documentation/ABI/testing/sysfs-platform-dfl-port
@@ -81,3 +81,42 @@ KernelVersion: 5.4
Contact: Wu Hao <[email protected]>
Description: Read-only. Read this file to get the status of issued command
to userclck_freqcntrcmd.
+
+What: /sys/bus/platform/devices/dfl-port.0/errors/revision
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-only. Read this file to get the revision of this error
+ reporting private feature.
+
+What: /sys/bus/platform/devices/dfl-port.0/errors/errors
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-only. Read this file to get errors detected on port and
+ Accelerated Function Unit (AFU).
+
+What: /sys/bus/platform/devices/dfl-port.0/errors/first_error
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-only. Read this file to get the first error detected by
+ hardware.
+
+What: /sys/bus/platform/devices/dfl-port.0/errors/first_malformed_req
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-only. Read this file to get the first malformed request
+ captured by hardware.
+
+What: /sys/bus/platform/devices/dfl-port.0/errors/clear
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Write-only. Write error code to this file to clear errors.
+ Write fails with -EINVAL if input parsing fails or input error
+ code doesn't match.
+ Write fails with -EBUSY or -ETIMEDOUT if error can't be cleared
+ as hardware is in low power state (-EBUSY) or not responding
+ (-ETIMEDOUT).
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 312b937..7255891 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_FPGA_DFL_AFU) += dfl-afu.o

dfl-fme-objs := dfl-fme-main.o dfl-fme-pr.o
dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
+dfl-afu-objs += dfl-afu-error.o

# Drivers for FPGAs which implement DFL
obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
diff --git a/drivers/fpga/dfl-afu-error.c b/drivers/fpga/dfl-afu-error.c
new file mode 100644
index 0000000..c5e0efa
--- /dev/null
+++ b/drivers/fpga/dfl-afu-error.c
@@ -0,0 +1,221 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for FPGA Accelerated Function Unit (AFU) Error Reporting
+ *
+ * Copyright 2019 Intel Corporation, Inc.
+ *
+ * Authors:
+ * Wu Hao <[email protected]>
+ * Xiao Guangrong <[email protected]>
+ * Joseph Grecco <[email protected]>
+ * Enno Luebbers <[email protected]>
+ * Tim Whisonant <[email protected]>
+ * Ananda Ravuri <[email protected]>
+ * Mitchel Henry <[email protected]>
+ */
+
+#include <linux/uaccess.h>
+
+#include "dfl-afu.h"
+
+#define PORT_ERROR_MASK 0x8
+#define PORT_ERROR 0x10
+#define PORT_FIRST_ERROR 0x18
+#define PORT_MALFORMED_REQ0 0x20
+#define PORT_MALFORMED_REQ1 0x28
+
+#define ERROR_MASK GENMASK_ULL(63, 0)
+
+/* mask or unmask port errors by the error mask register. */
+static void __port_err_mask(struct device *dev, bool mask)
+{
+ void __iomem *base;
+
+ base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
+
+ writeq(mask ? ERROR_MASK : 0, base + PORT_ERROR_MASK);
+}
+
+/* clear port errors. */
+static int __port_err_clear(struct device *dev, u64 err)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ void __iomem *base_err, *base_hdr;
+ int ret;
+ u64 v;
+
+ base_err = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
+ base_hdr = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
+
+ /*
+ * clear Port Errors
+ *
+ * - Check for AP6 State
+ * - Halt Port by keeping Port in reset
+ * - Set PORT Error mask to all 1 to mask errors
+ * - Clear all errors
+ * - Set Port mask to all 0 to enable errors
+ * - All errors start capturing new errors
+ * - Enable Port by pulling the port out of reset
+ */
+
+ /* if device is still in AP6 power state, can not clear any error. */
+ v = readq(base_hdr + PORT_HDR_STS);
+ if (FIELD_GET(PORT_STS_PWR_STATE, v) == PORT_STS_PWR_STATE_AP6) {
+ dev_err(dev, "Could not clear errors, device in AP6 state.\n");
+ return -EBUSY;
+ }
+
+ /* Halt Port by keeping Port in reset */
+ ret = __port_disable(pdev);
+ if (ret)
+ return ret;
+
+ /* Mask all errors */
+ __port_err_mask(dev, true);
+
+ /* Clear errors if err input matches with current port errors.*/
+ v = readq(base_err + PORT_ERROR);
+
+ if (v == err) {
+ writeq(v, base_err + PORT_ERROR);
+
+ v = readq(base_err + PORT_FIRST_ERROR);
+ writeq(v, base_err + PORT_FIRST_ERROR);
+ } else {
+ ret = -EINVAL;
+ }
+
+ /* Clear mask */
+ __port_err_mask(dev, false);
+
+ /* Enable the Port by clear the reset */
+ __port_enable(pdev);
+
+ return ret;
+}
+
+static ssize_t revision_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ void __iomem *base;
+
+ base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
+
+ return sprintf(buf, "%u\n", dfl_feature_revision(base));
+}
+static DEVICE_ATTR_RO(revision);
+
+static ssize_t errors_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
+ void __iomem *base;
+ u64 error;
+
+ base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
+
+ mutex_lock(&pdata->lock);
+ error = readq(base + PORT_ERROR);
+ mutex_unlock(&pdata->lock);
+
+ return sprintf(buf, "0x%llx\n", (unsigned long long)error);
+}
+static DEVICE_ATTR_RO(errors);
+
+static ssize_t first_error_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
+ void __iomem *base;
+ u64 error;
+
+ base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
+
+ mutex_lock(&pdata->lock);
+ error = readq(base + PORT_FIRST_ERROR);
+ mutex_unlock(&pdata->lock);
+
+ return sprintf(buf, "0x%llx\n", (unsigned long long)error);
+}
+static DEVICE_ATTR_RO(first_error);
+
+static ssize_t first_malformed_req_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
+ void __iomem *base;
+ u64 req0, req1;
+
+ base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
+
+ mutex_lock(&pdata->lock);
+ req0 = readq(base + PORT_MALFORMED_REQ0);
+ req1 = readq(base + PORT_MALFORMED_REQ1);
+ mutex_unlock(&pdata->lock);
+
+ return sprintf(buf, "0x%016llx%016llx\n",
+ (unsigned long long)req1, (unsigned long long)req0);
+}
+static DEVICE_ATTR_RO(first_malformed_req);
+
+static ssize_t clear_store(struct device *dev, struct device_attribute *attr,
+ const char *buff, size_t count)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
+ u64 value;
+ int ret;
+
+ if (kstrtou64(buff, 0, &value))
+ return -EINVAL;
+
+ mutex_lock(&pdata->lock);
+ ret = __port_err_clear(dev, value);
+ mutex_unlock(&pdata->lock);
+
+ return ret ? ret : count;
+}
+static DEVICE_ATTR_WO(clear);
+
+static struct attribute *port_err_attrs[] = {
+ &dev_attr_revision.attr,
+ &dev_attr_errors.attr,
+ &dev_attr_first_error.attr,
+ &dev_attr_first_malformed_req.attr,
+ &dev_attr_clear.attr,
+ NULL,
+};
+
+static struct attribute_group port_err_attr_group = {
+ .attrs = port_err_attrs,
+ .name = "errors",
+};
+
+static int port_err_init(struct platform_device *pdev,
+ struct dfl_feature *feature)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
+
+ mutex_lock(&pdata->lock);
+ __port_err_mask(&pdev->dev, false);
+ mutex_unlock(&pdata->lock);
+
+ return device_add_group(&pdev->dev, &port_err_attr_group);
+}
+
+static void port_err_uinit(struct platform_device *pdev,
+ struct dfl_feature *feature)
+{
+ device_remove_group(&pdev->dev, &port_err_attr_group);
+}
+
+const struct dfl_feature_id port_err_id_table[] = {
+ {.id = PORT_FEATURE_ID_ERROR,},
+ {0,}
+};
+
+const struct dfl_feature_ops port_err_ops = {
+ .init = port_err_init,
+ .uinit = port_err_uinit,
+};
diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
index e312179..b14df11 100644
--- a/drivers/fpga/dfl-afu-main.c
+++ b/drivers/fpga/dfl-afu-main.c
@@ -524,6 +524,10 @@ static void port_afu_uinit(struct platform_device *pdev,
.ops = &port_afu_ops,
},
{
+ .id_table = port_err_id_table,
+ .ops = &port_err_ops,
+ },
+ {
.ops = NULL,
}
};
diff --git a/drivers/fpga/dfl-afu.h b/drivers/fpga/dfl-afu.h
index 35e60c5..c3182a2 100644
--- a/drivers/fpga/dfl-afu.h
+++ b/drivers/fpga/dfl-afu.h
@@ -100,4 +100,8 @@ int afu_dma_map_region(struct dfl_feature_platform_data *pdata,
struct dfl_afu_dma_region *
afu_dma_region_find(struct dfl_feature_platform_data *pdata,
u64 iova, u64 size);
+
+extern const struct dfl_feature_ops port_err_ops;
+extern const struct dfl_feature_id port_err_id_table[];
+
#endif /* __DFL_AFU_H */
--
1.8.3.1

2019-08-04 10:40:28

by Wu Hao

Subject: [PATCH v4 06/12] fpga: dfl: afu: export __port_enable/disable function.

As these two functions are used by other private features. e.g.
in error reporting private feature, it requires to check port status
and reset port for error clearing.

Signed-off-by: Xu Yilun <[email protected]>
Signed-off-by: Wu Hao <[email protected]>
Acked-by: Moritz Fischer <[email protected]>
Acked-by: Alan Tull <[email protected]>
Signed-off-by: Moritz Fischer <[email protected]>
---
v2: rebased
---
drivers/fpga/dfl-afu-main.c | 25 ++++++++++++++-----------
drivers/fpga/dfl-afu.h | 3 +++
2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
index e013afb..e312179 100644
--- a/drivers/fpga/dfl-afu-main.c
+++ b/drivers/fpga/dfl-afu-main.c
@@ -22,14 +22,16 @@
#include "dfl-afu.h"

/**
- * port_enable - enable a port
+ * __port_enable - enable a port
* @pdev: port platform device.
*
* Enable Port by clear the port soft reset bit, which is set by default.
* The AFU is unable to respond to any MMIO access while in reset.
- * port_enable function should only be used after port_disable function.
+ * __port_enable function should only be used after __port_disable function.
+ *
+ * The caller needs to hold lock for protection.
*/
-static void port_enable(struct platform_device *pdev)
+void __port_enable(struct platform_device *pdev)
{
struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
void __iomem *base;
@@ -52,13 +54,14 @@ static void port_enable(struct platform_device *pdev)
#define RST_POLL_TIMEOUT 1000 /* us */

/**
- * port_disable - disable a port
+ * __port_disable - disable a port
* @pdev: port platform device.
*
- * Disable Port by setting the port soft reset bit, it puts the port into
- * reset.
+ * Disable Port by setting the port soft reset bit, it puts the port into reset.
+ *
+ * The caller needs to hold lock for protection.
*/
-static int port_disable(struct platform_device *pdev)
+int __port_disable(struct platform_device *pdev)
{
struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
void __iomem *base;
@@ -104,9 +107,9 @@ static int __port_reset(struct platform_device *pdev)
{
int ret;

- ret = port_disable(pdev);
+ ret = __port_disable(pdev);
if (!ret)
- port_enable(pdev);
+ __port_enable(pdev);

return ret;
}
@@ -806,9 +809,9 @@ static int port_enable_set(struct platform_device *pdev, bool enable)

mutex_lock(&pdata->lock);
if (enable)
- port_enable(pdev);
+ __port_enable(pdev);
else
- ret = port_disable(pdev);
+ ret = __port_disable(pdev);
mutex_unlock(&pdata->lock);

return ret;
diff --git a/drivers/fpga/dfl-afu.h b/drivers/fpga/dfl-afu.h
index 0c7630a..35e60c5 100644
--- a/drivers/fpga/dfl-afu.h
+++ b/drivers/fpga/dfl-afu.h
@@ -79,6 +79,9 @@ struct dfl_afu {
struct dfl_feature_platform_data *pdata;
};

+void __port_enable(struct platform_device *pdev);
+int __port_disable(struct platform_device *pdev);
+
void afu_mmio_region_init(struct dfl_feature_platform_data *pdata);
int afu_mmio_region_add(struct dfl_feature_platform_data *pdata,
u32 region_index, u64 region_size, u64 phys, u32 flags);
--
1.8.3.1

2019-08-04 10:41:26

by Wu Hao

Subject: [PATCH v4 03/12] fpga: dfl: afu: add AFU state related sysfs interfaces

This patch introduces more sysfs interfaces for Accelerated
Function Unit (AFU). These interfaces allow users to read
current AFU Power State (APx), read / clear AFU Power (APx)
events which are sticky to identify transient APx state,
and manage AFU's LTR (latency tolerance reporting).

Signed-off-by: Ananda Ravuri <[email protected]>
Signed-off-by: Xu Yilun <[email protected]>
Signed-off-by: Wu Hao <[email protected]>
Acked-by: Alan Tull <[email protected]>
Signed-off-by: Moritz Fischer <[email protected]>
---
v2: rebased, and remove DRV/MODULE_VERSION modifications
v3: update kernel version and date in sysfs doc
v4: improve description in sysfs doc avoid misunderstanding.
switch to kstrtobool in sysfs entry store function.
---
Documentation/ABI/testing/sysfs-platform-dfl-port | 32 +++++
drivers/fpga/dfl-afu-main.c | 137 ++++++++++++++++++++++
drivers/fpga/dfl.h | 11 ++
3 files changed, 180 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-platform-dfl-port b/Documentation/ABI/testing/sysfs-platform-dfl-port
index 6a92dda..1ab3e6f 100644
--- a/Documentation/ABI/testing/sysfs-platform-dfl-port
+++ b/Documentation/ABI/testing/sysfs-platform-dfl-port
@@ -14,3 +14,35 @@ Description: Read-only. User can program different PR bitstreams to FPGA
Accelerator Function Unit (AFU) for different functions. It
returns uuid which could be used to identify which PR bitstream
is programmed in this AFU.
+
+What: /sys/bus/platform/devices/dfl-port.0/power_state
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-only. It reports the APx (AFU Power) state, different APx
+ means different throttling level. When reading this file, it
+ returns "0" - Normal / "1" - AP1 / "2" - AP2 / "6" - AP6.
+
+What: /sys/bus/platform/devices/dfl-port.0/ap1_event
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-write. Read this file for AP1 (AFU Power State 1) event.
+ It's used to indicate transient AP1 state. Write 1 to this
+ file to clear AP1 event.
+
+What: /sys/bus/platform/devices/dfl-port.0/ap2_event
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-write. Read this file for AP2 (AFU Power State 2) event.
+ It's used to indicate transient AP2 state. Write 1 to this
+ file to clear AP2 event.
+
+What: /sys/bus/platform/devices/dfl-port.0/ltr
+Date: August 2019
+KernelVersion: 5.4
+Contact: Wu Hao <[email protected]>
+Description: Read-write. Read or set AFU latency tolerance reporting value.
+ Set ltr to 1 if the AFU can tolerate latency >= 40us or set it
+ to 0 if it is latency sensitive.
diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
index 68b4d08..12175bb 100644
--- a/drivers/fpga/dfl-afu-main.c
+++ b/drivers/fpga/dfl-afu-main.c
@@ -141,8 +141,145 @@ static int port_get_id(struct platform_device *pdev)
}
static DEVICE_ATTR_RO(id);

+static ssize_t
+ltr_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
+ void __iomem *base;
+ u64 v;
+
+ base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
+
+ mutex_lock(&pdata->lock);
+ v = readq(base + PORT_HDR_CTRL);
+ mutex_unlock(&pdata->lock);
+
+ return sprintf(buf, "%x\n", (u8)FIELD_GET(PORT_CTRL_LATENCY, v));
+}
+
+static ssize_t
+ltr_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
+ void __iomem *base;
+ bool ltr;
+ u64 v;
+
+ if (kstrtobool(buf, &ltr))
+ return -EINVAL;
+
+ base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
+
+ mutex_lock(&pdata->lock);
+ v = readq(base + PORT_HDR_CTRL);
+ v &= ~PORT_CTRL_LATENCY;
+ v |= FIELD_PREP(PORT_CTRL_LATENCY, ltr ? 1 : 0);
+ writeq(v, base + PORT_HDR_CTRL);
+ mutex_unlock(&pdata->lock);
+
+ return count;
+}
+static DEVICE_ATTR_RW(ltr);
+
+static ssize_t
+ap1_event_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
+ void __iomem *base;
+ u64 v;
+
+ base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
+
+ mutex_lock(&pdata->lock);
+ v = readq(base + PORT_HDR_STS);
+ mutex_unlock(&pdata->lock);
+
+ return sprintf(buf, "%x\n", (u8)FIELD_GET(PORT_STS_AP1_EVT, v));
+}
+
+static ssize_t
+ap1_event_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
+ void __iomem *base;
+ bool clear;
+
+ if (kstrtobool(buf, &clear) || !clear)
+ return -EINVAL;
+
+ base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
+
+ mutex_lock(&pdata->lock);
+ writeq(PORT_STS_AP1_EVT, base + PORT_HDR_STS);
+ mutex_unlock(&pdata->lock);
+
+ return count;
+}
+static DEVICE_ATTR_RW(ap1_event);
+
+static ssize_t
+ap2_event_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
+ void __iomem *base;
+ u64 v;
+
+ base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
+
+ mutex_lock(&pdata->lock);
+ v = readq(base + PORT_HDR_STS);
+ mutex_unlock(&pdata->lock);
+
+ return sprintf(buf, "%x\n", (u8)FIELD_GET(PORT_STS_AP2_EVT, v));
+}
+
+static ssize_t
+ap2_event_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
+ void __iomem *base;
+ bool clear;
+
+ if (kstrtobool(buf, &clear) || !clear)
+ return -EINVAL;
+
+ base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
+
+ mutex_lock(&pdata->lock);
+ writeq(PORT_STS_AP2_EVT, base + PORT_HDR_STS);
+ mutex_unlock(&pdata->lock);
+
+ return count;
+}
+static DEVICE_ATTR_RW(ap2_event);
+
+static ssize_t
+power_state_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
+ void __iomem *base;
+ u64 v;
+
+ base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
+
+ mutex_lock(&pdata->lock);
+ v = readq(base + PORT_HDR_STS);
+ mutex_unlock(&pdata->lock);
+
+ return sprintf(buf, "0x%x\n", (u8)FIELD_GET(PORT_STS_PWR_STATE, v));
+}
+static DEVICE_ATTR_RO(power_state);
+
static struct attribute *port_hdr_attrs[] = {
&dev_attr_id.attr,
+ &dev_attr_ltr.attr,
+ &dev_attr_ap1_event.attr,
+ &dev_attr_ap2_event.attr,
+ &dev_attr_power_state.attr,
NULL,
};
ATTRIBUTE_GROUPS(port_hdr);
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index b3f2f53..6625d73 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -119,6 +119,7 @@
#define PORT_HDR_NEXT_AFU NEXT_AFU
#define PORT_HDR_CAP 0x30
#define PORT_HDR_CTRL 0x38
+#define PORT_HDR_STS 0x40

/* Port Capability Register Bitfield */
#define PORT_CAP_PORT_NUM GENMASK_ULL(1, 0) /* ID of this port */
@@ -130,6 +131,16 @@
/* 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 */
+
+/* Port Status Register Bitfield */
+#define PORT_STS_AP2_EVT BIT_ULL(13) /* AP2 event detected */
+#define PORT_STS_AP1_EVT BIT_ULL(12) /* AP1 event detected */
+#define PORT_STS_PWR_STATE GENMASK_ULL(11, 8) /* AFU power states */
+#define PORT_STS_PWR_STATE_NORM 0
+#define PORT_STS_PWR_STATE_AP1 1 /* 50% throttling */
+#define PORT_STS_PWR_STATE_AP2 2 /* 90% throttling */
+#define PORT_STS_PWR_STATE_AP6 6 /* 100% throttling */
+
/**
* struct dfl_fpga_port_ops - port ops
*
--
1.8.3.1

2019-08-04 10:42:01

by Wu Hao

Subject: [PATCH v4 05/12] fpga: dfl: add id_table for dfl private feature driver

This patch adds id_table for each dfl private feature driver,
it allows to reuse same private feature driver to match and support
multiple dfl private features.

Signed-off-by: Xu Yilun <[email protected]>
Signed-off-by: Wu Hao <[email protected]>
Acked-by: Moritz Fischer <[email protected]>
Acked-by: Alan Tull <[email protected]>
Signed-off-by: Moritz Fischer <[email protected]>
---
v2: rebased, remove DRV/MODULE_VERSION modifications
---
drivers/fpga/dfl-afu-main.c | 14 ++++++++++++--
drivers/fpga/dfl-fme-main.c | 11 ++++++++---
drivers/fpga/dfl-fme-pr.c | 7 ++++++-
drivers/fpga/dfl-fme.h | 3 ++-
drivers/fpga/dfl.c | 18 ++++++++++++++++--
drivers/fpga/dfl.h | 21 +++++++++++++++------
6 files changed, 59 insertions(+), 15 deletions(-)

diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
index 407c97d..e013afb 100644
--- a/drivers/fpga/dfl-afu-main.c
+++ b/drivers/fpga/dfl-afu-main.c
@@ -435,6 +435,11 @@ static void port_hdr_uinit(struct platform_device *pdev,
return ret;
}

+static const struct dfl_feature_id port_hdr_id_table[] = {
+ {.id = PORT_FEATURE_ID_HEADER,},
+ {0,}
+};
+
static const struct dfl_feature_ops port_hdr_ops = {
.init = port_hdr_init,
.uinit = port_hdr_uinit,
@@ -496,6 +501,11 @@ static void port_afu_uinit(struct platform_device *pdev,
device_remove_groups(&pdev->dev, port_afu_groups);
}

+static const struct dfl_feature_id port_afu_id_table[] = {
+ {.id = PORT_FEATURE_ID_AFU,},
+ {0,}
+};
+
static const struct dfl_feature_ops port_afu_ops = {
.init = port_afu_init,
.uinit = port_afu_uinit,
@@ -503,11 +513,11 @@ static void port_afu_uinit(struct platform_device *pdev,

static struct dfl_feature_driver port_feature_drvs[] = {
{
- .id = PORT_FEATURE_ID_HEADER,
+ .id_table = port_hdr_id_table,
.ops = &port_hdr_ops,
},
{
- .id = PORT_FEATURE_ID_AFU,
+ .id_table = port_afu_id_table,
.ops = &port_afu_ops,
},
{
diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
index dfea2de..5fdce54 100644
--- a/drivers/fpga/dfl-fme-main.c
+++ b/drivers/fpga/dfl-fme-main.c
@@ -145,6 +145,11 @@ static long fme_hdr_ioctl(struct platform_device *pdev,
return -ENODEV;
}

+static const struct dfl_feature_id fme_hdr_id_table[] = {
+ {.id = FME_FEATURE_ID_HEADER,},
+ {0,}
+};
+
static const struct dfl_feature_ops fme_hdr_ops = {
.init = fme_hdr_init,
.uinit = fme_hdr_uinit,
@@ -153,12 +158,12 @@ static long fme_hdr_ioctl(struct platform_device *pdev,

static struct dfl_feature_driver fme_feature_drvs[] = {
{
- .id = FME_FEATURE_ID_HEADER,
+ .id_table = fme_hdr_id_table,
.ops = &fme_hdr_ops,
},
{
- .id = FME_FEATURE_ID_PR_MGMT,
- .ops = &pr_mgmt_ops,
+ .id_table = fme_pr_mgmt_id_table,
+ .ops = &fme_pr_mgmt_ops,
},
{
.ops = NULL,
diff --git a/drivers/fpga/dfl-fme-pr.c b/drivers/fpga/dfl-fme-pr.c
index 3c71dc3..a233a53 100644
--- a/drivers/fpga/dfl-fme-pr.c
+++ b/drivers/fpga/dfl-fme-pr.c
@@ -470,7 +470,12 @@ static long fme_pr_ioctl(struct platform_device *pdev,
return ret;
}

-const struct dfl_feature_ops pr_mgmt_ops = {
+const struct dfl_feature_id fme_pr_mgmt_id_table[] = {
+ {.id = FME_FEATURE_ID_PR_MGMT,},
+ {0}
+};
+
+const struct dfl_feature_ops fme_pr_mgmt_ops = {
.init = pr_mgmt_init,
.uinit = pr_mgmt_uinit,
.ioctl = fme_pr_ioctl,
diff --git a/drivers/fpga/dfl-fme.h b/drivers/fpga/dfl-fme.h
index 5394a21..e4131e8 100644
--- a/drivers/fpga/dfl-fme.h
+++ b/drivers/fpga/dfl-fme.h
@@ -33,6 +33,7 @@ struct dfl_fme {
struct dfl_feature_platform_data *pdata;
};

-extern const struct dfl_feature_ops pr_mgmt_ops;
+extern const struct dfl_feature_ops fme_pr_mgmt_ops;
+extern const struct dfl_feature_id fme_pr_mgmt_id_table[];

#endif /* __DFL_FME_H */
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index b913704..87eaef6 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -281,6 +281,21 @@ static int dfl_feature_instance_init(struct platform_device *pdev,
return ret;
}

+static bool dfl_feature_drv_match(struct dfl_feature *feature,
+ struct dfl_feature_driver *driver)
+{
+ const struct dfl_feature_id *ids = driver->id_table;
+
+ if (ids) {
+ while (ids->id) {
+ if (ids->id == feature->id)
+ return true;
+ ids++;
+ }
+ }
+ return false;
+}
+
/**
* dfl_fpga_dev_feature_init - init for sub features of dfl feature device
* @pdev: feature device.
@@ -301,8 +316,7 @@ int dfl_fpga_dev_feature_init(struct platform_device *pdev,

while (drv->ops) {
dfl_fpga_dev_for_each_feature(pdata, feature) {
- /* match feature and drv using id */
- if (feature->id == drv->id) {
+ if (dfl_feature_drv_match(feature, drv)) {
ret = dfl_feature_instance_init(pdev, pdata,
feature, drv);
if (ret)
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index c65ab29..9f0e656 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -30,8 +30,8 @@
/* 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
+/* Reserved 0xfe for Header Group Register and 0xff for AFU */
+#define FEATURE_ID_FIU_HEADER 0xfe
#define FEATURE_ID_AFU 0xff

#define FME_FEATURE_ID_HEADER FEATURE_ID_FIU_HEADER
@@ -169,13 +169,22 @@ struct dfl_fpga_port_ops {
int dfl_fpga_check_port_id(struct platform_device *pdev, void *pport_id);

/**
- * struct dfl_feature_driver - sub feature's driver
+ * struct dfl_feature_id - dfl private feature id
*
- * @id: sub feature id.
- * @ops: ops of this sub feature.
+ * @id: unique dfl private feature id.
*/
-struct dfl_feature_driver {
+struct dfl_feature_id {
u64 id;
+};
+
+/**
+ * struct dfl_feature_driver - dfl private feature driver
+ *
+ * @id_table: id_table for dfl private features supported by this driver.
+ * @ops: ops of this dfl private feature driver.
+ */
+struct dfl_feature_driver {
+ const struct dfl_feature_id *id_table;
const struct dfl_feature_ops *ops;
};

--
1.8.3.1

2019-08-05 15:52:25

by Greg Kroah-Hartman

Subject: Re: [PATCH v4 04/12] fpga: dfl: afu: add userclock sysfs interfaces.

On Sun, Aug 04, 2019 at 06:20:14PM +0800, Wu Hao wrote:
> This patch introduces userclock sysfs interfaces for AFU, user
> could use these interfaces for clock setting to AFU.
>
> Please note that, this is only working for port header feature
> with revision 0, for later revisions, userclock setting is moved
> to a separated private feature, so one revision sysfs interface
> is exposed to userspace application for this purpose too.
>
> Signed-off-by: Ananda Ravuri <[email protected]>
> Signed-off-by: Russ Weight <[email protected]>
> Signed-off-by: Xu Yilun <[email protected]>
> Signed-off-by: Wu Hao <[email protected]>
> Acked-by: Alan Tull <[email protected]>
> Signed-off-by: Moritz Fischer <[email protected]>
> ---
> v2: rebased, and switched to use device_add/remove_groups for sysfs
> v3: update kernel version and date in sysfs doc
> v4: rebased.
> ---
> Documentation/ABI/testing/sysfs-platform-dfl-port | 35 +++++++
> drivers/fpga/dfl-afu-main.c | 114 +++++++++++++++++++++-
> drivers/fpga/dfl.h | 9 ++
> 3 files changed, 157 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-dfl-port b/Documentation/ABI/testing/sysfs-platform-dfl-port
> index 1ab3e6f..5663441 100644
> --- a/Documentation/ABI/testing/sysfs-platform-dfl-port
> +++ b/Documentation/ABI/testing/sysfs-platform-dfl-port
> @@ -46,3 +46,38 @@ Contact: Wu Hao <[email protected]>
> Description: Read-write. Read or set AFU latency tolerance reporting value.
> Set ltr to 1 if the AFU can tolerate latency >= 40us or set it
> to 0 if it is latency sensitive.
> +
> +What: /sys/bus/platform/devices/dfl-port.0/revision
> +Date: August 2019
> +KernelVersion: 5.4
> +Contact: Wu Hao <[email protected]>
> +Description: Read-only. Read this file to get the revision of port header
> + feature.

What does "revision" mean?

It feels like you are creating a different set of sysfs files depending
on the revision field. Which is fine, sysfs is one-value-per-file and
userspace needs to handle if the file is present or not. So why not
just rely on that and not have to mess with 'revision' at all? What is
userspace going to do with that information?

> +
> +What: /sys/bus/platform/devices/dfl-port.0/userclk_freqcmd
> +Date: August 2019
> +KernelVersion: 5.4
> +Contact: Wu Hao <[email protected]>
> +Description: Write-only. User writes command to this interface to set
> + userclock to AFU.
> +
> +What: /sys/bus/platform/devices/dfl-port.0/userclk_freqsts
> +Date: August 2019
> +KernelVersion: 5.4
> +Contact: Wu Hao <[email protected]>
> +Description: Read-only. Read this file to get the status of issued command
> + to userclck_freqcmd.
> +
> +What: /sys/bus/platform/devices/dfl-port.0/userclk_freqcntrcmd
> +Date: August 2019
> +KernelVersion: 5.4
> +Contact: Wu Hao <[email protected]>
> +Description: Write-only. User writes command to this interface to set
> + userclock counter.
> +
> +What: /sys/bus/platform/devices/dfl-port.0/userclk_freqcntrsts
> +Date: August 2019
> +KernelVersion: 5.4
> +Contact: Wu Hao <[email protected]>
> +Description: Read-only. Read this file to get the status of issued command
> + to userclck_freqcntrcmd.
> diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
> index 12175bb..407c97d 100644
> --- a/drivers/fpga/dfl-afu-main.c
> +++ b/drivers/fpga/dfl-afu-main.c
> @@ -142,6 +142,17 @@ static int port_get_id(struct platform_device *pdev)
> static DEVICE_ATTR_RO(id);
>
> static ssize_t
> +revision_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + void __iomem *base;
> +
> + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
> +
> + return sprintf(buf, "%x\n", dfl_feature_revision(base));
> +}
> +static DEVICE_ATTR_RO(revision);
> +
> +static ssize_t
> ltr_show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> @@ -276,6 +287,7 @@ static int port_get_id(struct platform_device *pdev)
>
> static struct attribute *port_hdr_attrs[] = {
> &dev_attr_id.attr,
> + &dev_attr_revision.attr,
> &dev_attr_ltr.attr,
> &dev_attr_ap1_event.attr,
> &dev_attr_ap2_event.attr,
> @@ -284,14 +296,113 @@ static int port_get_id(struct platform_device *pdev)
> };
> ATTRIBUTE_GROUPS(port_hdr);
>
> +static ssize_t
> +userclk_freqcmd_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> + u64 userclk_freq_cmd;
> + void __iomem *base;
> +
> + if (kstrtou64(buf, 0, &userclk_freq_cmd))
> + return -EINVAL;
> +
> + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
> +
> + mutex_lock(&pdata->lock);
> + writeq(userclk_freq_cmd, base + PORT_HDR_USRCLK_CMD0);
> + mutex_unlock(&pdata->lock);
> +
> + return count;
> +}
> +static DEVICE_ATTR_WO(userclk_freqcmd);
> +
> +static ssize_t
> +userclk_freqcntrcmd_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> + u64 userclk_freqcntr_cmd;
> + void __iomem *base;
> +
> + if (kstrtou64(buf, 0, &userclk_freqcntr_cmd))
> + return -EINVAL;
> +
> + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
> +
> + mutex_lock(&pdata->lock);
> + writeq(userclk_freqcntr_cmd, base + PORT_HDR_USRCLK_CMD1);
> + mutex_unlock(&pdata->lock);
> +
> + return count;
> +}
> +static DEVICE_ATTR_WO(userclk_freqcntrcmd);
> +
> +static ssize_t
> +userclk_freqsts_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + u64 userclk_freqsts;
> + void __iomem *base;
> +
> + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
> +
> + userclk_freqsts = readq(base + PORT_HDR_USRCLK_STS0);
> +
> + return sprintf(buf, "0x%llx\n", (unsigned long long)userclk_freqsts);
> +}
> +static DEVICE_ATTR_RO(userclk_freqsts);
> +
> +static ssize_t
> +userclk_freqcntrsts_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + u64 userclk_freqcntrsts;
> + void __iomem *base;
> +
> + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
> +
> + userclk_freqcntrsts = readq(base + PORT_HDR_USRCLK_STS1);
> +
> + return sprintf(buf, "0x%llx\n",
> + (unsigned long long)userclk_freqcntrsts);
> +}
> +static DEVICE_ATTR_RO(userclk_freqcntrsts);
> +
> +static struct attribute *port_hdr_userclk_attrs[] = {
> + &dev_attr_userclk_freqcmd.attr,
> + &dev_attr_userclk_freqcntrcmd.attr,
> + &dev_attr_userclk_freqsts.attr,
> + &dev_attr_userclk_freqcntrsts.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(port_hdr_userclk);
> +
> static int port_hdr_init(struct platform_device *pdev,
> struct dfl_feature *feature)
> {
> + int ret;
> +
> dev_dbg(&pdev->dev, "PORT HDR Init.\n");
>
> port_reset(pdev);
>
> - return device_add_groups(&pdev->dev, port_hdr_groups);
> + ret = device_add_groups(&pdev->dev, port_hdr_groups);

This all needs to be reworked based on the ability for devices to
properly add groups when they are bound on probe (the core does it for
you, no need for the driver to do it.) But until then, you should at
least consider:

> + if (ret)
> + return ret;
> +
> + /*
> + * if revision > 0, the userclock will be moved from port hdr register
> + * region to a separated private feature.
> + */
> + if (dfl_feature_revision(feature->ioaddr) > 0)
> + return 0;
> +
> + ret = device_add_groups(&pdev->dev, port_hdr_userclk_groups);
> + if (ret)
> + device_remove_groups(&pdev->dev, port_hdr_groups);

struct attribute_group has is_visible() as a callback to have the core
show or not show, individual attributes when they are created. So no
need for a second group of attributes and you needing to add/remove
them, just add them all and let the callback handle the "is visible"
logic. Makes cleanup _so_ much easier (i.e. you don't have to do it.)

thanks,

greg k-h

2019-08-05 15:53:38

by Greg Kroah-Hartman

Subject: Re: [PATCH v4 06/12] fpga: dfl: afu: export __port_enable/disable function.

On Sun, Aug 04, 2019 at 06:20:16PM +0800, Wu Hao wrote:
> As these two functions are used by other private features. e.g.
> in error reporting private feature, it requires to check port status
> and reset port for error clearing.
>
> Signed-off-by: Xu Yilun <[email protected]>
> Signed-off-by: Wu Hao <[email protected]>
> Acked-by: Moritz Fischer <[email protected]>
> Acked-by: Alan Tull <[email protected]>
> Signed-off-by: Moritz Fischer <[email protected]>
> ---
> v2: rebased
> ---
> drivers/fpga/dfl-afu-main.c | 25 ++++++++++++++-----------
> drivers/fpga/dfl-afu.h | 3 +++
> 2 files changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
> index e013afb..e312179 100644
> --- a/drivers/fpga/dfl-afu-main.c
> +++ b/drivers/fpga/dfl-afu-main.c
> @@ -22,14 +22,16 @@
> #include "dfl-afu.h"
>
> /**
> - * port_enable - enable a port
> + * __port_enable - enable a port
> * @pdev: port platform device.
> *
> * Enable Port by clear the port soft reset bit, which is set by default.
> * The AFU is unable to respond to any MMIO access while in reset.
> - * port_enable function should only be used after port_disable function.
> + * __port_enable function should only be used after __port_disable function.
> + *
> + * The caller needs to hold lock for protection.
> */
> -static void port_enable(struct platform_device *pdev)
> +void __port_enable(struct platform_device *pdev)

worst global function name ever.

Don't polute the global namespace like this for a single driver. If you
REALLY need it, then use a prefix that shows it is your individual
dfl_special_sauce_platform_device_only type thing.

thanks,

greg k-h

2019-08-05 15:55:51

by Greg Kroah-Hartman

Subject: Re: [PATCH v4 07/12] fpga: dfl: afu: add error reporting support.

On Sun, Aug 04, 2019 at 06:20:17PM +0800, Wu Hao wrote:
> Error reporting is one important private feature, it reports error
> detected on port and accelerated function unit (AFU). It introduces
> several sysfs interfaces to allow userspace to check and clear
> errors detected by hardware.
>
> Signed-off-by: Xu Yilun <[email protected]>
> Signed-off-by: Wu Hao <[email protected]>
> Acked-by: Alan Tull <[email protected]>
> Signed-off-by: Moritz Fischer <[email protected]>
> ---
> v2: switch to device_add/remove_group for sysfs.
> v3: update kernel version and date in sysfs doc
> v4: remove dev_dbg in init/uinit callback function.
> ---
> Documentation/ABI/testing/sysfs-platform-dfl-port | 39 ++++
> drivers/fpga/Makefile | 1 +
> drivers/fpga/dfl-afu-error.c | 221 ++++++++++++++++++++++
> drivers/fpga/dfl-afu-main.c | 4 +
> drivers/fpga/dfl-afu.h | 4 +
> 5 files changed, 269 insertions(+)
> create mode 100644 drivers/fpga/dfl-afu-error.c
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-dfl-port b/Documentation/ABI/testing/sysfs-platform-dfl-port
> index 5663441..3b6580b 100644
> --- a/Documentation/ABI/testing/sysfs-platform-dfl-port
> +++ b/Documentation/ABI/testing/sysfs-platform-dfl-port
> @@ -81,3 +81,42 @@ KernelVersion: 5.4
> Contact: Wu Hao <[email protected]>
> Description: Read-only. Read this file to get the status of issued command
> to userclck_freqcntrcmd.
> +
> +What: /sys/bus/platform/devices/dfl-port.0/errors/revision
> +Date: August 2019
> +KernelVersion: 5.4
> +Contact: Wu Hao <[email protected]>
> +Description: Read-only. Read this file to get the revision of this error
> + reporting private feature.

Same revision question here that I had on an earlier patch.


> +
> +What: /sys/bus/platform/devices/dfl-port.0/errors/errors
> +Date: August 2019
> +KernelVersion: 5.4
> +Contact: Wu Hao <[email protected]>
> +Description: Read-only. Read this file to get errors detected on port and
> + Accelerated Function Unit (AFU).
> +
> +What: /sys/bus/platform/devices/dfl-port.0/errors/first_error
> +Date: August 2019
> +KernelVersion: 5.4
> +Contact: Wu Hao <[email protected]>
> +Description: Read-only. Read this file to get the first error detected by
> + hardware.
> +
> +What: /sys/bus/platform/devices/dfl-port.0/errors/first_malformed_req
> +Date: August 2019
> +KernelVersion: 5.4
> +Contact: Wu Hao <[email protected]>
> +Description: Read-only. Read this file to get the first malformed request
> + captured by hardware.
> +
> +What: /sys/bus/platform/devices/dfl-port.0/errors/clear
> +Date: August 2019
> +KernelVersion: 5.4
> +Contact: Wu Hao <[email protected]>
> +Description: Write-only. Write error code to this file to clear errors.
> + Write fails with -EINVAL if input parsing fails or input error
> + code doesn't match.
> + Write fails with -EBUSY or -ETIMEDOUT if error can't be cleared
> + as hardware is in low power state (-EBUSY) or not responding
> + (-ETIMEDOUT).
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 312b937..7255891 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_FPGA_DFL_AFU) += dfl-afu.o
>
> dfl-fme-objs := dfl-fme-main.o dfl-fme-pr.o
> dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
> +dfl-afu-objs += dfl-afu-error.o
>
> # Drivers for FPGAs which implement DFL
> obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
> diff --git a/drivers/fpga/dfl-afu-error.c b/drivers/fpga/dfl-afu-error.c
> new file mode 100644
> index 0000000..c5e0efa
> --- /dev/null
> +++ b/drivers/fpga/dfl-afu-error.c
> @@ -0,0 +1,221 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for FPGA Accelerated Function Unit (AFU) Error Reporting
> + *
> + * Copyright 2019 Intel Corporation, Inc.
> + *
> + * Authors:
> + * Wu Hao <[email protected]>
> + * Xiao Guangrong <[email protected]>
> + * Joseph Grecco <[email protected]>
> + * Enno Luebbers <[email protected]>
> + * Tim Whisonant <[email protected]>
> + * Ananda Ravuri <[email protected]>
> + * Mitchel Henry <[email protected]>
> + */
> +
> +#include <linux/uaccess.h>
> +
> +#include "dfl-afu.h"
> +
> +#define PORT_ERROR_MASK 0x8
> +#define PORT_ERROR 0x10
> +#define PORT_FIRST_ERROR 0x18
> +#define PORT_MALFORMED_REQ0 0x20
> +#define PORT_MALFORMED_REQ1 0x28
> +
> +#define ERROR_MASK GENMASK_ULL(63, 0)
> +
> +/* mask or unmask port errors by the error mask register. */
> +static void __port_err_mask(struct device *dev, bool mask)
> +{
> + void __iomem *base;
> +
> + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
> +
> + writeq(mask ? ERROR_MASK : 0, base + PORT_ERROR_MASK);
> +}
> +
> +/* clear port errors. */
> +static int __port_err_clear(struct device *dev, u64 err)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + void __iomem *base_err, *base_hdr;
> + int ret;
> + u64 v;
> +
> + base_err = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
> + base_hdr = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
> +
> + /*
> + * clear Port Errors
> + *
> + * - Check for AP6 State
> + * - Halt Port by keeping Port in reset
> + * - Set PORT Error mask to all 1 to mask errors
> + * - Clear all errors
> + * - Set Port mask to all 0 to enable errors
> + * - All errors start capturing new errors
> + * - Enable Port by pulling the port out of reset
> + */
> +
> + /* if device is still in AP6 power state, can not clear any error. */
> + v = readq(base_hdr + PORT_HDR_STS);
> + if (FIELD_GET(PORT_STS_PWR_STATE, v) == PORT_STS_PWR_STATE_AP6) {
> + dev_err(dev, "Could not clear errors, device in AP6 state.\n");
> + return -EBUSY;
> + }
> +
> + /* Halt Port by keeping Port in reset */
> + ret = __port_disable(pdev);
> + if (ret)
> + return ret;
> +
> + /* Mask all errors */
> + __port_err_mask(dev, true);
> +
> + /* Clear errors if err input matches with current port errors.*/
> + v = readq(base_err + PORT_ERROR);
> +
> + if (v == err) {
> + writeq(v, base_err + PORT_ERROR);
> +
> + v = readq(base_err + PORT_FIRST_ERROR);
> + writeq(v, base_err + PORT_FIRST_ERROR);
> + } else {
> + ret = -EINVAL;
> + }
> +
> + /* Clear mask */
> + __port_err_mask(dev, false);
> +
> + /* Enable the Port by clear the reset */
> + __port_enable(pdev);
> +
> + return ret;
> +}
> +
> +static ssize_t revision_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + void __iomem *base;
> +
> + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
> +
> + return sprintf(buf, "%u\n", dfl_feature_revision(base));
> +}
> +static DEVICE_ATTR_RO(revision);
> +
> +static ssize_t errors_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> + void __iomem *base;
> + u64 error;
> +
> + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
> +
> + mutex_lock(&pdata->lock);
> + error = readq(base + PORT_ERROR);
> + mutex_unlock(&pdata->lock);
> +
> + return sprintf(buf, "0x%llx\n", (unsigned long long)error);
> +}
> +static DEVICE_ATTR_RO(errors);
> +
> +static ssize_t first_error_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> + void __iomem *base;
> + u64 error;
> +
> + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
> +
> + mutex_lock(&pdata->lock);
> + error = readq(base + PORT_FIRST_ERROR);
> + mutex_unlock(&pdata->lock);
> +
> + return sprintf(buf, "0x%llx\n", (unsigned long long)error);
> +}
> +static DEVICE_ATTR_RO(first_error);
> +
> +static ssize_t first_malformed_req_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> + void __iomem *base;
> + u64 req0, req1;
> +
> + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
> +
> + mutex_lock(&pdata->lock);
> + req0 = readq(base + PORT_MALFORMED_REQ0);
> + req1 = readq(base + PORT_MALFORMED_REQ1);
> + mutex_unlock(&pdata->lock);
> +
> + return sprintf(buf, "0x%016llx%016llx\n",
> + (unsigned long long)req1, (unsigned long long)req0);
> +}
> +static DEVICE_ATTR_RO(first_malformed_req);
> +
> +static ssize_t clear_store(struct device *dev, struct device_attribute *attr,
> + const char *buff, size_t count)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> + u64 value;
> + int ret;
> +
> + if (kstrtou64(buff, 0, &value))
> + return -EINVAL;
> +
> + mutex_lock(&pdata->lock);
> + ret = __port_err_clear(dev, value);
> + mutex_unlock(&pdata->lock);
> +
> + return ret ? ret : count;
> +}
> +static DEVICE_ATTR_WO(clear);
> +
> +static struct attribute *port_err_attrs[] = {
> + &dev_attr_revision.attr,
> + &dev_attr_errors.attr,
> + &dev_attr_first_error.attr,
> + &dev_attr_first_malformed_req.attr,
> + &dev_attr_clear.attr,
> + NULL,
> +};
> +
> +static struct attribute_group port_err_attr_group = {
> + .attrs = port_err_attrs,
> + .name = "errors",
> +};
> +
> +static int port_err_init(struct platform_device *pdev,
> + struct dfl_feature *feature)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +
> + mutex_lock(&pdata->lock);
> + __port_err_mask(&pdev->dev, false);
> + mutex_unlock(&pdata->lock);

Locking one data structure and then modifying another one is up there
with "things never to do in the kernel unless you want a huge
headache!".

> +
> + return device_add_group(&pdev->dev, &port_err_attr_group);

You raced userspace and lost :(

thanks,

greg k-h

2019-08-05 15:58:36

by Greg Kroah-Hartman

Subject: Re: [PATCH v4 11/12] fpga: dfl: fme: add global error reporting support

On Sun, Aug 04, 2019 at 06:20:21PM +0800, Wu Hao wrote:
> +static int fme_global_err_init(struct platform_device *pdev,
> + struct dfl_feature *feature)
> +{
> + struct device *dev;
> + int ret = 0;
> +
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev)
> + return -ENOMEM;
> +
> + dev->parent = &pdev->dev;
> + dev->release = err_dev_release;
> + dev_set_name(dev, "errors");
> +
> + fme_error_enable(feature);
> +
> + ret = device_register(dev);
> + if (ret) {
> + put_device(dev);
> + return ret;
> + }
> +
> + ret = device_add_groups(dev, error_groups);

cute, but no, you do not create a whole struct device for a subdir. Use
the attribute group name like you did on earlier patches.

And again, you raced userspace and lost :(

thanks,

greg k-h

2019-08-07 02:36:26

by Wu Hao

Subject: Re: [PATCH v4 04/12] fpga: dfl: afu: add userclock sysfs interfaces.

On Mon, Aug 05, 2019 at 05:51:13PM +0200, Greg KH wrote:
> On Sun, Aug 04, 2019 at 06:20:14PM +0800, Wu Hao wrote:
> > This patch introduces userclock sysfs interfaces for AFU, user
> > could use these interfaces for clock setting to AFU.
> >
> > Please note that, this is only working for port header feature
> > with revision 0, for later revisions, userclock setting is moved
> > to a separated private feature, so one revision sysfs interface
> > is exposed to userspace application for this purpose too.
> >
> > Signed-off-by: Ananda Ravuri <[email protected]>
> > Signed-off-by: Russ Weight <[email protected]>
> > Signed-off-by: Xu Yilun <[email protected]>
> > Signed-off-by: Wu Hao <[email protected]>
> > Acked-by: Alan Tull <[email protected]>
> > Signed-off-by: Moritz Fischer <[email protected]>
> > ---
> > v2: rebased, and switched to use device_add/remove_groups for sysfs
> > v3: update kernel version and date in sysfs doc
> > v4: rebased.
> > ---
> > Documentation/ABI/testing/sysfs-platform-dfl-port | 35 +++++++
> > drivers/fpga/dfl-afu-main.c | 114 +++++++++++++++++++++-
> > drivers/fpga/dfl.h | 9 ++
> > 3 files changed, 157 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-platform-dfl-port b/Documentation/ABI/testing/sysfs-platform-dfl-port
> > index 1ab3e6f..5663441 100644
> > --- a/Documentation/ABI/testing/sysfs-platform-dfl-port
> > +++ b/Documentation/ABI/testing/sysfs-platform-dfl-port
> > @@ -46,3 +46,38 @@ Contact: Wu Hao <[email protected]>
> > Description: Read-write. Read or set AFU latency tolerance reporting value.
> > Set ltr to 1 if the AFU can tolerate latency >= 40us or set it
> > to 0 if it is latency sensitive.
> > +
> > +What: /sys/bus/platform/devices/dfl-port.0/revision
> > +Date: August 2019
> > +KernelVersion: 5.4
> > +Contact: Wu Hao <[email protected]>
> > +Description: Read-only. Read this file to get the revision of port header
> > + feature.
>
> What does "revision" mean?
>
> It feels like you are creating a different set of sysfs files depending
> on the revision field. Which is fine, sysfs is one-value-per-file and
> userspace needs to handle if the file is present or not. So why not
> just rely on that and not have to mess with 'revision' at all? What is
> userspace going to do with that information?

Hi Greg

Thanks for the review and comments,

Yes, different revision of private feature may have different hardware
features, so driver will expose different set of sysfs entries. revision
here is used to help userspace to distinguish them. I think it makes
sense to just rely on if sysfs entry exists or not, manage revision in
userspace code may be quit difficult. Plan to remove this entry in the
next version.

>
> > +
> > +What: /sys/bus/platform/devices/dfl-port.0/userclk_freqcmd
> > +Date: August 2019
> > +KernelVersion: 5.4
> > +Contact: Wu Hao <[email protected]>
> > +Description: Write-only. User writes command to this interface to set
> > + userclock to AFU.
> > +
> > +What: /sys/bus/platform/devices/dfl-port.0/userclk_freqsts
> > +Date: August 2019
> > +KernelVersion: 5.4
> > +Contact: Wu Hao <[email protected]>
> > +Description: Read-only. Read this file to get the status of issued command
> > + to userclck_freqcmd.
> > +
> > +What: /sys/bus/platform/devices/dfl-port.0/userclk_freqcntrcmd
> > +Date: August 2019
> > +KernelVersion: 5.4
> > +Contact: Wu Hao <[email protected]>
> > +Description: Write-only. User writes command to this interface to set
> > + userclock counter.
> > +
> > +What: /sys/bus/platform/devices/dfl-port.0/userclk_freqcntrsts
> > +Date: August 2019
> > +KernelVersion: 5.4
> > +Contact: Wu Hao <[email protected]>
> > +Description: Read-only. Read this file to get the status of issued command
> > + to userclck_freqcntrcmd.
> > diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
> > index 12175bb..407c97d 100644
> > --- a/drivers/fpga/dfl-afu-main.c
> > +++ b/drivers/fpga/dfl-afu-main.c
> > @@ -142,6 +142,17 @@ static int port_get_id(struct platform_device *pdev)
> > static DEVICE_ATTR_RO(id);
> >
> > static ssize_t
> > +revision_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > + void __iomem *base;
> > +
> > + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
> > +
> > + return sprintf(buf, "%x\n", dfl_feature_revision(base));
> > +}
> > +static DEVICE_ATTR_RO(revision);
> > +
> > +static ssize_t
> > ltr_show(struct device *dev, struct device_attribute *attr, char *buf)
> > {
> > struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> > @@ -276,6 +287,7 @@ static int port_get_id(struct platform_device *pdev)
> >
> > static struct attribute *port_hdr_attrs[] = {
> > &dev_attr_id.attr,
> > + &dev_attr_revision.attr,
> > &dev_attr_ltr.attr,
> > &dev_attr_ap1_event.attr,
> > &dev_attr_ap2_event.attr,
> > @@ -284,14 +296,113 @@ static int port_get_id(struct platform_device *pdev)
> > };
> > ATTRIBUTE_GROUPS(port_hdr);
> >
> > +static ssize_t
> > +userclk_freqcmd_store(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> > + u64 userclk_freq_cmd;
> > + void __iomem *base;
> > +
> > + if (kstrtou64(buf, 0, &userclk_freq_cmd))
> > + return -EINVAL;
> > +
> > + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
> > +
> > + mutex_lock(&pdata->lock);
> > + writeq(userclk_freq_cmd, base + PORT_HDR_USRCLK_CMD0);
> > + mutex_unlock(&pdata->lock);
> > +
> > + return count;
> > +}
> > +static DEVICE_ATTR_WO(userclk_freqcmd);
> > +
> > +static ssize_t
> > +userclk_freqcntrcmd_store(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> > + u64 userclk_freqcntr_cmd;
> > + void __iomem *base;
> > +
> > + if (kstrtou64(buf, 0, &userclk_freqcntr_cmd))
> > + return -EINVAL;
> > +
> > + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
> > +
> > + mutex_lock(&pdata->lock);
> > + writeq(userclk_freqcntr_cmd, base + PORT_HDR_USRCLK_CMD1);
> > + mutex_unlock(&pdata->lock);
> > +
> > + return count;
> > +}
> > +static DEVICE_ATTR_WO(userclk_freqcntrcmd);
> > +
> > +static ssize_t
> > +userclk_freqsts_show(struct device *dev, struct device_attribute *attr,
> > + char *buf)
> > +{
> > + u64 userclk_freqsts;
> > + void __iomem *base;
> > +
> > + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
> > +
> > + userclk_freqsts = readq(base + PORT_HDR_USRCLK_STS0);
> > +
> > + return sprintf(buf, "0x%llx\n", (unsigned long long)userclk_freqsts);
> > +}
> > +static DEVICE_ATTR_RO(userclk_freqsts);
> > +
> > +static ssize_t
> > +userclk_freqcntrsts_show(struct device *dev, struct device_attribute *attr,
> > + char *buf)
> > +{
> > + u64 userclk_freqcntrsts;
> > + void __iomem *base;
> > +
> > + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
> > +
> > + userclk_freqcntrsts = readq(base + PORT_HDR_USRCLK_STS1);
> > +
> > + return sprintf(buf, "0x%llx\n",
> > + (unsigned long long)userclk_freqcntrsts);
> > +}
> > +static DEVICE_ATTR_RO(userclk_freqcntrsts);
> > +
> > +static struct attribute *port_hdr_userclk_attrs[] = {
> > + &dev_attr_userclk_freqcmd.attr,
> > + &dev_attr_userclk_freqcntrcmd.attr,
> > + &dev_attr_userclk_freqsts.attr,
> > + &dev_attr_userclk_freqcntrsts.attr,
> > + NULL,
> > +};
> > +ATTRIBUTE_GROUPS(port_hdr_userclk);
> > +
> > static int port_hdr_init(struct platform_device *pdev,
> > struct dfl_feature *feature)
> > {
> > + int ret;
> > +
> > dev_dbg(&pdev->dev, "PORT HDR Init.\n");
> >
> > port_reset(pdev);
> >
> > - return device_add_groups(&pdev->dev, port_hdr_groups);
> > + ret = device_add_groups(&pdev->dev, port_hdr_groups);
>
> This all needs to be reworked based on the ability for devices to
> properly add groups when they are bound on probe (the core does it for
> you, no need for the driver to do it.) But until then, you should at
> least consider:
>
> > + if (ret)
> > + return ret;
> > +
> > + /*
> > + * if revision > 0, the userclock will be moved from port hdr register
> > + * region to a separated private feature.
> > + */
> > + if (dfl_feature_revision(feature->ioaddr) > 0)
> > + return 0;
> > +
> > + ret = device_add_groups(&pdev->dev, port_hdr_userclk_groups);
> > + if (ret)
> > + device_remove_groups(&pdev->dev, port_hdr_groups);
>
> struct attribute_group has is_visible() as a callback to have the core
> show or not show, individual attributes when they are created. So no
> need for a second group of attributes and you needing to add/remove
> them, just add them all and let the callback handle the "is visible"
> logic. Makes cleanup _so_ much easier (i.e. you don't have to do it.)

Sure, will use is_visible() here instead in the next version, it does
make thing more clear. Thanks a lot of the comments.

Hao

>
> thanks,
>
> greg k-h

2019-08-07 02:40:36

by Wu Hao

Subject: Re: [PATCH v4 06/12] fpga: dfl: afu: export __port_enable/disable function.

On Mon, Aug 05, 2019 at 05:52:40PM +0200, Greg KH wrote:
> On Sun, Aug 04, 2019 at 06:20:16PM +0800, Wu Hao wrote:
> > As these two functions are used by other private features. e.g.
> > in error reporting private feature, it requires to check port status
> > and reset port for error clearing.
> >
> > Signed-off-by: Xu Yilun <[email protected]>
> > Signed-off-by: Wu Hao <[email protected]>
> > Acked-by: Moritz Fischer <[email protected]>
> > Acked-by: Alan Tull <[email protected]>
> > Signed-off-by: Moritz Fischer <[email protected]>
> > ---
> > v2: rebased
> > ---
> > drivers/fpga/dfl-afu-main.c | 25 ++++++++++++++-----------
> > drivers/fpga/dfl-afu.h | 3 +++
> > 2 files changed, 17 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
> > index e013afb..e312179 100644
> > --- a/drivers/fpga/dfl-afu-main.c
> > +++ b/drivers/fpga/dfl-afu-main.c
> > @@ -22,14 +22,16 @@
> > #include "dfl-afu.h"
> >
> > /**
> > - * port_enable - enable a port
> > + * __port_enable - enable a port
> > * @pdev: port platform device.
> > *
> > * Enable Port by clear the port soft reset bit, which is set by default.
> > * The AFU is unable to respond to any MMIO access while in reset.
> > - * port_enable function should only be used after port_disable function.
> > + * __port_enable function should only be used after __port_disable function.
> > + *
> > + * The caller needs to hold lock for protection.
> > */
> > -static void port_enable(struct platform_device *pdev)
> > +void __port_enable(struct platform_device *pdev)
>
> worst global function name ever.
>
> Don't polute the global namespace like this for a single driver. If you
> REALLY need it, then use a prefix that shows it is your individual
> dfl_special_sauce_platform_device_only type thing.

Oh.. Sure.. Let me fix the naming in the next version.

Thanks
Hao

>
> thanks,
>
> greg k-h

2019-08-07 02:53:59

by Wu Hao

Subject: Re: [PATCH v4 07/12] fpga: dfl: afu: add error reporting support.

On Mon, Aug 05, 2019 at 05:54:37PM +0200, Greg KH wrote:
> On Sun, Aug 04, 2019 at 06:20:17PM +0800, Wu Hao wrote:
> > Error reporting is one important private feature, it reports error
> > detected on port and accelerated function unit (AFU). It introduces
> > several sysfs interfaces to allow userspace to check and clear
> > errors detected by hardware.
> >
> > Signed-off-by: Xu Yilun <[email protected]>
> > Signed-off-by: Wu Hao <[email protected]>
> > Acked-by: Alan Tull <[email protected]>
> > Signed-off-by: Moritz Fischer <[email protected]>
> > ---
> > v2: switch to device_add/remove_group for sysfs.
> > v3: update kernel version and date in sysfs doc
> > v4: remove dev_dbg in init/uinit callback function.
> > ---
> > Documentation/ABI/testing/sysfs-platform-dfl-port | 39 ++++
> > drivers/fpga/Makefile | 1 +
> > drivers/fpga/dfl-afu-error.c | 221 ++++++++++++++++++++++
> > drivers/fpga/dfl-afu-main.c | 4 +
> > drivers/fpga/dfl-afu.h | 4 +
> > 5 files changed, 269 insertions(+)
> > create mode 100644 drivers/fpga/dfl-afu-error.c
> >
> > diff --git a/Documentation/ABI/testing/sysfs-platform-dfl-port b/Documentation/ABI/testing/sysfs-platform-dfl-port
> > index 5663441..3b6580b 100644
> > --- a/Documentation/ABI/testing/sysfs-platform-dfl-port
> > +++ b/Documentation/ABI/testing/sysfs-platform-dfl-port
> > @@ -81,3 +81,42 @@ KernelVersion: 5.4
> > Contact: Wu Hao <[email protected]>
> > Description: Read-only. Read this file to get the status of issued command
> > to userclck_freqcntrcmd.
> > +
> > +What: /sys/bus/platform/devices/dfl-port.0/errors/revision
> > +Date: August 2019
> > +KernelVersion: 5.4
> > +Contact: Wu Hao <[email protected]>
> > +Description: Read-only. Read this file to get the revision of this error
> > + reporting private feature.
>
> Same revision question here that I had on an earlier patch.
>
>
> > +
> > +What: /sys/bus/platform/devices/dfl-port.0/errors/errors
> > +Date: August 2019
> > +KernelVersion: 5.4
> > +Contact: Wu Hao <[email protected]>
> > +Description: Read-only. Read this file to get errors detected on port and
> > + Accelerated Function Unit (AFU).
> > +
> > +What: /sys/bus/platform/devices/dfl-port.0/errors/first_error
> > +Date: August 2019
> > +KernelVersion: 5.4
> > +Contact: Wu Hao <[email protected]>
> > +Description: Read-only. Read this file to get the first error detected by
> > + hardware.
> > +
> > +What: /sys/bus/platform/devices/dfl-port.0/errors/first_malformed_req
> > +Date: August 2019
> > +KernelVersion: 5.4
> > +Contact: Wu Hao <[email protected]>
> > +Description: Read-only. Read this file to get the first malformed request
> > + captured by hardware.
> > +
> > +What: /sys/bus/platform/devices/dfl-port.0/errors/clear
> > +Date: August 2019
> > +KernelVersion: 5.4
> > +Contact: Wu Hao <[email protected]>
> > +Description: Write-only. Write error code to this file to clear errors.
> > + Write fails with -EINVAL if input parsing fails or input error
> > + code doesn't match.
> > + Write fails with -EBUSY or -ETIMEDOUT if error can't be cleared
> > + as hardware is in low power state (-EBUSY) or not responding
> > + (-ETIMEDOUT).
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index 312b937..7255891 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -41,6 +41,7 @@ obj-$(CONFIG_FPGA_DFL_AFU) += dfl-afu.o
> >
> > dfl-fme-objs := dfl-fme-main.o dfl-fme-pr.o
> > dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
> > +dfl-afu-objs += dfl-afu-error.o
> >
> > # Drivers for FPGAs which implement DFL
> > obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
> > diff --git a/drivers/fpga/dfl-afu-error.c b/drivers/fpga/dfl-afu-error.c
> > new file mode 100644
> > index 0000000..c5e0efa
> > --- /dev/null
> > +++ b/drivers/fpga/dfl-afu-error.c
> > @@ -0,0 +1,221 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for FPGA Accelerated Function Unit (AFU) Error Reporting
> > + *
> > + * Copyright 2019 Intel Corporation, Inc.
> > + *
> > + * Authors:
> > + * Wu Hao <[email protected]>
> > + * Xiao Guangrong <[email protected]>
> > + * Joseph Grecco <[email protected]>
> > + * Enno Luebbers <[email protected]>
> > + * Tim Whisonant <[email protected]>
> > + * Ananda Ravuri <[email protected]>
> > + * Mitchel Henry <[email protected]>
> > + */
> > +
> > +#include <linux/uaccess.h>
> > +
> > +#include "dfl-afu.h"
> > +
> > +#define PORT_ERROR_MASK 0x8
> > +#define PORT_ERROR 0x10
> > +#define PORT_FIRST_ERROR 0x18
> > +#define PORT_MALFORMED_REQ0 0x20
> > +#define PORT_MALFORMED_REQ1 0x28
> > +
> > +#define ERROR_MASK GENMASK_ULL(63, 0)
> > +
> > +/* mask or unmask port errors by the error mask register. */
> > +static void __port_err_mask(struct device *dev, bool mask)
> > +{
> > + void __iomem *base;
> > +
> > + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
> > +
> > + writeq(mask ? ERROR_MASK : 0, base + PORT_ERROR_MASK);
> > +}
> > +
> > +/* clear port errors. */
> > +static int __port_err_clear(struct device *dev, u64 err)
> > +{
> > + struct platform_device *pdev = to_platform_device(dev);
> > + void __iomem *base_err, *base_hdr;
> > + int ret;
> > + u64 v;
> > +
> > + base_err = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
> > + base_hdr = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
> > +
> > + /*
> > + * clear Port Errors
> > + *
> > + * - Check for AP6 State
> > + * - Halt Port by keeping Port in reset
> > + * - Set PORT Error mask to all 1 to mask errors
> > + * - Clear all errors
> > + * - Set Port mask to all 0 to enable errors
> > + * - All errors start capturing new errors
> > + * - Enable Port by pulling the port out of reset
> > + */
> > +
> > + /* if device is still in AP6 power state, can not clear any error. */
> > + v = readq(base_hdr + PORT_HDR_STS);
> > + if (FIELD_GET(PORT_STS_PWR_STATE, v) == PORT_STS_PWR_STATE_AP6) {
> > + dev_err(dev, "Could not clear errors, device in AP6 state.\n");
> > + return -EBUSY;
> > + }
> > +
> > + /* Halt Port by keeping Port in reset */
> > + ret = __port_disable(pdev);
> > + if (ret)
> > + return ret;
> > +
> > + /* Mask all errors */
> > + __port_err_mask(dev, true);
> > +
> > + /* Clear errors if err input matches with current port errors.*/
> > + v = readq(base_err + PORT_ERROR);
> > +
> > + if (v == err) {
> > + writeq(v, base_err + PORT_ERROR);
> > +
> > + v = readq(base_err + PORT_FIRST_ERROR);
> > + writeq(v, base_err + PORT_FIRST_ERROR);
> > + } else {
> > + ret = -EINVAL;
> > + }
> > +
> > + /* Clear mask */
> > + __port_err_mask(dev, false);
> > +
> > + /* Enable the Port by clear the reset */
> > + __port_enable(pdev);
> > +
> > + return ret;
> > +}
> > +
> > +static ssize_t revision_show(struct device *dev, struct device_attribute *attr,
> > + char *buf)
> > +{
> > + void __iomem *base;
> > +
> > + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
> > +
> > + return sprintf(buf, "%u\n", dfl_feature_revision(base));
> > +}
> > +static DEVICE_ATTR_RO(revision);
> > +
> > +static ssize_t errors_show(struct device *dev, struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> > + void __iomem *base;
> > + u64 error;
> > +
> > + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
> > +
> > + mutex_lock(&pdata->lock);
> > + error = readq(base + PORT_ERROR);
> > + mutex_unlock(&pdata->lock);
> > +
> > + return sprintf(buf, "0x%llx\n", (unsigned long long)error);
> > +}
> > +static DEVICE_ATTR_RO(errors);
> > +
> > +static ssize_t first_error_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> > + void __iomem *base;
> > + u64 error;
> > +
> > + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
> > +
> > + mutex_lock(&pdata->lock);
> > + error = readq(base + PORT_FIRST_ERROR);
> > + mutex_unlock(&pdata->lock);
> > +
> > + return sprintf(buf, "0x%llx\n", (unsigned long long)error);
> > +}
> > +static DEVICE_ATTR_RO(first_error);
> > +
> > +static ssize_t first_malformed_req_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> > + void __iomem *base;
> > + u64 req0, req1;
> > +
> > + base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
> > +
> > + mutex_lock(&pdata->lock);
> > + req0 = readq(base + PORT_MALFORMED_REQ0);
> > + req1 = readq(base + PORT_MALFORMED_REQ1);
> > + mutex_unlock(&pdata->lock);
> > +
> > + return sprintf(buf, "0x%016llx%016llx\n",
> > + (unsigned long long)req1, (unsigned long long)req0);
> > +}
> > +static DEVICE_ATTR_RO(first_malformed_req);
> > +
> > +static ssize_t clear_store(struct device *dev, struct device_attribute *attr,
> > + const char *buff, size_t count)
> > +{
> > + struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> > + u64 value;
> > + int ret;
> > +
> > + if (kstrtou64(buff, 0, &value))
> > + return -EINVAL;
> > +
> > + mutex_lock(&pdata->lock);
> > + ret = __port_err_clear(dev, value);
> > + mutex_unlock(&pdata->lock);
> > +
> > + return ret ? ret : count;
> > +}
> > +static DEVICE_ATTR_WO(clear);
> > +
> > +static struct attribute *port_err_attrs[] = {
> > + &dev_attr_revision.attr,
> > + &dev_attr_errors.attr,
> > + &dev_attr_first_error.attr,
> > + &dev_attr_first_malformed_req.attr,
> > + &dev_attr_clear.attr,
> > + NULL,
> > +};
> > +
> > +static struct attribute_group port_err_attr_group = {
> > + .attrs = port_err_attrs,
> > + .name = "errors",
> > +};
> > +
> > +static int port_err_init(struct platform_device *pdev,
> > + struct dfl_feature *feature)
> > +{
> > + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > +
> > + mutex_lock(&pdata->lock);
> > + __port_err_mask(&pdev->dev, false);
> > + mutex_unlock(&pdata->lock);
>
> Locking one data structure and then modifying another one is up there
> with "things never to do in the kernel unless you want a huge
> headache!".

Actually we always use the same lock for protection as other places, but
the code may cause some misunderstanding, let me improve this part in
the next version.

>
> > +
> > + return device_add_group(&pdev->dev, &port_err_attr_group);
>
> You raced userspace and lost :(

Do you mind giving some more hints on this one? I guess I didn't fully
understand this. :( Add handling if device_add_group failed here, or
something else I should fix?

Thanks
Hao

>
> thanks,
>
> greg k-h

2019-08-07 03:16:48

by Wu Hao

Subject: Re: [PATCH v4 11/12] fpga: dfl: fme: add global error reporting support

On Mon, Aug 05, 2019 at 05:56:26PM +0200, Greg KH wrote:
> On Sun, Aug 04, 2019 at 06:20:21PM +0800, Wu Hao wrote:
> > +static int fme_global_err_init(struct platform_device *pdev,
> > + struct dfl_feature *feature)
> > +{
> > + struct device *dev;
> > + int ret = 0;
> > +
> > + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > + if (!dev)
> > + return -ENOMEM;
> > +
> > + dev->parent = &pdev->dev;
> > + dev->release = err_dev_release;
> > + dev_set_name(dev, "errors");
> > +
> > + fme_error_enable(feature);
> > +
> > + ret = device_register(dev);
> > + if (ret) {
> > + put_device(dev);
> > + return ret;
> > + }
> > +
> > + ret = device_add_groups(dev, error_groups);
>
> cute, but no, you do not create a whole struct device for a subdir. Use
> the attribute group name like you did on earlier patches.

Sure, let me fix it in the next version.

>
> And again, you raced userspace and lost :(

Same here, could you please give some more hints here?

Thanks in advance.
Hao

>
> thanks,
>
> greg k-h

2019-08-07 08:35:20

by Wu Hao

Subject: Re: [PATCH v4 11/12] fpga: dfl: fme: add global error reporting support

On Wed, Aug 07, 2019 at 10:45:22AM +0800, Wu Hao wrote:
> On Mon, Aug 05, 2019 at 05:56:26PM +0200, Greg KH wrote:
> > On Sun, Aug 04, 2019 at 06:20:21PM +0800, Wu Hao wrote:
> > > +static int fme_global_err_init(struct platform_device *pdev,
> > > + struct dfl_feature *feature)
> > > +{
> > > + struct device *dev;
> > > + int ret = 0;
> > > +
> > > + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > > + if (!dev)
> > > + return -ENOMEM;
> > > +
> > > + dev->parent = &pdev->dev;
> > > + dev->release = err_dev_release;
> > > + dev_set_name(dev, "errors");
> > > +
> > > + fme_error_enable(feature);
> > > +
> > > + ret = device_register(dev);
> > > + if (ret) {
> > > + put_device(dev);
> > > + return ret;
> > > + }
> > > +
> > > + ret = device_add_groups(dev, error_groups);
> >
> > cute, but no, you do not create a whole struct device for a subdir. Use
> > the attribute group name like you did on earlier patches.
>
> Sure, let me fix it in the next version.
>
> >
> > And again, you raced userspace and lost :(
>
> Same here, could you please give some more hints here?

Oh.. I see..

I should follow [1] as this is a platform driver. I will fix it. Thanks!

[PATCH 00/11] Platform drivers, provide a way to add sysfs groups easily

[1]https://lkml.org/lkml/2019/7/4/181

Hao

>
> Thanks in advance.
> Hao
>
> >
> > thanks,
> >
> > greg k-h

2019-08-07 09:32:24

by Greg Kroah-Hartman

Subject: Re: [PATCH v4 11/12] fpga: dfl: fme: add global error reporting support

On Wed, Aug 07, 2019 at 04:08:25PM +0800, Wu Hao wrote:
> On Wed, Aug 07, 2019 at 10:45:22AM +0800, Wu Hao wrote:
> > On Mon, Aug 05, 2019 at 05:56:26PM +0200, Greg KH wrote:
> > > On Sun, Aug 04, 2019 at 06:20:21PM +0800, Wu Hao wrote:
> > > > +static int fme_global_err_init(struct platform_device *pdev,
> > > > + struct dfl_feature *feature)
> > > > +{
> > > > + struct device *dev;
> > > > + int ret = 0;
> > > > +
> > > > + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > > > + if (!dev)
> > > > + return -ENOMEM;
> > > > +
> > > > + dev->parent = &pdev->dev;
> > > > + dev->release = err_dev_release;
> > > > + dev_set_name(dev, "errors");
> > > > +
> > > > + fme_error_enable(feature);
> > > > +
> > > > + ret = device_register(dev);
> > > > + if (ret) {
> > > > + put_device(dev);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = device_add_groups(dev, error_groups);
> > >
> > > cute, but no, you do not create a whole struct device for a subdir. Use
> > > the attribute group name like you did on earlier patches.
> >
> > Sure, let me fix it in the next version.
> >
> > >
> > > And again, you raced userspace and lost :(
> >
> > Same here, could you please give some more hints here?
>
> Oh.. I see..
>
> I should follow [1] as this is a platform driver. I will fix it. Thanks!
>
> [PATCH 00/11] Platform drivers, provide a way to add sysfs groups easily
>
> [1]https://lkml.org/lkml/2019/7/4/181

Yes, that is the correct thing to do.

thanks,

greg k-h