LinuxLists.cc - [PATCH] fpga: region: handle compat

2021-07-26 20:28:00

Subject: [PATCH] fpga: region: handle compat_id as an uuid

From: Tom Rix <[email protected]>

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

Signed-off-by: Tom Rix <[email protected]>
---
.../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 <[email protected]>
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", &region->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);
};
--
2.26.3

2021-07-26 22:16:23

by Russ Weight

[permalink] [raw]

Subject: Re: [PATCH] fpga: region: handle compat_id as an uuid

On 7/26/21 1:26 PM, [email protected] wrote:
> From: Tom Rix <[email protected]>
>
> 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.

- Russ

>
> Signed-off-by: Tom Rix <[email protected]>
> ---
> .../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 <[email protected]>
> 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", &region->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);
> };

2021-07-26 23:36:54

On Wed, Aug 04, 2021 at 06:21:55AM -0700, Tom Rix wrote:
>
> On 8/3/21 6:44 PM, Xu Yilun wrote:
> > On Tue, Aug 03, 2021 at 05:21:54AM -0700, Tom Rix wrote:
> > > On 8/2/21 8:32 PM, Xu Yilun wrote:
> > > > On Fri, Jul 30, 2021 at 05:07:00AM -0700, Tom Rix wrote:
> > > > > On 7/29/21 6:48 PM, Xu Yilun wrote:
> > > > > > On Thu, Jul 29, 2021 at 12:16:47PM -0700, Tom Rix wrote:
> > > > > > > On 7/29/21 11:51 AM, Moritz Fischer wrote:
> > > > > > > > On Wed, Jul 28, 2021 at 01:36:56AM +0000, Wu, Hao wrote:
> > > > > > > > > > On 7/26/21 3:12 PM, Russ Weight wrote:
> > > > > > > > > > > On 7/26/21 1:26 PM, [email protected] wrote:
> > > > > > > > > > > > From: Tom Rix <[email protected]>
> > > > > > > > > > > >
> > > > > > > > > > > > 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.
> > > > > > > > > This change limited the output format to uuid_t, but if any hardware doesn't
> > > > > > > > > use uuid_t on hardware may have to convert it back from the sysfs output in
> > > > > > > > > userspace. Leave it to print hardware values (e.g. from register), and convert
> > > > > > > > > it in userspace should be fine too I think.
> > > > > > > > I'm not entirely sure. I seem to recall there being examples of sysfs
> > > > > > > > files returning different things for different drivers.
> > > > > > > >
> > > > > > > > That being said it seems largely cosmetic to add the '-' in between.
> > > > > > > >
> > > > > > > > If it breaks userspace, I'm against it. If you *need* it make a
> > > > > > > > compat_uuid entry or something in that case?
> > > > > > > My gripe is
> > > > > > >
> > > > > > > For a nominally common interface, compat_id has a vendor specific output.
> > > > > > >
> > > > > > > If for example another vendor wanted to use this field but their natural
> > > > > > > format was an OF string.
> > > > > > >
> > > > > > > 16 bytes of raw hex would not work for them, so they would roll their own.
> > > > > > >
> > > > > > > which defeats the purpose of a common interface.
> > > > > > >
> > > > > > >
> > > > > > > The language in the docs as-is is vague on the output format.
> > > > > > >
> > > > > > > DFL is the only user of the interface.
> > > > > > >
> > > > > > > So ver 2
> > > > > > >
> > > > > > > https://lore.kernel.org/linux-fpga/[email protected]/
> > > > > > >
> > > > > > > Keeps the output as-is for dfl, so nothing breaks in userspace
> > > > > > >
> > > > > > > And adds flexibility for vendors to output their appropriate natural form.
> > > > > > >
> > > > > > > So compat_id becomes generally useful.
> > > > > > Mixing types seems be strongly against in Documentation/filesystems/sysfs.rst.
> > > > > > So in my opinion there should be a determined format for the output. The
> > > > > > concern for this patch is which one is a better format, uuid style or
> > > > > > 128 bit raw hex?
> > > > > >
> > > > > > And I vote for 128 bit raw hex, as other vendors may not use uuid_t as
> > > > > > the identifier, may be an OF string. So we don't have to force them
> > > > > > decorate it as the uuid style.
> > > > > So you would be ok with v2 of this patchset ?
> > > > No. I prefer we keep the current implementation. I mean 128 bit raw hex
> > > > could be a more compatible output format for all vendors, uuid style,
> > > > string style or others.
> > > How would 128 bit raw hex be compatible if a vendor's natural format was an
> > > OF string ?
> > >
> > > Why would they even use compat_id ?
> > As I mentioned before, I assume there should be a unified output format
> > for all vendors. So we need to choose one from raw hex, uuid_t, string
> > and all other formats. Raw hex is the fundamental format, which could be
> > further interpreted by user as a uuid_t, a string or other objects.
>
> Having one format is not a requirement, the format is not listed in the
> docs.

My understanding comes from the "Mixing types" description in
Documentation/filesystems/sysfs.txt, I assume a sysfs node should not
output different data types in different conditions.

Thanks,
Yilun

>
> How would converting an OF string to hex work and why would a vendor want to
> display it as hex ?
>
> For the user, instead of reading an OF string directly they would be
> required to decrypt it.
>
> So it they had a compat_id they would not use this one, they would stick it
> somewhere else.
>
> Defeating the purpose of a having it a common area.
>
> Tom
>
> >
> > Thanks,
> > Yilun
> >
> > > Tom
> > >
> > > > Thanks,
> > > > Yilun
> > > >
> > > > > Tom
> > > > >
> > > > > > Thanks
> > > > > > Yilun
> > > > > >
> > > > > > > Tom
> > > > > > >
> > > > > > >
> > > > > > > > - Moritz
> > > > > > > >