[PATCH v2 0/4] fpga: fpga-mgr: move compat_id from fpga_mgr to dfl

[PATCH v2 0/4] fpga: fpga-mgr: move compat_id from fpga_mgr to dfl

[trix]

From: Tom Rix <trix@redhat.com>

A followup to
https://lore.kernel.org/linux-fpga/aa06a7ca-eff3-5c0d-f3b0-f1d9ddb74526@redhat.com/
The current storing of compat_id in fpga_manager is dfl specific.
This makes the refactoring of the release()'s complicated because there
is a dfl specific flavor of register().

Keeping the compat_id sysfs abi, each implementation through the new
compat_id_show() fpga_region op can print out whatever value they need
to the sysfs.  Currently only dfl does.

Since there are now two ops for fpga_region, give fpga_region its
own ops table.  Add a wrapper for get_bridges().

Changes from
v1
  Completely written to keep sysfs abi

Tom Rix (4):
  fpga: region: introduce fpga_region_ops
  fpga: region: introduce compat_id_show op
  fpga: dfl: implement the compat_id_show region op
  fpga: remove compat_id from fpga_manager and fpga_region

 Documentation/driver-api/fpga/fpga-region.rst |  6 ++-
 drivers/fpga/dfl-fme-mgr.c                    | 23 ++++++-----
 drivers/fpga/dfl-fme-pr.c                     |  2 +-
 drivers/fpga/dfl-fme-region.c                 | 21 +++++++++-
 drivers/fpga/dfl.h                            | 14 +++++++
 drivers/fpga/fpga-region.c                    | 40 ++++++++++---------
 drivers/fpga/of-fpga-region.c                 |  6 ++-
 include/linux/fpga/fpga-mgr.h                 | 13 ------
 include/linux/fpga/fpga-region.h              | 26 +++++++++---
 9 files changed, 99 insertions(+), 52 deletions(-)

-- 
2.26.3

[PATCH v2 1/4] fpga: region: introduce fpga_region_ops

[trix]

From: Tom Rix <trix@redhat.com>

Convert passing of a get_bridges() function pointer in the
the *fpga_region_create() to passing an ops table with
get_bridges() as an element.

For backward compatibility, because *create() could take a NULL
function pointer, *create() and take a NULL ops table.

Non NULL uses were converted to ops tables.

Add a fpga_region_get_bridges() wrapper handle to the NULL cases.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 Documentation/driver-api/fpga/fpga-region.rst |  6 +++-
 drivers/fpga/dfl-fme-pr.c                     |  2 +-
 drivers/fpga/dfl-fme-region.c                 |  6 +++-
 drivers/fpga/fpga-region.c                    | 32 +++++++++++--------
 drivers/fpga/of-fpga-region.c                 |  6 +++-
 include/linux/fpga/fpga-region.h              | 22 ++++++++++---
 6 files changed, 53 insertions(+), 21 deletions(-)

diff --git a/Documentation/driver-api/fpga/fpga-region.rst b/Documentation/driver-api/fpga/fpga-region.rst
index 363a8171ab0a5..2f621de51130d 100644
--- a/Documentation/driver-api/fpga/fpga-region.rst
+++ b/Documentation/driver-api/fpga/fpga-region.rst
@@ -46,6 +46,7 @@ API to add a new FPGA region
 ----------------------------
 
 * struct fpga_region — The FPGA region struct
+* struct fpga_region_ops —  Low level FPGA region driver ops
 * devm_fpga_region_create() — Allocate and init a region struct
 * fpga_region_register() —  Register an FPGA region
 * fpga_region_unregister() —  Unregister an FPGA region
@@ -63,7 +64,7 @@ The FPGA region will need to specify which bridges to control while programming
 the FPGA.  The region driver can build a list of bridges during probe time
 (:c:expr:`fpga_region->bridge_list`) or it can have a function that creates
 the list of bridges to program just before programming
-(:c:expr:`fpga_region->get_bridges`).  The FPGA bridge framework supplies the
+(:c:expr:`fpga_region_ops->get_bridges`).  The FPGA bridge framework supplies the
 following APIs to handle building or tearing down that list.
 
 * fpga_bridge_get_to_list() — Get a ref of an FPGA bridge, add it to a
@@ -75,6 +76,9 @@ following APIs to handle building or tearing down that list.
 .. kernel-doc:: include/linux/fpga/fpga-region.h
    :functions: fpga_region
 
+.. kernel-doc:: include/linux/fpga/fpga-region.h
+   :functions: fpga_region_ops
+
 .. kernel-doc:: drivers/fpga/fpga-region.c
    :functions: devm_fpga_region_create
 
diff --git a/drivers/fpga/dfl-fme-pr.c b/drivers/fpga/dfl-fme-pr.c
index 1194c0e850e07..5869f7cb188f6 100644
--- a/drivers/fpga/dfl-fme-pr.c
+++ b/drivers/fpga/dfl-fme-pr.c
@@ -151,7 +151,7 @@ static int fme_pr(struct platform_device *pdev, unsigned long arg)
 	 * reenabling the bridge to clear things out between accleration runs.
 	 * so no need to hold the bridges after partial reconfiguration.
 	 */
-	if (region->get_bridges)
+	if (region->rops && region->rops->get_bridges)
 		fpga_bridges_put(&region->bridge_list);
 
 	put_device(&region->dev);
diff --git a/drivers/fpga/dfl-fme-region.c b/drivers/fpga/dfl-fme-region.c
index 1eeb42af10122..ca7277d3d30a9 100644
--- a/drivers/fpga/dfl-fme-region.c
+++ b/drivers/fpga/dfl-fme-region.c
@@ -27,6 +27,10 @@ static int fme_region_get_bridges(struct fpga_region *region)
 	return fpga_bridge_get_to_list(dev, region->info, &region->bridge_list);
 }
 
+static const struct fpga_region_ops fme_fpga_region_ops = {
+	.get_bridges = fme_region_get_bridges,
+};
+
 static int fme_region_probe(struct platform_device *pdev)
 {
 	struct dfl_fme_region_pdata *pdata = dev_get_platdata(&pdev->dev);
@@ -39,7 +43,7 @@ static int fme_region_probe(struct platform_device *pdev)
 	if (IS_ERR(mgr))
 		return -EPROBE_DEFER;
 
-	region = devm_fpga_region_create(dev, mgr, fme_region_get_bridges);
+	region = devm_fpga_region_create(dev, mgr, &fme_fpga_region_ops);
 	if (!region) {
 		ret = -ENOMEM;
 		goto eprobe_mgr_put;
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index a4838715221ff..dfa35c2dc2720 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -18,6 +18,14 @@
 static DEFINE_IDA(fpga_region_ida);
 static struct class *fpga_region_class;
 
+static int fpga_region_get_bridges(struct fpga_region *region)
+{
+	if (region->rops && region->rops->get_bridges)
+		return region->rops->get_bridges(region);
+
+	return 0;
+}
+
 struct fpga_region *fpga_region_class_find(
 	struct device *start, const void *data,
 	int (*match)(struct device *, const void *))
@@ -115,12 +123,10 @@ int fpga_region_program_fpga(struct fpga_region *region)
 	 * In some cases, we already have a list of bridges in the
 	 * fpga region struct.  Or we don't have any bridges.
 	 */
-	if (region->get_bridges) {
-		ret = region->get_bridges(region);
-		if (ret) {
-			dev_err(dev, "failed to get fpga region bridges\n");
-			goto err_unlock_mgr;
-		}
+	ret = fpga_region_get_bridges(region);
+	if (ret) {
+		dev_err(dev, "failed to get fpga region bridges\n");
+		goto err_unlock_mgr;
 	}
 
 	ret = fpga_bridges_disable(&region->bridge_list);
@@ -147,7 +153,7 @@ int fpga_region_program_fpga(struct fpga_region *region)
 	return 0;
 
 err_put_br:
-	if (region->get_bridges)
+	if (region->rops && region->rops->get_bridges)
 		fpga_bridges_put(&region->bridge_list);
 err_unlock_mgr:
 	fpga_mgr_unlock(region->mgr);
@@ -183,7 +189,7 @@ ATTRIBUTE_GROUPS(fpga_region);
  * fpga_region_create - alloc and init a struct fpga_region
  * @parent: device parent
  * @mgr: manager that programs this region
- * @get_bridges: optional function to get bridges to a list
+ * @rops:  optional pointer to struct for fpga region ops
  *
  * The caller of this function is responsible for freeing the resulting region
  * struct with fpga_region_free().  Using devm_fpga_region_create() instead is
@@ -194,7 +200,7 @@ ATTRIBUTE_GROUPS(fpga_region);
 struct fpga_region
 *fpga_region_create(struct device *parent,
 		    struct fpga_manager *mgr,
-		    int (*get_bridges)(struct fpga_region *))
+		    const struct fpga_region_ops *rops)
 {
 	struct fpga_region *region;
 	int id, ret = 0;
@@ -208,7 +214,7 @@ struct fpga_region
 		goto err_free;
 
 	region->mgr = mgr;
-	region->get_bridges = get_bridges;
+	region->rops = rops;
 	mutex_init(&region->mutex);
 	INIT_LIST_HEAD(&region->bridge_list);
 
@@ -255,7 +261,7 @@ static void devm_fpga_region_release(struct device *dev, void *res)
  * devm_fpga_region_create - create and initialize a managed FPGA region struct
  * @parent: device parent
  * @mgr: manager that programs this region
- * @get_bridges: optional function to get bridges to a list
+ * @rops:  optional pointer to struct for fpga region ops
  *
  * This function is intended for use in an FPGA region driver's probe function.
  * After the region driver creates the region struct with
@@ -270,7 +276,7 @@ static void devm_fpga_region_release(struct device *dev, void *res)
 struct fpga_region
 *devm_fpga_region_create(struct device *parent,
 			 struct fpga_manager *mgr,
-			 int (*get_bridges)(struct fpga_region *))
+			 const struct fpga_region_ops *rops)
 {
 	struct fpga_region **ptr, *region;
 
@@ -278,7 +284,7 @@ struct fpga_region
 	if (!ptr)
 		return NULL;
 
-	region = fpga_region_create(parent, mgr, get_bridges);
+	region = fpga_region_create(parent, mgr, rops);
 	if (!region) {
 		devres_free(ptr);
 	} else {
diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
index e3c25576b6b9d..2c99605e008a6 100644
--- a/drivers/fpga/of-fpga-region.c
+++ b/drivers/fpga/of-fpga-region.c
@@ -138,6 +138,10 @@ static int of_fpga_region_get_bridges(struct fpga_region *region)
 	return 0;
 }
 
+static const struct fpga_region_ops of_fpga_region_ops = {
+	.get_bridges = of_fpga_region_get_bridges,
+};
+
 /**
  * child_regions_with_firmware
  * @overlay: device node of the overlay
@@ -405,7 +409,7 @@ static int of_fpga_region_probe(struct platform_device *pdev)
 	if (IS_ERR(mgr))
 		return -EPROBE_DEFER;
 
-	region = devm_fpga_region_create(dev, mgr, of_fpga_region_get_bridges);
+	region = devm_fpga_region_create(dev, mgr, &of_fpga_region_ops);
 	if (!region) {
 		ret = -ENOMEM;
 		goto eprobe_mgr_put;
diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
index 27cb706275dba..d712344fd00a7 100644
--- a/include/linux/fpga/fpga-region.h
+++ b/include/linux/fpga/fpga-region.h
@@ -7,6 +7,20 @@
 #include <linux/fpga/fpga-mgr.h>
 #include <linux/fpga/fpga-bridge.h>
 
+struct fpga_region;
+
+/**
+ * struct fpga_region_ops - ops for low level fpga region drivers
+ * @get_bridges: optional function to get bridges to a list
+ *
+ * fpga_region_ops are the low level functions implemented by a specific
+ * fpga region driver.  The optional ones are tested for NULL before being
+ * called, so leaving them out is fine.
+ */
+struct fpga_region_ops {
+	int (*get_bridges)(struct fpga_region *region);
+};
+
 /**
  * struct fpga_region - FPGA Region structure
  * @dev: FPGA Region device
@@ -16,7 +30,7 @@
  * @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
+ * @rops: optional pointer to struct for fpga region ops
  */
 struct fpga_region {
 	struct device dev;
@@ -26,7 +40,7 @@ struct fpga_region {
 	struct fpga_image_info *info;
 	struct fpga_compat_id *compat_id;
 	void *priv;
-	int (*get_bridges)(struct fpga_region *region);
+	const struct fpga_region_ops *rops;
 };
 
 #define to_fpga_region(d) container_of(d, struct fpga_region, dev)
@@ -39,13 +53,13 @@ int fpga_region_program_fpga(struct fpga_region *region);
 
 struct fpga_region
 *fpga_region_create(struct device *dev, struct fpga_manager *mgr,
-		    int (*get_bridges)(struct fpga_region *));
+		    const struct fpga_region_ops *rops);
 void fpga_region_free(struct fpga_region *region);
 int fpga_region_register(struct fpga_region *region);
 void fpga_region_unregister(struct fpga_region *region);
 
 struct fpga_region
 *devm_fpga_region_create(struct device *dev, struct fpga_manager *mgr,
-			int (*get_bridges)(struct fpga_region *));
+			 const struct fpga_region_ops *rops);
 
 #endif /* _FPGA_REGION_H */
-- 
2.26.3

[PATCH v2 2/4] fpga: region: introduce compat_id_show op

[trix]

From: Tom Rix <trix@redhat.com>

The sysfs reported value for compat_id is implementation
dependent.  So add an optional op to fpga_region_ops to allow
an implementation to override the default.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/fpga/fpga-region.c       | 3 +++
 include/linux/fpga/fpga-region.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index dfa35c2dc2720..864dd4f290e3b 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -169,6 +169,9 @@ static ssize_t compat_id_show(struct device *dev,
 {
 	struct fpga_region *region = to_fpga_region(dev);
 
+	if (region->rops && region->rops->compat_id_show)
+		return region->rops->compat_id_show(region, buf);
+
 	if (!region->compat_id)
 		return -ENOENT;
 
diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
index d712344fd00a7..236d3819f1c13 100644
--- a/include/linux/fpga/fpga-region.h
+++ b/include/linux/fpga/fpga-region.h
@@ -12,6 +12,7 @@ struct fpga_region;
 /**
  * struct fpga_region_ops - ops for low level fpga region drivers
  * @get_bridges: optional function to get bridges to a list
+ * @compat_id_show: optional function emit to sysfs a compatible id
  *
  * fpga_region_ops are the low level functions implemented by a specific
  * fpga region driver.  The optional ones are tested for NULL before being
@@ -19,6 +20,7 @@ struct fpga_region;
  */
 struct fpga_region_ops {
 	int (*get_bridges)(struct fpga_region *region);
+	ssize_t (*compat_id_show)(struct fpga_region *region, char *buf);
 };
 
 /**
-- 
2.26.3

[PATCH v2 3/4] fpga: dfl: implement the compat_id_show region op

[trix]

From: Tom Rix <trix@redhat.com>

Make sure dfl will work as previously when compat_id is removed
from struct fpga_manager.  Store and pass the compat_id values
internal to dfl.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/fpga/dfl-fme-mgr.c    | 16 +++++++++++++---
 drivers/fpga/dfl-fme-region.c | 14 ++++++++++++++
 drivers/fpga/dfl.h            | 14 ++++++++++++++
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
index d5861d13b3069..cd0b9157ea6e5 100644
--- a/drivers/fpga/dfl-fme-mgr.c
+++ b/drivers/fpga/dfl-fme-mgr.c
@@ -22,6 +22,7 @@
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/fpga/fpga-mgr.h>
 
+#include "dfl.h"
 #include "dfl-fme-pr.h"
 
 /* FME Partial Reconfiguration Sub Feature Register Set */
@@ -70,6 +71,7 @@
 struct fme_mgr_priv {
 	void __iomem *ioaddr;
 	u64 pr_error;
+	struct dfl_compat_id compat_id;
 };
 
 static u64 pr_error_to_mgr_status(u64 err)
@@ -272,13 +274,21 @@ static const struct fpga_manager_ops fme_mgr_ops = {
 	.status = fme_mgr_status,
 };
 
-static void fme_mgr_get_compat_id(void __iomem *fme_pr,
-				  struct fpga_compat_id *id)
+static void _fme_mgr_get_compat_id(void __iomem *fme_pr,
+				   struct dfl_compat_id *id)
 {
 	id->id_l = readq(fme_pr + FME_PR_INTFC_ID_L);
 	id->id_h = readq(fme_pr + FME_PR_INTFC_ID_H);
 }
 
+void fme_mgr_get_compat_id(struct fpga_manager *mgr,
+			   struct dfl_compat_id *id)
+{
+	struct fme_mgr_priv *priv = mgr->priv;
+	*id = priv->compat_id;
+}
+EXPORT_SYMBOL_GPL(fme_mgr_get_compat_id);
+
 static int fme_mgr_probe(struct platform_device *pdev)
 {
 	struct dfl_fme_mgr_pdata *pdata = dev_get_platdata(&pdev->dev);
@@ -306,7 +316,7 @@ static int fme_mgr_probe(struct platform_device *pdev)
 	if (!compat_id)
 		return -ENOMEM;
 
-	fme_mgr_get_compat_id(priv->ioaddr, compat_id);
+	_fme_mgr_get_compat_id(priv->ioaddr, &priv->compat_id);
 
 	mgr = devm_fpga_mgr_create(dev, "DFL FME FPGA Manager",
 				   &fme_mgr_ops, priv);
diff --git a/drivers/fpga/dfl-fme-region.c b/drivers/fpga/dfl-fme-region.c
index ca7277d3d30a9..d21eacbf2469f 100644
--- a/drivers/fpga/dfl-fme-region.c
+++ b/drivers/fpga/dfl-fme-region.c
@@ -17,6 +17,7 @@
 #include <linux/fpga/fpga-mgr.h>
 #include <linux/fpga/fpga-region.h>
 
+#include "dfl.h"
 #include "dfl-fme-pr.h"
 
 static int fme_region_get_bridges(struct fpga_region *region)
@@ -27,8 +28,21 @@ static int fme_region_get_bridges(struct fpga_region *region)
 	return fpga_bridge_get_to_list(dev, region->info, &region->bridge_list);
 }
 
+static ssize_t fme_region_compat_id_show(struct fpga_region *region, char *buf)
+{
+	struct fpga_manager *mgr = region->mgr;
+	struct dfl_compat_id compat_id;
+
+	fme_mgr_get_compat_id(mgr, &compat_id);
+
+	return sysfs_emit(buf, "%016llx%016llx\n",
+			  (unsigned long long)compat_id.id_h,
+			  (unsigned long long)compat_id.id_l);
+}
+
 static const struct fpga_region_ops fme_fpga_region_ops = {
 	.get_bridges = fme_region_get_bridges,
+	.compat_id_show = fme_region_compat_id_show,
 };
 
 static int fme_region_probe(struct platform_device *pdev)
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 2b82c96ba56c7..a83fd11b390fc 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -169,6 +169,20 @@
 #define PORT_UINT_CAP_INT_NUM	GENMASK_ULL(11, 0)	/* Interrupts num */
 #define PORT_UINT_CAP_FST_VECT	GENMASK_ULL(23, 12)	/* First Vector */
 
+/**
+ * struct dfl_compat_id - id for compatibility check
+ *
+ * @id_h: high 64bit of the compat_id
+ * @id_l: low 64bit of the compat_id
+ */
+struct dfl_compat_id {
+	u64 id_h;
+	u64 id_l;
+};
+
+void fme_mgr_get_compat_id(struct fpga_manager *mgr,
+			   struct dfl_compat_id *id);
+
 /**
  * struct dfl_fpga_port_ops - port ops
  *
-- 
2.26.3

Re: [PATCH v2 3/4] fpga: dfl: implement the compat_id_show region op

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1501 bytes --]

Hi,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.13 next-20210709]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/trix-redhat-com/fpga-fpga-mgr-move-compat_id-from-fpga_mgr-to-dfl/20210709-214433
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f55966571d5eb2876a11e48e798b4592fa1ffbb7
config: i386-randconfig-a016-20210709 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/afee0ef49cd3ec87aedcae556893cdea33cf7f1f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review trix-redhat-com/fpga-fpga-mgr-move-compat_id-from-fpga_mgr-to-dfl/20210709-214433
        git checkout afee0ef49cd3ec87aedcae556893cdea33cf7f1f
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "fme_mgr_get_compat_id" [drivers/fpga/dfl-fme-region.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36683 bytes --]

RE: [PATCH v2 3/4] fpga: dfl: implement the compat_id_show region op

[Wu, Hao]

> Subject: [PATCH v2 3/4] fpga: dfl: implement the compat_id_show region op
> 
> From: Tom Rix <trix@redhat.com>
> 
> Make sure dfl will work as previously when compat_id is removed
> from struct fpga_manager.  Store and pass the compat_id values
> internal to dfl.
> 
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  drivers/fpga/dfl-fme-mgr.c    | 16 +++++++++++++---
>  drivers/fpga/dfl-fme-region.c | 14 ++++++++++++++
>  drivers/fpga/dfl.h            | 14 ++++++++++++++
>  3 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
> index d5861d13b3069..cd0b9157ea6e5 100644
> --- a/drivers/fpga/dfl-fme-mgr.c
> +++ b/drivers/fpga/dfl-fme-mgr.c
> @@ -22,6 +22,7 @@
>  #include <linux/io-64-nonatomic-lo-hi.h>
>  #include <linux/fpga/fpga-mgr.h>
> 
> +#include "dfl.h"
>  #include "dfl-fme-pr.h"
> 
>  /* FME Partial Reconfiguration Sub Feature Register Set */
> @@ -70,6 +71,7 @@
>  struct fme_mgr_priv {
>  	void __iomem *ioaddr;
>  	u64 pr_error;
> +	struct dfl_compat_id compat_id;
>  };
> 
>  static u64 pr_error_to_mgr_status(u64 err)
> @@ -272,13 +274,21 @@ static const struct fpga_manager_ops fme_mgr_ops
> = {
>  	.status = fme_mgr_status,
>  };
> 
> -static void fme_mgr_get_compat_id(void __iomem *fme_pr,
> -				  struct fpga_compat_id *id)
> +static void _fme_mgr_get_compat_id(void __iomem *fme_pr,
> +				   struct dfl_compat_id *id)
>  {
>  	id->id_l = readq(fme_pr + FME_PR_INTFC_ID_L);
>  	id->id_h = readq(fme_pr + FME_PR_INTFC_ID_H);
>  }
> 
> +void fme_mgr_get_compat_id(struct fpga_manager *mgr,
> +			   struct dfl_compat_id *id)
> +{
> +	struct fme_mgr_priv *priv = mgr->priv;
> +	*id = priv->compat_id;
> +}
> +EXPORT_SYMBOL_GPL(fme_mgr_get_compat_id);
> +
>  static int fme_mgr_probe(struct platform_device *pdev)
>  {
>  	struct dfl_fme_mgr_pdata *pdata = dev_get_platdata(&pdev->dev);
> @@ -306,7 +316,7 @@ static int fme_mgr_probe(struct platform_device *pdev)
>  	if (!compat_id)
>  		return -ENOMEM;
> 
> -	fme_mgr_get_compat_id(priv->ioaddr, compat_id);
> +	_fme_mgr_get_compat_id(priv->ioaddr, &priv->compat_id);
> 
>  	mgr = devm_fpga_mgr_create(dev, "DFL FME FPGA Manager",
>  				   &fme_mgr_ops, priv);
> diff --git a/drivers/fpga/dfl-fme-region.c b/drivers/fpga/dfl-fme-region.c
> index ca7277d3d30a9..d21eacbf2469f 100644
> --- a/drivers/fpga/dfl-fme-region.c
> +++ b/drivers/fpga/dfl-fme-region.c
> @@ -17,6 +17,7 @@
>  #include <linux/fpga/fpga-mgr.h>
>  #include <linux/fpga/fpga-region.h>
> 
> +#include "dfl.h"
>  #include "dfl-fme-pr.h"
> 
>  static int fme_region_get_bridges(struct fpga_region *region)
> @@ -27,8 +28,21 @@ static int fme_region_get_bridges(struct fpga_region
> *region)
>  	return fpga_bridge_get_to_list(dev, region->info, &region->bridge_list);
>  }
> 
> +static ssize_t fme_region_compat_id_show(struct fpga_region *region, char
> *buf)
> +{
> +	struct fpga_manager *mgr = region->mgr;
> +	struct dfl_compat_id compat_id;
> +
> +	fme_mgr_get_compat_id(mgr, &compat_id);

It's better to have a common interface, otherwise this region driver depends on
one specific mgr driver FPGA_DFL_FME_MGR. Ideally this region driver can be
reused with a new fpga mgr driver.

Compat id can be per-region or shared one from fpga-mgr. In this hardware, all
regions share the same compat_id from fpga-mgr.

I think reuse fpga-mgr compatibility id can be done via fpga-mgr data structure
or some common API exposed by fpga-mgr. In the first implementation, we
added it to fpga-mgr data structure which has less code.

Thanks
Hao

> +
> +	return sysfs_emit(buf, "%016llx%016llx\n",
> +			  (unsigned long long)compat_id.id_h,
> +			  (unsigned long long)compat_id.id_l);
> +}
> +
>  static const struct fpga_region_ops fme_fpga_region_ops = {
>  	.get_bridges = fme_region_get_bridges,
> +	.compat_id_show = fme_region_compat_id_show,
>  };
> 
>  static int fme_region_probe(struct platform_device *pdev)
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 2b82c96ba56c7..a83fd11b390fc 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -169,6 +169,20 @@
>  #define PORT_UINT_CAP_INT_NUM	GENMASK_ULL(11, 0)	/* Interrupts
> num */
>  #define PORT_UINT_CAP_FST_VECT	GENMASK_ULL(23, 12)	/* First Vector
> */
> 
> +/**
> + * struct dfl_compat_id - id for compatibility check
> + *
> + * @id_h: high 64bit of the compat_id
> + * @id_l: low 64bit of the compat_id
> + */
> +struct dfl_compat_id {
> +	u64 id_h;
> +	u64 id_l;
> +};
> +
> +void fme_mgr_get_compat_id(struct fpga_manager *mgr,
> +			   struct dfl_compat_id *id);
> +
>  /**
>   * struct dfl_fpga_port_ops - port ops
>   *
> --
> 2.26.3

[PATCH v2 4/4] fpga: remove compat_id from fpga_manager and fpga_region

[trix]

From: Tom Rix <trix@redhat.com>

compat_id is implementation specific.  So the data should be
stored at the implemeation layer, not the infrastructure layer.
Remove the compat_id elements and supporting code.

Printing out the compat_id is done with the fpga_region
compat_id_show() op.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/fpga/dfl-fme-mgr.c       |  7 -------
 drivers/fpga/dfl-fme-region.c    |  1 -
 drivers/fpga/fpga-region.c       |  7 +------
 include/linux/fpga/fpga-mgr.h    | 13 -------------
 include/linux/fpga/fpga-region.h |  2 --
 5 files changed, 1 insertion(+), 29 deletions(-)

diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
index cd0b9157ea6e5..8c5423eeffe75 100644
--- a/drivers/fpga/dfl-fme-mgr.c
+++ b/drivers/fpga/dfl-fme-mgr.c
@@ -292,7 +292,6 @@ EXPORT_SYMBOL_GPL(fme_mgr_get_compat_id);
 static int fme_mgr_probe(struct platform_device *pdev)
 {
 	struct dfl_fme_mgr_pdata *pdata = dev_get_platdata(&pdev->dev);
-	struct fpga_compat_id *compat_id;
 	struct device *dev = &pdev->dev;
 	struct fme_mgr_priv *priv;
 	struct fpga_manager *mgr;
@@ -312,10 +311,6 @@ static int fme_mgr_probe(struct platform_device *pdev)
 			return PTR_ERR(priv->ioaddr);
 	}
 
-	compat_id = devm_kzalloc(dev, sizeof(*compat_id), GFP_KERNEL);
-	if (!compat_id)
-		return -ENOMEM;
-
 	_fme_mgr_get_compat_id(priv->ioaddr, &priv->compat_id);
 
 	mgr = devm_fpga_mgr_create(dev, "DFL FME FPGA Manager",
@@ -323,8 +318,6 @@ static int fme_mgr_probe(struct platform_device *pdev)
 	if (!mgr)
 		return -ENOMEM;
 
-	mgr->compat_id = compat_id;
-
 	return devm_fpga_mgr_register(dev, mgr);
 }
 
diff --git a/drivers/fpga/dfl-fme-region.c b/drivers/fpga/dfl-fme-region.c
index d21eacbf2469f..be1d57ee37666 100644
--- a/drivers/fpga/dfl-fme-region.c
+++ b/drivers/fpga/dfl-fme-region.c
@@ -64,7 +64,6 @@ static int fme_region_probe(struct platform_device *pdev)
 	}
 
 	region->priv = pdata;
-	region->compat_id = mgr->compat_id;
 	platform_set_drvdata(pdev, region);
 
 	ret = fpga_region_register(region);
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 864dd4f290e3b..b08d3914716f0 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -172,12 +172,7 @@ static ssize_t compat_id_show(struct device *dev,
 	if (region->rops && region->rops->compat_id_show)
 		return region->rops->compat_id_show(region, buf);
 
-	if (!region->compat_id)
-		return -ENOENT;
-
-	return sprintf(buf, "%016llx%016llx\n",
-		       (unsigned long long)region->compat_id->id_h,
-		       (unsigned long long)region->compat_id->id_l);
+	return -ENOENT;
 }
 
 static DEVICE_ATTR_RO(compat_id);
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index ec2cd8bfceb00..ebdea215a8643 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -143,24 +143,12 @@ struct fpga_manager_ops {
 #define FPGA_MGR_STATUS_IP_PROTOCOL_ERR		BIT(3)
 #define FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR	BIT(4)
 
-/**
- * struct fpga_compat_id - id for compatibility check
- *
- * @id_h: high 64bit of the compat_id
- * @id_l: low 64bit of the compat_id
- */
-struct fpga_compat_id {
-	u64 id_h;
-	u64 id_l;
-};
-
 /**
  * struct fpga_manager - fpga manager structure
  * @name: name of low level fpga manager
  * @dev: fpga manager device
  * @ref_mutex: only allows one reference to fpga manager
  * @state: state of fpga manager
- * @compat_id: FPGA manager id for compatibility check.
  * @mops: pointer to struct of fpga manager ops
  * @priv: low level driver private date
  */
@@ -169,7 +157,6 @@ struct fpga_manager {
 	struct device dev;
 	struct mutex ref_mutex;
 	enum fpga_mgr_states state;
-	struct fpga_compat_id *compat_id;
 	const struct fpga_manager_ops *mops;
 	void *priv;
 };
diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
index 236d3819f1c13..afc79784b2823 100644
--- a/include/linux/fpga/fpga-region.h
+++ b/include/linux/fpga/fpga-region.h
@@ -30,7 +30,6 @@ struct fpga_region_ops {
  * @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
  * @rops: optional pointer to struct for fpga region ops
  */
@@ -40,7 +39,6 @@ struct fpga_region {
 	struct list_head bridge_list;
 	struct fpga_manager *mgr;
 	struct fpga_image_info *info;
-	struct fpga_compat_id *compat_id;
 	void *priv;
 	const struct fpga_region_ops *rops;
 };
-- 
2.26.3

RE: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and fpga_region

[Wu, Hao]

> -----Original Message-----
> From: trix@redhat.com <trix@redhat.com>
> Sent: Friday, July 9, 2021 9:42 PM
> To: mdf@kernel.org; corbet@lwn.net; Wu, Hao <hao.wu@intel.com>
> Cc: linux-fpga@vger.kernel.org; linux-doc@vger.kernel.org; linux-
> kernel@vger.kernel.org; Tom Rix <trix@redhat.com>
> Subject: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and
> fpga_region
> 
> From: Tom Rix <trix@redhat.com>
> 
> compat_id is implementation specific.  So the data should be
> stored at the implemeation layer, not the infrastructure layer.
> Remove the compat_id elements and supporting code.

I think current compat_id format can meet the checking requirement.
Actually I hope other hardware which needs compatible checking
to expose the same format compat_id. Then we can have more 
unified/common code, e.g. userspace application/lib handling.

Currently I didn't see any other usage or requirement on this part
now, only DFL uses it.  So should we leave it here at this moment?
I feel we don't have to change it for now to move it to a
Per-fpga-mgr format. : )

Thanks
Hao

> 
> Printing out the compat_id is done with the fpga_region
> compat_id_show() op.
> 
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  drivers/fpga/dfl-fme-mgr.c       |  7 -------
>  drivers/fpga/dfl-fme-region.c    |  1 -
>  drivers/fpga/fpga-region.c       |  7 +------
>  include/linux/fpga/fpga-mgr.h    | 13 -------------
>  include/linux/fpga/fpga-region.h |  2 --
>  5 files changed, 1 insertion(+), 29 deletions(-)
> 
> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
> index cd0b9157ea6e5..8c5423eeffe75 100644
> --- a/drivers/fpga/dfl-fme-mgr.c
> +++ b/drivers/fpga/dfl-fme-mgr.c
> @@ -292,7 +292,6 @@ EXPORT_SYMBOL_GPL(fme_mgr_get_compat_id);
>  static int fme_mgr_probe(struct platform_device *pdev)
>  {
>  	struct dfl_fme_mgr_pdata *pdata = dev_get_platdata(&pdev->dev);
> -	struct fpga_compat_id *compat_id;
>  	struct device *dev = &pdev->dev;
>  	struct fme_mgr_priv *priv;
>  	struct fpga_manager *mgr;
> @@ -312,10 +311,6 @@ static int fme_mgr_probe(struct platform_device
> *pdev)
>  			return PTR_ERR(priv->ioaddr);
>  	}
> 
> -	compat_id = devm_kzalloc(dev, sizeof(*compat_id), GFP_KERNEL);
> -	if (!compat_id)
> -		return -ENOMEM;
> -
>  	_fme_mgr_get_compat_id(priv->ioaddr, &priv->compat_id);
> 
>  	mgr = devm_fpga_mgr_create(dev, "DFL FME FPGA Manager",
> @@ -323,8 +318,6 @@ static int fme_mgr_probe(struct platform_device *pdev)
>  	if (!mgr)
>  		return -ENOMEM;
> 
> -	mgr->compat_id = compat_id;
> -
>  	return devm_fpga_mgr_register(dev, mgr);
>  }
> 
> diff --git a/drivers/fpga/dfl-fme-region.c b/drivers/fpga/dfl-fme-region.c
> index d21eacbf2469f..be1d57ee37666 100644
> --- a/drivers/fpga/dfl-fme-region.c
> +++ b/drivers/fpga/dfl-fme-region.c
> @@ -64,7 +64,6 @@ static int fme_region_probe(struct platform_device *pdev)
>  	}
> 
>  	region->priv = pdata;
> -	region->compat_id = mgr->compat_id;
>  	platform_set_drvdata(pdev, region);
> 
>  	ret = fpga_region_register(region);
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index 864dd4f290e3b..b08d3914716f0 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -172,12 +172,7 @@ static ssize_t compat_id_show(struct device *dev,
>  	if (region->rops && region->rops->compat_id_show)
>  		return region->rops->compat_id_show(region, buf);
> 
> -	if (!region->compat_id)
> -		return -ENOENT;
> -
> -	return sprintf(buf, "%016llx%016llx\n",
> -		       (unsigned long long)region->compat_id->id_h,
> -		       (unsigned long long)region->compat_id->id_l);
> +	return -ENOENT;
>  }
> 
>  static DEVICE_ATTR_RO(compat_id);
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index ec2cd8bfceb00..ebdea215a8643 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -143,24 +143,12 @@ struct fpga_manager_ops {
>  #define FPGA_MGR_STATUS_IP_PROTOCOL_ERR		BIT(3)
>  #define FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR	BIT(4)
> 
> -/**
> - * struct fpga_compat_id - id for compatibility check
> - *
> - * @id_h: high 64bit of the compat_id
> - * @id_l: low 64bit of the compat_id
> - */
> -struct fpga_compat_id {
> -	u64 id_h;
> -	u64 id_l;
> -};
> -
>  /**
>   * struct fpga_manager - fpga manager structure
>   * @name: name of low level fpga manager
>   * @dev: fpga manager device
>   * @ref_mutex: only allows one reference to fpga manager
>   * @state: state of fpga manager
> - * @compat_id: FPGA manager id for compatibility check.
>   * @mops: pointer to struct of fpga manager ops
>   * @priv: low level driver private date
>   */
> @@ -169,7 +157,6 @@ struct fpga_manager {
>  	struct device dev;
>  	struct mutex ref_mutex;
>  	enum fpga_mgr_states state;
> -	struct fpga_compat_id *compat_id;
>  	const struct fpga_manager_ops *mops;
>  	void *priv;
>  };
> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
> index 236d3819f1c13..afc79784b2823 100644
> --- a/include/linux/fpga/fpga-region.h
> +++ b/include/linux/fpga/fpga-region.h
> @@ -30,7 +30,6 @@ struct fpga_region_ops {
>   * @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
>   * @rops: optional pointer to struct for fpga region ops
>   */
> @@ -40,7 +39,6 @@ struct fpga_region {
>  	struct list_head bridge_list;
>  	struct fpga_manager *mgr;
>  	struct fpga_image_info *info;
> -	struct fpga_compat_id *compat_id;
>  	void *priv;
>  	const struct fpga_region_ops *rops;
>  };
> --
> 2.26.3

Re: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and fpga_region

[Tom Rix]

On 7/11/21 6:40 PM, Wu, Hao wrote:
>> -----Original Message-----
>> From: trix@redhat.com <trix@redhat.com>
>> Sent: Friday, July 9, 2021 9:42 PM
>> To: mdf@kernel.org; corbet@lwn.net; Wu, Hao <hao.wu@intel.com>
>> Cc: linux-fpga@vger.kernel.org; linux-doc@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Tom Rix <trix@redhat.com>
>> Subject: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and
>> fpga_region
>>
>> From: Tom Rix <trix@redhat.com>
>>
>> compat_id is implementation specific.  So the data should be
>> stored at the implemeation layer, not the infrastructure layer.
>> Remove the compat_id elements and supporting code.
> I think current compat_id format can meet the checking requirement.
> Actually I hope other hardware which needs compatible checking
> to expose the same format compat_id. Then we can have more
> unified/common code, e.g. userspace application/lib handling.

v2 does not change the current ABI. The dfl output is the same as 
before, the other nonusers get -ENOENT.

For dfl compat_id is 2 64 bit registers.

For compat_id to be useful to the others, they need the flexibility to 
print to the sysfs in the manner that aligns with whatever their user 
library interface is, 2 64 values isn't going to work for everyone.  ex/ 
xrt likely would be a uuid_t printed out a special way. someone else 
maybe just string in the board fw, maybe some has a 8 or 256 bits of 
compat_id  etc.

as a driver region specific op, others are free to do whatever is required.

> Currently I didn't see any other usage or requirement on this part
> now, only DFL uses it.  So should we leave it here at this moment?
> I feel we don't have to change it for now to move it to a
> Per-fpga-mgr format. : )

The motivation for doing this now is the 'use standard class dev release 
.. ' patchset

I really do not like 2 register functions.

By moving compat_id, the 2 register functions reduces down to 1.

I did a poc here

https://lore.kernel.org/linux-fpga/20210709184511.2521508-1-trix@redhat.com/

Tom

>
> Thanks
> Hao
>
>> Printing out the compat_id is done with the fpga_region
>> compat_id_show() op.
>>
>> Signed-off-by: Tom Rix <trix@redhat.com>
>> ---
>>   drivers/fpga/dfl-fme-mgr.c       |  7 -------
>>   drivers/fpga/dfl-fme-region.c    |  1 -
>>   drivers/fpga/fpga-region.c       |  7 +------
>>   include/linux/fpga/fpga-mgr.h    | 13 -------------
>>   include/linux/fpga/fpga-region.h |  2 --
>>   5 files changed, 1 insertion(+), 29 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
>> index cd0b9157ea6e5..8c5423eeffe75 100644
>> --- a/drivers/fpga/dfl-fme-mgr.c
>> +++ b/drivers/fpga/dfl-fme-mgr.c
>> @@ -292,7 +292,6 @@ EXPORT_SYMBOL_GPL(fme_mgr_get_compat_id);
>>   static int fme_mgr_probe(struct platform_device *pdev)
>>   {
>>   	struct dfl_fme_mgr_pdata *pdata = dev_get_platdata(&pdev->dev);
>> -	struct fpga_compat_id *compat_id;
>>   	struct device *dev = &pdev->dev;
>>   	struct fme_mgr_priv *priv;
>>   	struct fpga_manager *mgr;
>> @@ -312,10 +311,6 @@ static int fme_mgr_probe(struct platform_device
>> *pdev)
>>   			return PTR_ERR(priv->ioaddr);
>>   	}
>>
>> -	compat_id = devm_kzalloc(dev, sizeof(*compat_id), GFP_KERNEL);
>> -	if (!compat_id)
>> -		return -ENOMEM;
>> -
>>   	_fme_mgr_get_compat_id(priv->ioaddr, &priv->compat_id);
>>
>>   	mgr = devm_fpga_mgr_create(dev, "DFL FME FPGA Manager",
>> @@ -323,8 +318,6 @@ static int fme_mgr_probe(struct platform_device *pdev)
>>   	if (!mgr)
>>   		return -ENOMEM;
>>
>> -	mgr->compat_id = compat_id;
>> -
>>   	return devm_fpga_mgr_register(dev, mgr);
>>   }
>>
>> diff --git a/drivers/fpga/dfl-fme-region.c b/drivers/fpga/dfl-fme-region.c
>> index d21eacbf2469f..be1d57ee37666 100644
>> --- a/drivers/fpga/dfl-fme-region.c
>> +++ b/drivers/fpga/dfl-fme-region.c
>> @@ -64,7 +64,6 @@ static int fme_region_probe(struct platform_device *pdev)
>>   	}
>>
>>   	region->priv = pdata;
>> -	region->compat_id = mgr->compat_id;
>>   	platform_set_drvdata(pdev, region);
>>
>>   	ret = fpga_region_register(region);
>> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
>> index 864dd4f290e3b..b08d3914716f0 100644
>> --- a/drivers/fpga/fpga-region.c
>> +++ b/drivers/fpga/fpga-region.c
>> @@ -172,12 +172,7 @@ static ssize_t compat_id_show(struct device *dev,
>>   	if (region->rops && region->rops->compat_id_show)
>>   		return region->rops->compat_id_show(region, buf);
>>
>> -	if (!region->compat_id)
>> -		return -ENOENT;
>> -
>> -	return sprintf(buf, "%016llx%016llx\n",
>> -		       (unsigned long long)region->compat_id->id_h,
>> -		       (unsigned long long)region->compat_id->id_l);
>> +	return -ENOENT;
>>   }
>>
>>   static DEVICE_ATTR_RO(compat_id);
>> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
>> index ec2cd8bfceb00..ebdea215a8643 100644
>> --- a/include/linux/fpga/fpga-mgr.h
>> +++ b/include/linux/fpga/fpga-mgr.h
>> @@ -143,24 +143,12 @@ struct fpga_manager_ops {
>>   #define FPGA_MGR_STATUS_IP_PROTOCOL_ERR		BIT(3)
>>   #define FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR	BIT(4)
>>
>> -/**
>> - * struct fpga_compat_id - id for compatibility check
>> - *
>> - * @id_h: high 64bit of the compat_id
>> - * @id_l: low 64bit of the compat_id
>> - */
>> -struct fpga_compat_id {
>> -	u64 id_h;
>> -	u64 id_l;
>> -};
>> -
>>   /**
>>    * struct fpga_manager - fpga manager structure
>>    * @name: name of low level fpga manager
>>    * @dev: fpga manager device
>>    * @ref_mutex: only allows one reference to fpga manager
>>    * @state: state of fpga manager
>> - * @compat_id: FPGA manager id for compatibility check.
>>    * @mops: pointer to struct of fpga manager ops
>>    * @priv: low level driver private date
>>    */
>> @@ -169,7 +157,6 @@ struct fpga_manager {
>>   	struct device dev;
>>   	struct mutex ref_mutex;
>>   	enum fpga_mgr_states state;
>> -	struct fpga_compat_id *compat_id;
>>   	const struct fpga_manager_ops *mops;
>>   	void *priv;
>>   };
>> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
>> index 236d3819f1c13..afc79784b2823 100644
>> --- a/include/linux/fpga/fpga-region.h
>> +++ b/include/linux/fpga/fpga-region.h
>> @@ -30,7 +30,6 @@ struct fpga_region_ops {
>>    * @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
>>    * @rops: optional pointer to struct for fpga region ops
>>    */
>> @@ -40,7 +39,6 @@ struct fpga_region {
>>   	struct list_head bridge_list;
>>   	struct fpga_manager *mgr;
>>   	struct fpga_image_info *info;
>> -	struct fpga_compat_id *compat_id;
>>   	void *priv;
>>   	const struct fpga_region_ops *rops;
>>   };
>> --
>> 2.26.3

RE: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and fpga_region

[Wu, Hao]

> On 7/11/21 6:40 PM, Wu, Hao wrote:
> >> -----Original Message-----
> >> From: trix@redhat.com <trix@redhat.com>
> >> Sent: Friday, July 9, 2021 9:42 PM
> >> To: mdf@kernel.org; corbet@lwn.net; Wu, Hao <hao.wu@intel.com>
> >> Cc: linux-fpga@vger.kernel.org; linux-doc@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; Tom Rix <trix@redhat.com>
> >> Subject: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and
> >> fpga_region
> >>
> >> From: Tom Rix <trix@redhat.com>
> >>
> >> compat_id is implementation specific.  So the data should be
> >> stored at the implemeation layer, not the infrastructure layer.
> >> Remove the compat_id elements and supporting code.
> > I think current compat_id format can meet the checking requirement.
> > Actually I hope other hardware which needs compatible checking
> > to expose the same format compat_id. Then we can have more
> > unified/common code, e.g. userspace application/lib handling.
> 
> v2 does not change the current ABI. The dfl output is the same as
> before, the other nonusers get -ENOENT.

I think the common ABI is changed somehow, as output format can
be anything with your change, this confuses userspace too.

> 
> For dfl compat_id is 2 64 bit registers.
> 
> For compat_id to be useful to the others, they need the flexibility to
> print to the sysfs in the manner that aligns with whatever their user
> library interface is, 2 64 values isn't going to work for everyone.  ex/
> xrt likely would be a uuid_t printed out a special way. someone else
> maybe just string in the board fw, maybe some has a 8 or 256 bits of
> compat_id  etc.
> 
> as a driver region specific op, others are free to do whatever is required.
> 
> > Currently I didn't see any other usage or requirement on this part
> > now, only DFL uses it.  So should we leave it here at this moment?
> > I feel we don't have to change it for now to move it to a
> > Per-fpga-mgr format. : )
> 
> The motivation for doing this now is the 'use standard class dev release
> .. ' patchset
> 
> I really do not like 2 register functions.
> 
> By moving compat_id, the 2 register functions reduces down to 1.
> 

You don't have to moving compact_id, you can have 1 parameter
with a data structure including everything.

Thanks
Hao

> I did a poc here
> 
> https://lore.kernel.org/linux-fpga/20210709184511.2521508-1-
> trix@redhat.com/
> 
> Tom
> 
> >
> > Thanks
> > Hao
> >
> >> Printing out the compat_id is done with the fpga_region
> >> compat_id_show() op.
> >>
> >> Signed-off-by: Tom Rix <trix@redhat.com>
> >> ---
> >>   drivers/fpga/dfl-fme-mgr.c       |  7 -------
> >>   drivers/fpga/dfl-fme-region.c    |  1 -
> >>   drivers/fpga/fpga-region.c       |  7 +------
> >>   include/linux/fpga/fpga-mgr.h    | 13 -------------
> >>   include/linux/fpga/fpga-region.h |  2 --
> >>   5 files changed, 1 insertion(+), 29 deletions(-)
> >>
> >> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
> >> index cd0b9157ea6e5..8c5423eeffe75 100644
> >> --- a/drivers/fpga/dfl-fme-mgr.c
> >> +++ b/drivers/fpga/dfl-fme-mgr.c
> >> @@ -292,7 +292,6 @@ EXPORT_SYMBOL_GPL(fme_mgr_get_compat_id);
> >>   static int fme_mgr_probe(struct platform_device *pdev)
> >>   {
> >>   	struct dfl_fme_mgr_pdata *pdata = dev_get_platdata(&pdev->dev);
> >> -	struct fpga_compat_id *compat_id;
> >>   	struct device *dev = &pdev->dev;
> >>   	struct fme_mgr_priv *priv;
> >>   	struct fpga_manager *mgr;
> >> @@ -312,10 +311,6 @@ static int fme_mgr_probe(struct platform_device
> >> *pdev)
> >>   			return PTR_ERR(priv->ioaddr);
> >>   	}
> >>
> >> -	compat_id = devm_kzalloc(dev, sizeof(*compat_id), GFP_KERNEL);
> >> -	if (!compat_id)
> >> -		return -ENOMEM;
> >> -
> >>   	_fme_mgr_get_compat_id(priv->ioaddr, &priv->compat_id);
> >>
> >>   	mgr = devm_fpga_mgr_create(dev, "DFL FME FPGA Manager",
> >> @@ -323,8 +318,6 @@ static int fme_mgr_probe(struct platform_device
> *pdev)
> >>   	if (!mgr)
> >>   		return -ENOMEM;
> >>
> >> -	mgr->compat_id = compat_id;
> >> -
> >>   	return devm_fpga_mgr_register(dev, mgr);
> >>   }
> >>
> >> diff --git a/drivers/fpga/dfl-fme-region.c b/drivers/fpga/dfl-fme-region.c
> >> index d21eacbf2469f..be1d57ee37666 100644
> >> --- a/drivers/fpga/dfl-fme-region.c
> >> +++ b/drivers/fpga/dfl-fme-region.c
> >> @@ -64,7 +64,6 @@ static int fme_region_probe(struct platform_device
> *pdev)
> >>   	}
> >>
> >>   	region->priv = pdata;
> >> -	region->compat_id = mgr->compat_id;
> >>   	platform_set_drvdata(pdev, region);
> >>
> >>   	ret = fpga_region_register(region);
> >> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> >> index 864dd4f290e3b..b08d3914716f0 100644
> >> --- a/drivers/fpga/fpga-region.c
> >> +++ b/drivers/fpga/fpga-region.c
> >> @@ -172,12 +172,7 @@ static ssize_t compat_id_show(struct device *dev,
> >>   	if (region->rops && region->rops->compat_id_show)
> >>   		return region->rops->compat_id_show(region, buf);
> >>
> >> -	if (!region->compat_id)
> >> -		return -ENOENT;
> >> -
> >> -	return sprintf(buf, "%016llx%016llx\n",
> >> -		       (unsigned long long)region->compat_id->id_h,
> >> -		       (unsigned long long)region->compat_id->id_l);
> >> +	return -ENOENT;
> >>   }
> >>
> >>   static DEVICE_ATTR_RO(compat_id);
> >> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> >> index ec2cd8bfceb00..ebdea215a8643 100644
> >> --- a/include/linux/fpga/fpga-mgr.h
> >> +++ b/include/linux/fpga/fpga-mgr.h
> >> @@ -143,24 +143,12 @@ struct fpga_manager_ops {
> >>   #define FPGA_MGR_STATUS_IP_PROTOCOL_ERR		BIT(3)
> >>   #define FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR	BIT(4)
> >>
> >> -/**
> >> - * struct fpga_compat_id - id for compatibility check
> >> - *
> >> - * @id_h: high 64bit of the compat_id
> >> - * @id_l: low 64bit of the compat_id
> >> - */
> >> -struct fpga_compat_id {
> >> -	u64 id_h;
> >> -	u64 id_l;
> >> -};
> >> -
> >>   /**
> >>    * struct fpga_manager - fpga manager structure
> >>    * @name: name of low level fpga manager
> >>    * @dev: fpga manager device
> >>    * @ref_mutex: only allows one reference to fpga manager
> >>    * @state: state of fpga manager
> >> - * @compat_id: FPGA manager id for compatibility check.
> >>    * @mops: pointer to struct of fpga manager ops
> >>    * @priv: low level driver private date
> >>    */
> >> @@ -169,7 +157,6 @@ struct fpga_manager {
> >>   	struct device dev;
> >>   	struct mutex ref_mutex;
> >>   	enum fpga_mgr_states state;
> >> -	struct fpga_compat_id *compat_id;
> >>   	const struct fpga_manager_ops *mops;
> >>   	void *priv;
> >>   };
> >> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-
> region.h
> >> index 236d3819f1c13..afc79784b2823 100644
> >> --- a/include/linux/fpga/fpga-region.h
> >> +++ b/include/linux/fpga/fpga-region.h
> >> @@ -30,7 +30,6 @@ struct fpga_region_ops {
> >>    * @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
> >>    * @rops: optional pointer to struct for fpga region ops
> >>    */
> >> @@ -40,7 +39,6 @@ struct fpga_region {
> >>   	struct list_head bridge_list;
> >>   	struct fpga_manager *mgr;
> >>   	struct fpga_image_info *info;
> >> -	struct fpga_compat_id *compat_id;
> >>   	void *priv;
> >>   	const struct fpga_region_ops *rops;
> >>   };
> >> --
> >> 2.26.3

Re: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and fpga_region

[Tom Rix]

On 7/20/21 9:48 PM, Wu, Hao wrote:
>> On 7/11/21 6:40 PM, Wu, Hao wrote:
>>>> -----Original Message-----
>>>> From: trix@redhat.com <trix@redhat.com>
>>>> Sent: Friday, July 9, 2021 9:42 PM
>>>> To: mdf@kernel.org; corbet@lwn.net; Wu, Hao <hao.wu@intel.com>
>>>> Cc: linux-fpga@vger.kernel.org; linux-doc@vger.kernel.org; linux-
>>>> kernel@vger.kernel.org; Tom Rix <trix@redhat.com>
>>>> Subject: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and
>>>> fpga_region
>>>>
>>>> From: Tom Rix <trix@redhat.com>
>>>>
>>>> compat_id is implementation specific.  So the data should be
>>>> stored at the implemeation layer, not the infrastructure layer.
>>>> Remove the compat_id elements and supporting code.
>>> I think current compat_id format can meet the checking requirement.
>>> Actually I hope other hardware which needs compatible checking
>>> to expose the same format compat_id. Then we can have more
>>> unified/common code, e.g. userspace application/lib handling.
>> v2 does not change the current ABI. The dfl output is the same as
>> before, the other nonusers get -ENOENT.
> I think the common ABI is changed somehow, as output format can
> be anything with your change, this confuses userspace too.

Only dfl uses this interface, any dfl userspace like opae reading the 
sysfs compat_id would remain unchanged.

Others will continue to receive the -ENOENT.

If the others wanted to use this entry in the future, the

existing ABI documentation is consistent with with allowing them to

define it as they wish.  The format of the output is not specified

only the error condition. with language the leaves it up to the region

creator to define.

from sysfs-class-fpga-region

"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."

>
>> For dfl compat_id is 2 64 bit registers.
>>
>> For compat_id to be useful to the others, they need the flexibility to
>> print to the sysfs in the manner that aligns with whatever their user
>> library interface is, 2 64 values isn't going to work for everyone.  ex/
>> xrt likely would be a uuid_t printed out a special way. someone else
>> maybe just string in the board fw, maybe some has a 8 or 256 bits of
>> compat_id  etc.
>>
>> as a driver region specific op, others are free to do whatever is required.
>>
>>> Currently I didn't see any other usage or requirement on this part
>>> now, only DFL uses it.  So should we leave it here at this moment?
>>> I feel we don't have to change it for now to move it to a
>>> Per-fpga-mgr format. : )
>> The motivation for doing this now is the 'use standard class dev release
>> .. ' patchset
>>
>> I really do not like 2 register functions.
>>
>> By moving compat_id, the 2 register functions reduces down to 1.
>>
> You don't have to moving compact_id, you can have 1 parameter
> with a data structure including everything.

I like the fpga_mgr_register( ... , const struct fpga_magager_info 
*info) better as well because it will stabilize the public api.

Since we agree on that, do you agree Russ's patch can be resolved by

from include/linux/fpga-mgr.h

keep

struct fpga_manager *
fpga_mgr_register(struct device *parent, const struct fpga_manager_info 
*info);

remove *simple() from the public api, move it to driver/fpga/

and something similar for fpga-region.h ?

However the compat_id refactor goes, having just *register(... *info) is 
fine and could be done first.

Tom


>
> Thanks
> Hao
>
>> I did a poc here
>>
>> https://lore.kernel.org/linux-fpga/20210709184511.2521508-1-
>> trix@redhat.com/
>>
>> Tom
>>
>>> Thanks
>>> Hao
>>>
>>>> Printing out the compat_id is done with the fpga_region
>>>> compat_id_show() op.
>>>>
>>>> Signed-off-by: Tom Rix <trix@redhat.com>
>>>> ---
>>>>    drivers/fpga/dfl-fme-mgr.c       |  7 -------
>>>>    drivers/fpga/dfl-fme-region.c    |  1 -
>>>>    drivers/fpga/fpga-region.c       |  7 +------
>>>>    include/linux/fpga/fpga-mgr.h    | 13 -------------
>>>>    include/linux/fpga/fpga-region.h |  2 --
>>>>    5 files changed, 1 insertion(+), 29 deletions(-)
>>>>
>>>> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
>>>> index cd0b9157ea6e5..8c5423eeffe75 100644
>>>> --- a/drivers/fpga/dfl-fme-mgr.c
>>>> +++ b/drivers/fpga/dfl-fme-mgr.c
>>>> @@ -292,7 +292,6 @@ EXPORT_SYMBOL_GPL(fme_mgr_get_compat_id);
>>>>    static int fme_mgr_probe(struct platform_device *pdev)
>>>>    {
>>>>    	struct dfl_fme_mgr_pdata *pdata = dev_get_platdata(&pdev->dev);
>>>> -	struct fpga_compat_id *compat_id;
>>>>    	struct device *dev = &pdev->dev;
>>>>    	struct fme_mgr_priv *priv;
>>>>    	struct fpga_manager *mgr;
>>>> @@ -312,10 +311,6 @@ static int fme_mgr_probe(struct platform_device
>>>> *pdev)
>>>>    			return PTR_ERR(priv->ioaddr);
>>>>    	}
>>>>
>>>> -	compat_id = devm_kzalloc(dev, sizeof(*compat_id), GFP_KERNEL);
>>>> -	if (!compat_id)
>>>> -		return -ENOMEM;
>>>> -
>>>>    	_fme_mgr_get_compat_id(priv->ioaddr, &priv->compat_id);
>>>>
>>>>    	mgr = devm_fpga_mgr_create(dev, "DFL FME FPGA Manager",
>>>> @@ -323,8 +318,6 @@ static int fme_mgr_probe(struct platform_device
>> *pdev)
>>>>    	if (!mgr)
>>>>    		return -ENOMEM;
>>>>
>>>> -	mgr->compat_id = compat_id;
>>>> -
>>>>    	return devm_fpga_mgr_register(dev, mgr);
>>>>    }
>>>>
>>>> diff --git a/drivers/fpga/dfl-fme-region.c b/drivers/fpga/dfl-fme-region.c
>>>> index d21eacbf2469f..be1d57ee37666 100644
>>>> --- a/drivers/fpga/dfl-fme-region.c
>>>> +++ b/drivers/fpga/dfl-fme-region.c
>>>> @@ -64,7 +64,6 @@ static int fme_region_probe(struct platform_device
>> *pdev)
>>>>    	}
>>>>
>>>>    	region->priv = pdata;
>>>> -	region->compat_id = mgr->compat_id;
>>>>    	platform_set_drvdata(pdev, region);
>>>>
>>>>    	ret = fpga_region_register(region);
>>>> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
>>>> index 864dd4f290e3b..b08d3914716f0 100644
>>>> --- a/drivers/fpga/fpga-region.c
>>>> +++ b/drivers/fpga/fpga-region.c
>>>> @@ -172,12 +172,7 @@ static ssize_t compat_id_show(struct device *dev,
>>>>    	if (region->rops && region->rops->compat_id_show)
>>>>    		return region->rops->compat_id_show(region, buf);
>>>>
>>>> -	if (!region->compat_id)
>>>> -		return -ENOENT;
>>>> -
>>>> -	return sprintf(buf, "%016llx%016llx\n",
>>>> -		       (unsigned long long)region->compat_id->id_h,
>>>> -		       (unsigned long long)region->compat_id->id_l);
>>>> +	return -ENOENT;
>>>>    }
>>>>
>>>>    static DEVICE_ATTR_RO(compat_id);
>>>> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
>>>> index ec2cd8bfceb00..ebdea215a8643 100644
>>>> --- a/include/linux/fpga/fpga-mgr.h
>>>> +++ b/include/linux/fpga/fpga-mgr.h
>>>> @@ -143,24 +143,12 @@ struct fpga_manager_ops {
>>>>    #define FPGA_MGR_STATUS_IP_PROTOCOL_ERR		BIT(3)
>>>>    #define FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR	BIT(4)
>>>>
>>>> -/**
>>>> - * struct fpga_compat_id - id for compatibility check
>>>> - *
>>>> - * @id_h: high 64bit of the compat_id
>>>> - * @id_l: low 64bit of the compat_id
>>>> - */
>>>> -struct fpga_compat_id {
>>>> -	u64 id_h;
>>>> -	u64 id_l;
>>>> -};
>>>> -
>>>>    /**
>>>>     * struct fpga_manager - fpga manager structure
>>>>     * @name: name of low level fpga manager
>>>>     * @dev: fpga manager device
>>>>     * @ref_mutex: only allows one reference to fpga manager
>>>>     * @state: state of fpga manager
>>>> - * @compat_id: FPGA manager id for compatibility check.
>>>>     * @mops: pointer to struct of fpga manager ops
>>>>     * @priv: low level driver private date
>>>>     */
>>>> @@ -169,7 +157,6 @@ struct fpga_manager {
>>>>    	struct device dev;
>>>>    	struct mutex ref_mutex;
>>>>    	enum fpga_mgr_states state;
>>>> -	struct fpga_compat_id *compat_id;
>>>>    	const struct fpga_manager_ops *mops;
>>>>    	void *priv;
>>>>    };
>>>> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-
>> region.h
>>>> index 236d3819f1c13..afc79784b2823 100644
>>>> --- a/include/linux/fpga/fpga-region.h
>>>> +++ b/include/linux/fpga/fpga-region.h
>>>> @@ -30,7 +30,6 @@ struct fpga_region_ops {
>>>>     * @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
>>>>     * @rops: optional pointer to struct for fpga region ops
>>>>     */
>>>> @@ -40,7 +39,6 @@ struct fpga_region {
>>>>    	struct list_head bridge_list;
>>>>    	struct fpga_manager *mgr;
>>>>    	struct fpga_image_info *info;
>>>> -	struct fpga_compat_id *compat_id;
>>>>    	void *priv;
>>>>    	const struct fpga_region_ops *rops;
>>>>    };
>>>> --
>>>> 2.26.3

RE: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and fpga_region

[Wu, Hao]

 
> On 7/20/21 9:48 PM, Wu, Hao wrote:
> >> On 7/11/21 6:40 PM, Wu, Hao wrote:
> >>>> -----Original Message-----
> >>>> From: trix@redhat.com <trix@redhat.com>
> >>>> Sent: Friday, July 9, 2021 9:42 PM
> >>>> To: mdf@kernel.org; corbet@lwn.net; Wu, Hao <hao.wu@intel.com>
> >>>> Cc: linux-fpga@vger.kernel.org; linux-doc@vger.kernel.org; linux-
> >>>> kernel@vger.kernel.org; Tom Rix <trix@redhat.com>
> >>>> Subject: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and
> >>>> fpga_region
> >>>>
> >>>> From: Tom Rix <trix@redhat.com>
> >>>>
> >>>> compat_id is implementation specific.  So the data should be
> >>>> stored at the implemeation layer, not the infrastructure layer.
> >>>> Remove the compat_id elements and supporting code.
> >>> I think current compat_id format can meet the checking requirement.
> >>> Actually I hope other hardware which needs compatible checking
> >>> to expose the same format compat_id. Then we can have more
> >>> unified/common code, e.g. userspace application/lib handling.
> >> v2 does not change the current ABI. The dfl output is the same as
> >> before, the other nonusers get -ENOENT.
> > I think the common ABI is changed somehow, as output format can
> > be anything with your change, this confuses userspace too.
> 
> Only dfl uses this interface, any dfl userspace like opae reading the
> sysfs compat_id would remain unchanged.
> 
> Others will continue to receive the -ENOENT.
> 
> If the others wanted to use this entry in the future, the
> 
> existing ABI documentation is consistent with with allowing them to
> 
> define it as they wish.  The format of the output is not specified
> 
> only the error condition. with language the leaves it up to the region
> 
> creator to define.
> 
> from sysfs-class-fpga-region
> 
> "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."
> 

As we have fixed compat_id format, so the output format is fixed.
If output format is not fixed then we will never have reusable code based
on this common ABI on fpga region, only vendor specific code can.

> >
> >> For dfl compat_id is 2 64 bit registers.
> >>
> >> For compat_id to be useful to the others, they need the flexibility to
> >> print to the sysfs in the manner that aligns with whatever their user
> >> library interface is, 2 64 values isn't going to work for everyone.  ex/
> >> xrt likely would be a uuid_t printed out a special way. someone else
> >> maybe just string in the board fw, maybe some has a 8 or 256 bits of
> >> compat_id  etc.
> >>
> >> as a driver region specific op, others are free to do whatever is required.
> >>
> >>> Currently I didn't see any other usage or requirement on this part
> >>> now, only DFL uses it.  So should we leave it here at this moment?
> >>> I feel we don't have to change it for now to move it to a
> >>> Per-fpga-mgr format. : )
> >> The motivation for doing this now is the 'use standard class dev release
> >> .. ' patchset
> >>
> >> I really do not like 2 register functions.
> >>
> >> By moving compat_id, the 2 register functions reduces down to 1.
> >>
> > You don't have to moving compact_id, you can have 1 parameter
> > with a data structure including everything.
> 
> I like the fpga_mgr_register( ... , const struct fpga_magager_info
> *info) better as well because it will stabilize the public api.
> 
> Since we agree on that, do you agree Russ's patch can be resolved by
> 
> from include/linux/fpga-mgr.h
> 
> keep
> 
> struct fpga_manager *
> fpga_mgr_register(struct device *parent, const struct fpga_manager_info
> *info);
> 
> remove *simple() from the public api, move it to driver/fpga/

Yes, that sounds good to me.

> 
> and something similar for fpga-region.h ?
> 
> However the compat_id refactor goes, having just *register(... *info) is
> fine and could be done first.

Yes. Adding or removing thing later won't impact this register interface.

Hao

> 
> Tom
> 
> 
> >
> > Thanks
> > Hao
> >
> >> I did a poc here
> >>
> >> https://lore.kernel.org/linux-fpga/20210709184511.2521508-1-
> >> trix@redhat.com/
> >>
> >> Tom
> >>
> >>> Thanks
> >>> Hao
> >>>
> >>>> Printing out the compat_id is done with the fpga_region
> >>>> compat_id_show() op.
> >>>>
> >>>> Signed-off-by: Tom Rix <trix@redhat.com>
> >>>> ---
> >>>>    drivers/fpga/dfl-fme-mgr.c       |  7 -------
> >>>>    drivers/fpga/dfl-fme-region.c    |  1 -
> >>>>    drivers/fpga/fpga-region.c       |  7 +------
> >>>>    include/linux/fpga/fpga-mgr.h    | 13 -------------
> >>>>    include/linux/fpga/fpga-region.h |  2 --
> >>>>    5 files changed, 1 insertion(+), 29 deletions(-)
> >>>>
> >>>> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
> >>>> index cd0b9157ea6e5..8c5423eeffe75 100644
> >>>> --- a/drivers/fpga/dfl-fme-mgr.c
> >>>> +++ b/drivers/fpga/dfl-fme-mgr.c
> >>>> @@ -292,7 +292,6 @@ EXPORT_SYMBOL_GPL(fme_mgr_get_compat_id);
> >>>>    static int fme_mgr_probe(struct platform_device *pdev)
> >>>>    {
> >>>>    	struct dfl_fme_mgr_pdata *pdata = dev_get_platdata(&pdev->dev);
> >>>> -	struct fpga_compat_id *compat_id;
> >>>>    	struct device *dev = &pdev->dev;
> >>>>    	struct fme_mgr_priv *priv;
> >>>>    	struct fpga_manager *mgr;
> >>>> @@ -312,10 +311,6 @@ static int fme_mgr_probe(struct platform_device
> >>>> *pdev)
> >>>>    			return PTR_ERR(priv->ioaddr);
> >>>>    	}
> >>>>
> >>>> -	compat_id = devm_kzalloc(dev, sizeof(*compat_id), GFP_KERNEL);
> >>>> -	if (!compat_id)
> >>>> -		return -ENOMEM;
> >>>> -
> >>>>    	_fme_mgr_get_compat_id(priv->ioaddr, &priv->compat_id);
> >>>>
> >>>>    	mgr = devm_fpga_mgr_create(dev, "DFL FME FPGA Manager",
> >>>> @@ -323,8 +318,6 @@ static int fme_mgr_probe(struct platform_device
> >> *pdev)
> >>>>    	if (!mgr)
> >>>>    		return -ENOMEM;
> >>>>
> >>>> -	mgr->compat_id = compat_id;
> >>>> -
> >>>>    	return devm_fpga_mgr_register(dev, mgr);
> >>>>    }
> >>>>
> >>>> diff --git a/drivers/fpga/dfl-fme-region.c b/drivers/fpga/dfl-fme-region.c
> >>>> index d21eacbf2469f..be1d57ee37666 100644
> >>>> --- a/drivers/fpga/dfl-fme-region.c
> >>>> +++ b/drivers/fpga/dfl-fme-region.c
> >>>> @@ -64,7 +64,6 @@ static int fme_region_probe(struct platform_device
> >> *pdev)
> >>>>    	}
> >>>>
> >>>>    	region->priv = pdata;
> >>>> -	region->compat_id = mgr->compat_id;
> >>>>    	platform_set_drvdata(pdev, region);
> >>>>
> >>>>    	ret = fpga_region_register(region);
> >>>> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> >>>> index 864dd4f290e3b..b08d3914716f0 100644
> >>>> --- a/drivers/fpga/fpga-region.c
> >>>> +++ b/drivers/fpga/fpga-region.c
> >>>> @@ -172,12 +172,7 @@ static ssize_t compat_id_show(struct device *dev,
> >>>>    	if (region->rops && region->rops->compat_id_show)
> >>>>    		return region->rops->compat_id_show(region, buf);
> >>>>
> >>>> -	if (!region->compat_id)
> >>>> -		return -ENOENT;
> >>>> -
> >>>> -	return sprintf(buf, "%016llx%016llx\n",
> >>>> -		       (unsigned long long)region->compat_id->id_h,
> >>>> -		       (unsigned long long)region->compat_id->id_l);
> >>>> +	return -ENOENT;
> >>>>    }
> >>>>
> >>>>    static DEVICE_ATTR_RO(compat_id);
> >>>> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> >>>> index ec2cd8bfceb00..ebdea215a8643 100644
> >>>> --- a/include/linux/fpga/fpga-mgr.h
> >>>> +++ b/include/linux/fpga/fpga-mgr.h
> >>>> @@ -143,24 +143,12 @@ struct fpga_manager_ops {
> >>>>    #define FPGA_MGR_STATUS_IP_PROTOCOL_ERR		BIT(3)
> >>>>    #define FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR	BIT(4)
> >>>>
> >>>> -/**
> >>>> - * struct fpga_compat_id - id for compatibility check
> >>>> - *
> >>>> - * @id_h: high 64bit of the compat_id
> >>>> - * @id_l: low 64bit of the compat_id
> >>>> - */
> >>>> -struct fpga_compat_id {
> >>>> -	u64 id_h;
> >>>> -	u64 id_l;
> >>>> -};
> >>>> -
> >>>>    /**
> >>>>     * struct fpga_manager - fpga manager structure
> >>>>     * @name: name of low level fpga manager
> >>>>     * @dev: fpga manager device
> >>>>     * @ref_mutex: only allows one reference to fpga manager
> >>>>     * @state: state of fpga manager
> >>>> - * @compat_id: FPGA manager id for compatibility check.
> >>>>     * @mops: pointer to struct of fpga manager ops
> >>>>     * @priv: low level driver private date
> >>>>     */
> >>>> @@ -169,7 +157,6 @@ struct fpga_manager {
> >>>>    	struct device dev;
> >>>>    	struct mutex ref_mutex;
> >>>>    	enum fpga_mgr_states state;
> >>>> -	struct fpga_compat_id *compat_id;
> >>>>    	const struct fpga_manager_ops *mops;
> >>>>    	void *priv;
> >>>>    };
> >>>> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-
> >> region.h
> >>>> index 236d3819f1c13..afc79784b2823 100644
> >>>> --- a/include/linux/fpga/fpga-region.h
> >>>> +++ b/include/linux/fpga/fpga-region.h
> >>>> @@ -30,7 +30,6 @@ struct fpga_region_ops {
> >>>>     * @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
> >>>>     * @rops: optional pointer to struct for fpga region ops
> >>>>     */
> >>>> @@ -40,7 +39,6 @@ struct fpga_region {
> >>>>    	struct list_head bridge_list;
> >>>>    	struct fpga_manager *mgr;
> >>>>    	struct fpga_image_info *info;
> >>>> -	struct fpga_compat_id *compat_id;
> >>>>    	void *priv;
> >>>>    	const struct fpga_region_ops *rops;
> >>>>    };
> >>>> --
> >>>> 2.26.3

Re: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and fpga_region

[Tom Rix]

On 7/21/21 5:18 PM, Wu, Hao wrote:
>   
>> On 7/20/21 9:48 PM, Wu, Hao wrote:
>>>> On 7/11/21 6:40 PM, Wu, Hao wrote:
>>>>>> -----Original Message-----
>>>>>> From: trix@redhat.com <trix@redhat.com>
>>>>>> Sent: Friday, July 9, 2021 9:42 PM
>>>>>> To: mdf@kernel.org; corbet@lwn.net; Wu, Hao <hao.wu@intel.com>
>>>>>> Cc: linux-fpga@vger.kernel.org; linux-doc@vger.kernel.org; linux-
>>>>>> kernel@vger.kernel.org; Tom Rix <trix@redhat.com>
>>>>>> Subject: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and
>>>>>> fpga_region
>>>>>>
>>>>>> From: Tom Rix <trix@redhat.com>
>>>>>>
>>>>>> compat_id is implementation specific.  So the data should be
>>>>>> stored at the implemeation layer, not the infrastructure layer.
>>>>>> Remove the compat_id elements and supporting code.
>>>>> I think current compat_id format can meet the checking requirement.
>>>>> Actually I hope other hardware which needs compatible checking
>>>>> to expose the same format compat_id. Then we can have more
>>>>> unified/common code, e.g. userspace application/lib handling.
>>>> v2 does not change the current ABI. The dfl output is the same as
>>>> before, the other nonusers get -ENOENT.
>>> I think the common ABI is changed somehow, as output format can
>>> be anything with your change, this confuses userspace too.
>> Only dfl uses this interface, any dfl userspace like opae reading the
>> sysfs compat_id would remain unchanged.
>>
>> Others will continue to receive the -ENOENT.
>>
>> If the others wanted to use this entry in the future, the
>>
>> existing ABI documentation is consistent with with allowing them to
>>
>> define it as they wish.  The format of the output is not specified
>>
>> only the error condition. with language the leaves it up to the region
>>
>> creator to define.
>>
>> from sysfs-class-fpga-region
>>
>> "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."
>>
> As we have fixed compat_id format, so the output format is fixed.
> If output format is not fixed then we will never have reusable code based
> on this common ABI on fpga region, only vendor specific code can.

Looking for a compromise that leaves the data in fpga_manager,

The data type of currently is vendor specific, 2 64 bit values.

can we change that to a neutral type like uuid_t ?

It is treated as a uuid_t in opae, with.

being read byte string with this logic

     for (i = 0; i < 32; i += 2) {
         tmp = buf[i + 2];
         buf[i + 2] = 0;

         octet = 0;
         sscanf(&buf[i], "%x", &octet);
         guid[i / 2] = (uint8_t)octet;

         buf[i + 2] = tmp;
     }

Into this final type

/**
  * Globally unique identifier (GUID)
  *
  * GUIDs are used widely within OPAE for helping identify FPGA 
resources. For
  * example, every FPGA resource has a `guid` property, which can be 
(and in the
  * case of FPGA_ACCELERATOR resource primarily is) used for enumerating 
a resource of a
  * specific type.
   *
  * `fpga_guid` is compatible with libuuid's uuid_t, so users can use 
libuuid
  * functions like uuid_parse() to create and work with GUIDs.
  */
typedef uint8_t fpga_guid[16];

Tom


>
>>>> For dfl compat_id is 2 64 bit registers.
>>>>
>>>> For compat_id to be useful to the others, they need the flexibility to
>>>> print to the sysfs in the manner that aligns with whatever their user
>>>> library interface is, 2 64 values isn't going to work for everyone.  ex/
>>>> xrt likely would be a uuid_t printed out a special way. someone else
>>>> maybe just string in the board fw, maybe some has a 8 or 256 bits of
>>>> compat_id  etc.
>>>>
>>>> as a driver region specific op, others are free to do whatever is required.
>>>>
>>>>> Currently I didn't see any other usage or requirement on this part
>>>>> now, only DFL uses it.  So should we leave it here at this moment?
>>>>> I feel we don't have to change it for now to move it to a
>>>>> Per-fpga-mgr format. : )
>>>> The motivation for doing this now is the 'use standard class dev release
>>>> .. ' patchset
>>>>
>>>> I really do not like 2 register functions.
>>>>
>>>> By moving compat_id, the 2 register functions reduces down to 1.
>>>>
>>> You don't have to moving compact_id, you can have 1 parameter
>>> with a data structure including everything.
>> I like the fpga_mgr_register( ... , const struct fpga_magager_info
>> *info) better as well because it will stabilize the public api.
>>
>> Since we agree on that, do you agree Russ's patch can be resolved by
>>
>> from include/linux/fpga-mgr.h
>>
>> keep
>>
>> struct fpga_manager *
>> fpga_mgr_register(struct device *parent, const struct fpga_manager_info
>> *info);
>>
>> remove *simple() from the public api, move it to driver/fpga/
> Yes, that sounds good to me.
>
>> and something similar for fpga-region.h ?
>>
>> However the compat_id refactor goes, having just *register(... *info) is
>> fine and could be done first.
> Yes. Adding or removing thing later won't impact this register interface.
>
> Hao
>
>> Tom
>>
>>
>>> Thanks
>>> Hao
>>>
>>>> I did a poc here
>>>>
>>>> https://lore.kernel.org/linux-fpga/20210709184511.2521508-1-
>>>> trix@redhat.com/
>>>>
>>>> Tom
>>>>
>>>>> Thanks
>>>>> Hao
>>>>>
>>>>>> Printing out the compat_id is done with the fpga_region
>>>>>> compat_id_show() op.
>>>>>>
>>>>>> Signed-off-by: Tom Rix <trix@redhat.com>
>>>>>> ---
>>>>>>     drivers/fpga/dfl-fme-mgr.c       |  7 -------
>>>>>>     drivers/fpga/dfl-fme-region.c    |  1 -
>>>>>>     drivers/fpga/fpga-region.c       |  7 +------
>>>>>>     include/linux/fpga/fpga-mgr.h    | 13 -------------
>>>>>>     include/linux/fpga/fpga-region.h |  2 --
>>>>>>     5 files changed, 1 insertion(+), 29 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
>>>>>> index cd0b9157ea6e5..8c5423eeffe75 100644
>>>>>> --- a/drivers/fpga/dfl-fme-mgr.c
>>>>>> +++ b/drivers/fpga/dfl-fme-mgr.c
>>>>>> @@ -292,7 +292,6 @@ EXPORT_SYMBOL_GPL(fme_mgr_get_compat_id);
>>>>>>     static int fme_mgr_probe(struct platform_device *pdev)
>>>>>>     {
>>>>>>     	struct dfl_fme_mgr_pdata *pdata = dev_get_platdata(&pdev->dev);
>>>>>> -	struct fpga_compat_id *compat_id;
>>>>>>     	struct device *dev = &pdev->dev;
>>>>>>     	struct fme_mgr_priv *priv;
>>>>>>     	struct fpga_manager *mgr;
>>>>>> @@ -312,10 +311,6 @@ static int fme_mgr_probe(struct platform_device
>>>>>> *pdev)
>>>>>>     			return PTR_ERR(priv->ioaddr);
>>>>>>     	}
>>>>>>
>>>>>> -	compat_id = devm_kzalloc(dev, sizeof(*compat_id), GFP_KERNEL);
>>>>>> -	if (!compat_id)
>>>>>> -		return -ENOMEM;
>>>>>> -
>>>>>>     	_fme_mgr_get_compat_id(priv->ioaddr, &priv->compat_id);
>>>>>>
>>>>>>     	mgr = devm_fpga_mgr_create(dev, "DFL FME FPGA Manager",
>>>>>> @@ -323,8 +318,6 @@ static int fme_mgr_probe(struct platform_device
>>>> *pdev)
>>>>>>     	if (!mgr)
>>>>>>     		return -ENOMEM;
>>>>>>
>>>>>> -	mgr->compat_id = compat_id;
>>>>>> -
>>>>>>     	return devm_fpga_mgr_register(dev, mgr);
>>>>>>     }
>>>>>>
>>>>>> diff --git a/drivers/fpga/dfl-fme-region.c b/drivers/fpga/dfl-fme-region.c
>>>>>> index d21eacbf2469f..be1d57ee37666 100644
>>>>>> --- a/drivers/fpga/dfl-fme-region.c
>>>>>> +++ b/drivers/fpga/dfl-fme-region.c
>>>>>> @@ -64,7 +64,6 @@ static int fme_region_probe(struct platform_device
>>>> *pdev)
>>>>>>     	}
>>>>>>
>>>>>>     	region->priv = pdata;
>>>>>> -	region->compat_id = mgr->compat_id;
>>>>>>     	platform_set_drvdata(pdev, region);
>>>>>>
>>>>>>     	ret = fpga_region_register(region);
>>>>>> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
>>>>>> index 864dd4f290e3b..b08d3914716f0 100644
>>>>>> --- a/drivers/fpga/fpga-region.c
>>>>>> +++ b/drivers/fpga/fpga-region.c
>>>>>> @@ -172,12 +172,7 @@ static ssize_t compat_id_show(struct device *dev,
>>>>>>     	if (region->rops && region->rops->compat_id_show)
>>>>>>     		return region->rops->compat_id_show(region, buf);
>>>>>>
>>>>>> -	if (!region->compat_id)
>>>>>> -		return -ENOENT;
>>>>>> -
>>>>>> -	return sprintf(buf, "%016llx%016llx\n",
>>>>>> -		       (unsigned long long)region->compat_id->id_h,
>>>>>> -		       (unsigned long long)region->compat_id->id_l);
>>>>>> +	return -ENOENT;
>>>>>>     }
>>>>>>
>>>>>>     static DEVICE_ATTR_RO(compat_id);
>>>>>> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
>>>>>> index ec2cd8bfceb00..ebdea215a8643 100644
>>>>>> --- a/include/linux/fpga/fpga-mgr.h
>>>>>> +++ b/include/linux/fpga/fpga-mgr.h
>>>>>> @@ -143,24 +143,12 @@ struct fpga_manager_ops {
>>>>>>     #define FPGA_MGR_STATUS_IP_PROTOCOL_ERR		BIT(3)
>>>>>>     #define FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR	BIT(4)
>>>>>>
>>>>>> -/**
>>>>>> - * struct fpga_compat_id - id for compatibility check
>>>>>> - *
>>>>>> - * @id_h: high 64bit of the compat_id
>>>>>> - * @id_l: low 64bit of the compat_id
>>>>>> - */
>>>>>> -struct fpga_compat_id {
>>>>>> -	u64 id_h;
>>>>>> -	u64 id_l;
>>>>>> -};
>>>>>> -
>>>>>>     /**
>>>>>>      * struct fpga_manager - fpga manager structure
>>>>>>      * @name: name of low level fpga manager
>>>>>>      * @dev: fpga manager device
>>>>>>      * @ref_mutex: only allows one reference to fpga manager
>>>>>>      * @state: state of fpga manager
>>>>>> - * @compat_id: FPGA manager id for compatibility check.
>>>>>>      * @mops: pointer to struct of fpga manager ops
>>>>>>      * @priv: low level driver private date
>>>>>>      */
>>>>>> @@ -169,7 +157,6 @@ struct fpga_manager {
>>>>>>     	struct device dev;
>>>>>>     	struct mutex ref_mutex;
>>>>>>     	enum fpga_mgr_states state;
>>>>>> -	struct fpga_compat_id *compat_id;
>>>>>>     	const struct fpga_manager_ops *mops;
>>>>>>     	void *priv;
>>>>>>     };
>>>>>> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-
>>>> region.h
>>>>>> index 236d3819f1c13..afc79784b2823 100644
>>>>>> --- a/include/linux/fpga/fpga-region.h
>>>>>> +++ b/include/linux/fpga/fpga-region.h
>>>>>> @@ -30,7 +30,6 @@ struct fpga_region_ops {
>>>>>>      * @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
>>>>>>      * @rops: optional pointer to struct for fpga region ops
>>>>>>      */
>>>>>> @@ -40,7 +39,6 @@ struct fpga_region {
>>>>>>     	struct list_head bridge_list;
>>>>>>     	struct fpga_manager *mgr;
>>>>>>     	struct fpga_image_info *info;
>>>>>> -	struct fpga_compat_id *compat_id;
>>>>>>     	void *priv;
>>>>>>     	const struct fpga_region_ops *rops;
>>>>>>     };
>>>>>> --
>>>>>> 2.26.3

Re: [PATCH v2 0/4] fpga: fpga-mgr: move compat_id from fpga_mgr to dfl

[Tom Rix]

On 7/9/21 6:42 AM, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
>
> A followup to
> https://lore.kernel.org/linux-fpga/aa06a7ca-eff3-5c0d-f3b0-f1d9ddb74526@redhat.com/
> The current storing of compat_id in fpga_manager is dfl specific.
> This makes the refactoring of the release()'s complicated because there
> is a dfl specific flavor of register().
>
> Keeping the compat_id sysfs abi, each implementation through the new
> compat_id_show() fpga_region op can print out whatever value they need
> to the sysfs.  Currently only dfl does.
>
> Since there are now two ops for fpga_region, give fpga_region its
> own ops table.  Add a wrapper for get_bridges().
>
> Changes from
> v1
>    Completely written to keep sysfs abi

Moritz and Hao,

Can you comment on v2 ?

The compat_id abi is unchanged, so dfl's opae userspace access will not 
change.

For ever other board, the nonuse error is the same.

Otherwise, the new region ops is consistent with manger ops.

I can split this first patch out if the refactor of compat_id is 
contentious .

Tom

>
> Tom Rix (4):
>    fpga: region: introduce fpga_region_ops
>    fpga: region: introduce compat_id_show op
>    fpga: dfl: implement the compat_id_show region op
>    fpga: remove compat_id from fpga_manager and fpga_region
>
>   Documentation/driver-api/fpga/fpga-region.rst |  6 ++-
>   drivers/fpga/dfl-fme-mgr.c                    | 23 ++++++-----
>   drivers/fpga/dfl-fme-pr.c                     |  2 +-
>   drivers/fpga/dfl-fme-region.c                 | 21 +++++++++-
>   drivers/fpga/dfl.h                            | 14 +++++++
>   drivers/fpga/fpga-region.c                    | 40 ++++++++++---------
>   drivers/fpga/of-fpga-region.c                 |  6 ++-
>   include/linux/fpga/fpga-mgr.h                 | 13 ------
>   include/linux/fpga/fpga-region.h              | 26 +++++++++---
>   9 files changed, 99 insertions(+), 52 deletions(-)
>