Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] fpga: region: handle compat_id as an uuid
To:     Russ Weight <russell.h.weight@intel.com>, hao.wu@intel.com,
        mdf@kernel.org
Cc:     linux-kernel@vger.kernel.org, linux-fpga@vger.kernel.org
References: <20210726202650.4074614-1-trix@redhat.com>
 <6f30a4c6-61a0-bb57-9f13-bcad3f3589b8@intel.com>
From:   Tom Rix <trix@redhat.com>
Message-ID: <ba28bac6-9c6d-de73-523f-b8ba4bef84de@redhat.com>
Date:   Mon, 26 Jul 2021 17:16:45 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <6f30a4c6-61a0-bb57-9f13-bcad3f3589b8@intel.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk


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);
>>   };