On Mon, Mar 05, 2018 at 01:42:41PM -0600, Alan Tull wrote:

On Thu, Mar 1, 2018 at 12:17 AM, Wu Hao [off-list ref] wrote:

quoted

On Wed, Feb 28, 2018 at 04:55:15PM -0600, Alan Tull wrote:

quoted

On Tue, Feb 13, 2018 at 3:24 AM, Wu Hao [off-list ref] wrote:

Hi Hao,

Hi Alan,

Thanks for the review.

quoted

quoted

This patch introduces a compat_id member and sysfs interface for each
fpga-region, e.g userspace applications could read the compat_id
from the sysfs interface for compatibility checking before PR.

Signed-off-by: Wu Hao <redacted>
---
 Documentation/ABI/testing/sysfs-class-fpga-region |  5 +++++
 drivers/fpga/fpga-region.c                        | 19 +++++++++++++++++++
 include/linux/fpga/fpga-region.h                  | 13 +++++++++++++
 3 files changed, 37 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-region

diff --git a/Documentation/ABI/testing/sysfs-class-fpga-region b/Documentation/ABI/testing/sysfs-class-fpga-region
new file mode 100644
index 0000000..419d930
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-fpga-region

@@ -0,0 +1,5 @@
+What:          /sys/class/fpga_region/<region>/compat_id
+Date:          February 2018
+KernelVersion: 4.16
+Contact:       Wu Hao <hao.wu@intel.com>
+Description:   FPGA region id for compatibility check.

It would be helpful to add some explanation here that although the
intended function of compat_id is set, the way the actual value is
defined or calculated is set by the layer that is creating the FPGA
region.

Sure.

Description: FPGA region id for compatibility check, e.g compatibility
             of the FPGA reconfiguration hardware and image. This value
             is defined or calculated by the layer that is creating the
             FPGA region. This interface returns the compat_id value or
             just error code -ENOENT in case compat_id is not used.

quoted

quoted

quoted

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 660a91b..babec96 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c

@@ -162,6 +162,24 @@ int fpga_region_program_fpga(struct fpga_region *region)
 }
 EXPORT_SYMBOL_GPL(fpga_region_program_fpga);

+static ssize_t compat_id_show(struct device *dev,
+                             struct device_attribute *attr, char *buf)
+{
+       struct fpga_region *region = to_fpga_region(dev);

This looks good, but not all users of FPGA are going to use compat_id.
How would you feel about making it a pointer in struct fpga_region?
With compat_id as a pointer, could check for non-null compat_id
pointer and return an error if it wasn't initialized.

It sounds good to me.

if (!region->compat_id)
        return -ENOENT;

Yes, thanks!

Thanks for the review.

Hao

Alan

quoted

quoted

quoted

+
+       return sprintf(buf, "%016llx%016llx\n",
+                      (unsigned long long)region->compat_id.id_h,
+                      (unsigned long long)region->compat_id.id_l);
+}
+
+static DEVICE_ATTR_RO(compat_id);
+
+static struct attribute *fpga_region_attrs[] = {
+       &dev_attr_compat_id.attr,
+       NULL,
+};
+ATTRIBUTE_GROUPS(fpga_region);
+
 int fpga_region_register(struct fpga_region *region)
 {
        struct device *dev = region->parent;

@@ -226,6 +244,7 @@ static int __init fpga_region_init(void)
        if (IS_ERR(fpga_region_class))
                return PTR_ERR(fpga_region_class);

+       fpga_region_class->dev_groups = fpga_region_groups;
        fpga_region_class->dev_release = fpga_region_dev_release;

        return 0;

diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
index 423c87e..bf97dcc 100644
--- a/include/linux/fpga/fpga-region.h
+++ b/include/linux/fpga/fpga-region.h

@@ -6,6 +6,17 @@
 #include <linux/fpga/fpga-bridge.h>

 /**
+ * struct fpga_region_compat_id - FPGA Region id for compatibility check
+ *
+ * @id_h: high 64bit of the compat_id
+ * @id_l: low 64bit of the compat_id
+ */
+struct fpga_region_compat_id {
+       u64 id_h;
+       u64 id_l;

I guess each user will choose how to define these bits.

Yes.

quoted

quoted

+};
+
+/**
  * struct fpga_region - FPGA Region structure
  * @dev: FPGA Region device
  * @parent: parent device

@@ -13,6 +24,7 @@
  * @bridge_list: list of FPGA bridges specified in region
  * @mgr: FPGA manager
  * @info: FPGA image info
+ * @compat_id: FPGA region id for compatibility check.
  * @priv: private data
  * @get_bridges: optional function to get bridges to a list
  * @groups: optional attribute groups.

@@ -24,6 +36,7 @@ struct fpga_region {
        struct list_head bridge_list;
        struct fpga_manager *mgr;
        struct fpga_image_info *info;
+       struct fpga_region_compat_id compat_id;

Here it would be a pointer instead.

Yes. Will update this patch.

Thanks
Hao

quoted

Alan

quoted

        void *priv;
        int (*get_bridges)(struct fpga_region *region);
        const struct attribute_group **groups;
--
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html