Re: [PATCH] fpga: region: handle compat_id as an uuid
From: Tom Rix
Date: Mon Jul 26 2021 - 20:16:56 EST
On 7/26/21 3:12 PM, Russ Weight wrote:
On 7/26/21 1:26 PM, trix@xxxxxxxxxx wrote:
From: Tom Rix <trix@xxxxxxxxxx>
An fpga region's compat_id is exported by the sysfs
as a 128 bit hex string formed by concatenating two
64 bit values together.
The only user of compat_id is dfl. Its user library
opae converts this value into a uuid.
ex/
$ cat /sys/class/fpga_region/region1/compat_id
f3c9941350814aadbced07eb84a6d0bb
Is reported as
$ fpgainfo bmc
...
Pr Interface Id : f3c99413-5081-4aad-bced-07eb84a6d0bb
Storing a uuid as 2 64 bit values is vendor specific.
And concatenating them together is vendor specific.
It is better to store and print out as a vendor neutral uuid.
Change fpga_compat_id from a struct to a union.
Keep the old 64 bit values for dfl.
Sysfs output is now
f3c99413-5081-4aad-bced-07eb84a6d0bb
I'm fowarding feedback from Tim Whisonant, one of the OPAE userspace
developers:
I think that this change to the sysfs for the compat_id node will
end up breaking the SDK, which does not expect the '-' characters to
be included when parsing the sysfs value. Currently, it is parsed as
a raw hex string without regard to any '-' characters. This goes for
any "guid" currently exported by sysfs and for what we read in the
device MMIO space.
Yes, it will.
And there are other places, like dfl-afu-main.c:afu_id_show()
outputs raw hex that sdk turns into a uuid.
Some options.
If no one but dfl will ever use it, then v1 of patchset.
If others can use it but don't want to change dfl, then v2 of patchset,
my favorite.
Or this one for uuid for everyone, what have been v3 but changed too much.
could dfl change generally to output uuid's to the sysfs ?
this would be generally helpful and a one time disruption to the sdk.
Tom
- Russ
Signed-off-by: Tom Rix <trix@xxxxxxxxxx>
---
.../ABI/testing/sysfs-class-fpga-region | 4 ++--
drivers/fpga/dfl-fme-mgr.c | 8 ++++----
drivers/fpga/fpga-region.c | 4 +---
include/linux/fpga/fpga-mgr.h | 18 ++++++++++++------
include/linux/fpga/fpga-region.h | 2 +-
5 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-class-fpga-region b/Documentation/ABI/testing/sysfs-class-fpga-region
index bc7ec644acc9a..241359fb74a55 100644
--- a/Documentation/ABI/testing/sysfs-class-fpga-region
+++ b/Documentation/ABI/testing/sysfs-class-fpga-region
@@ -5,5 +5,5 @@ Contact: Wu Hao <hao.wu@xxxxxxxxx>
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.
+ FPGA region. This interface returns a uuid value or just
+ error code -ENOENT in case compat_id is not used.
diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
index d5861d13b3069..012b72712684c 100644
--- a/drivers/fpga/dfl-fme-mgr.c
+++ b/drivers/fpga/dfl-fme-mgr.c
@@ -273,16 +273,16 @@ static const struct fpga_manager_ops fme_mgr_ops = {
};
static void fme_mgr_get_compat_id(void __iomem *fme_pr,
- struct fpga_compat_id *id)
+ union fpga_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);
+ id->id_l = cpu_to_be64(readq(fme_pr + FME_PR_INTFC_ID_L));
+ id->id_h = cpu_to_be64(readq(fme_pr + FME_PR_INTFC_ID_H));
}
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;
+ union fpga_compat_id *compat_id;
struct device *dev = &pdev->dev;
struct fme_mgr_priv *priv;
struct fpga_manager *mgr;
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index a4838715221ff..f1083b5894635 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -166,9 +166,7 @@ static ssize_t compat_id_show(struct device *dev,
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 sprintf(buf, "%pU\n", ®ion->compat_id->uuid);
}
static DEVICE_ATTR_RO(compat_id);
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index ec2cd8bfceb00..b12f9994932e1 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -10,6 +10,7 @@
#include <linux/mutex.h>
#include <linux/platform_device.h>
+#include <linux/uuid.h>
struct fpga_manager;
struct sg_table;
@@ -144,14 +145,19 @@ struct fpga_manager_ops {
#define FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR BIT(4)
/**
- * struct fpga_compat_id - id for compatibility check
- *
+ * union fpga_compat_id - id for compatibility check
+ * Can be accessed as either:
+ * @uuid: the base uuid_t type
+ * or
* @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;
+union fpga_compat_id {
+ uuid_t uuid;
+ struct {
+ u64 id_h;
+ u64 id_l;
+ };
};
/**
@@ -169,7 +175,7 @@ struct fpga_manager {
struct device dev;
struct mutex ref_mutex;
enum fpga_mgr_states state;
- struct fpga_compat_id *compat_id;
+ union 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 27cb706275dba..7cc2ee543efb4 100644
--- a/include/linux/fpga/fpga-region.h
+++ b/include/linux/fpga/fpga-region.h
@@ -24,7 +24,7 @@ struct fpga_region {
struct list_head bridge_list;
struct fpga_manager *mgr;
struct fpga_image_info *info;
- struct fpga_compat_id *compat_id;
+ union fpga_compat_id *compat_id;
void *priv;
int (*get_bridges)(struct fpga_region *region);
};