Date:   Thu, 22 Jul 2021 06:58:19 -0700
From:   Tom Rix <trix@...hat.com>
To:     "Wu, Hao" <hao.wu@...el.com>, "mdf@...nel.org" <mdf@...nel.org>,
        "corbet@....net" <corbet@....net>,
        "Weight, Russell H" <russell.h.weight@...el.com>
Cc:     "linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and
 fpga_region


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

Looking for a compromise that leaves the data in fpga_manager,

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

can we change that to a neutral type like uuid_t ?

It is treated as a uuid_t in opae, with.

being read byte string with this logic

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

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

         buf[i + 2] = tmp;
     }

Into this final type

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

Tom


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

Powered by blists - more mailing lists