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

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

[trix]

From: Tom Rix <trix@redhat.com>

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 <trix@redhat.com>
---
 .../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@intel.com>
 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

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

[Russ Weight]

On 7/26/21 1:26 PM, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
>
> 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 <trix@redhat.com>
> ---
>  .../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@intel.com>
>  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);
>  };

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

[Tom Rix]

On 7/26/21 3:12 PM, Russ Weight wrote:
> On 7/26/21 1:26 PM, trix@redhat.com wrote:
>> From: Tom Rix <trix@redhat.com>
>>
>> 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@redhat.com>
>> ---
>>   .../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@intel.com>
>>   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);
>>   };

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

[Wu, Hao]

> On 7/26/21 3:12 PM, Russ Weight wrote:
> > On 7/26/21 1:26 PM, trix@redhat.com wrote:
> >> From: Tom Rix <trix@redhat.com>
> >>
> >> 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.

Thanks
Hao

> 
> Tom
> 
> >
> > - Russ
> >
> >> Signed-off-by: Tom Rix <trix@redhat.com>
> >> ---
> >>   .../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@intel.com>
> >>   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);
> >>   };

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

[Moritz Fischer]

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, trix@redhat.com wrote:
> > >> From: Tom Rix <trix@redhat.com>
> > >>
> > >> 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?

- Moritz

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

[Tom Rix]

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, trix@redhat.com wrote:
>>>>> From: Tom Rix <trix@redhat.com>
>>>>>
>>>>> 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/4ab7dd2d-c215-6333-6860-6f7d0ac64c3d@redhat.com/

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.


Tom


>
> - Moritz
>

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

[Xu Yilun]

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, trix@redhat.com wrote:
> > > > > > From: Tom Rix <trix@redhat.com>
> > > > > > 
> > > > > > 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/4ab7dd2d-c215-6333-6860-6f7d0ac64c3d@redhat.com/
> 
> 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.

Thanks
Yilun

> 
> 
> Tom
> 
> 
> > 
> > - Moritz
> > 

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

[Tom Rix]

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, trix@redhat.com wrote:
>>>>>>> From: Tom Rix <trix@redhat.com>
>>>>>>>
>>>>>>> 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/4ab7dd2d-c215-6333-6860-6f7d0ac64c3d@redhat.com/
>>
>> 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 ?

Tom

>
> Thanks
> Yilun
>
>>
>> Tom
>>
>>
>>> - Moritz
>>>

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

[Xu Yilun]

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, trix@redhat.com wrote:
> > > > > > > > From: Tom Rix <trix@redhat.com>
> > > > > > > > 
> > > > > > > > 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/4ab7dd2d-c215-6333-6860-6f7d0ac64c3d@redhat.com/
> > > 
> > > 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.

Thanks,
Yilun

> 
> Tom
> 
> > 
> > Thanks
> > Yilun
> > 
> > > 
> > > Tom
> > > 
> > > 
> > > > - Moritz
> > > > 

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

[Tom Rix]

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, trix@redhat.com wrote:
>>>>>>>>> From: Tom Rix <trix@redhat.com>
>>>>>>>>>
>>>>>>>>> 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/4ab7dd2d-c215-6333-6860-6f7d0ac64c3d@redhat.com/
>>>>
>>>> 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 ?

Tom

>
> Thanks,
> Yilun
>
>> Tom
>>
>>> Thanks
>>> Yilun
>>>
>>>> Tom
>>>>
>>>>
>>>>> - Moritz
>>>>>

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

[Xu Yilun]

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, trix@redhat.com wrote:
> > > > > > > > > > From: Tom Rix <trix@redhat.com>
> > > > > > > > > > 
> > > > > > > > > > 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/4ab7dd2d-c215-6333-6860-6f7d0ac64c3d@redhat.com/
> > > > > 
> > > > > 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.

Thanks,
Yilun

> 
> Tom
> 
> > 
> > Thanks,
> > Yilun
> > 
> > > Tom
> > > 
> > > > Thanks
> > > > Yilun
> > > > 
> > > > > Tom
> > > > > 
> > > > > 
> > > > > > - Moritz
> > > > > > 

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

[Tom Rix]

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, trix@redhat.com wrote:
>>>>>>>>>>> From: Tom Rix <trix@redhat.com>
>>>>>>>>>>>
>>>>>>>>>>> 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/4ab7dd2d-c215-6333-6860-6f7d0ac64c3d@redhat.com/
>>>>>>
>>>>>> 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.

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

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

[Xu Yilun]

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, trix@redhat.com wrote:
> > > > > > > > > > > > From: Tom Rix <trix@redhat.com>
> > > > > > > > > > > > 
> > > > > > > > > > > > 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/4ab7dd2d-c215-6333-6860-6f7d0ac64c3d@redhat.com/
> > > > > > > 
> > > > > > > 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
> > > > > > > > 

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

[kernel test robot]

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

Hi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.14-rc3 next-20210726]
[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-region-handle-compat_id-as-an-uuid/20210727-042850
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git ff1176468d368232b684f75e82563369208bc371
config: x86_64-randconfig-s032-20210726 (attached as .config)
compiler: gcc-10 (Ubuntu 10.3.0-1ubuntu1~20.04) 10.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/fc6e727a3f214d2b60561c3accc1f054982e2281
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review trix-redhat-com/fpga-region-handle-compat_id-as-an-uuid/20210727-042850
        git checkout fc6e727a3f214d2b60561c3accc1f054982e2281
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

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


sparse warnings: (new ones prefixed by >>)
>> drivers/fpga/dfl-fme-mgr.c:278:18: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned long long [usertype] id_l @@     got restricted __be64 [usertype] @@
   drivers/fpga/dfl-fme-mgr.c:278:18: sparse:     expected unsigned long long [usertype] id_l
   drivers/fpga/dfl-fme-mgr.c:278:18: sparse:     got restricted __be64 [usertype]
>> drivers/fpga/dfl-fme-mgr.c:279:18: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned long long [usertype] id_h @@     got restricted __be64 [usertype] @@
   drivers/fpga/dfl-fme-mgr.c:279:18: sparse:     expected unsigned long long [usertype] id_h
   drivers/fpga/dfl-fme-mgr.c:279:18: sparse:     got restricted __be64 [usertype]

vim +278 drivers/fpga/dfl-fme-mgr.c

   274	
   275	static void fme_mgr_get_compat_id(void __iomem *fme_pr,
   276					  union fpga_compat_id *id)
   277	{
 > 278		id->id_l = cpu_to_be64(readq(fme_pr + FME_PR_INTFC_ID_L));
 > 279		id->id_h = cpu_to_be64(readq(fme_pr + FME_PR_INTFC_ID_H));
   280	}
   281	

---
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: 31853 bytes --]