Messages in this thread Patch in this message | | | Subject | Re: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and fpga_region | From | Tom Rix <> | Date | Thu, 22 Jul 2021 06:58:19 -0700 |
| |
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@redhat.com <trix@redhat.com> >>>>>> Sent: Friday, July 9, 2021 9:42 PM >>>>>> To: mdf@kernel.org; corbet@lwn.net; Wu, Hao <hao.wu@intel.com> >>>>>> Cc: linux-fpga@vger.kernel.org; linux-doc@vger.kernel.org; linux- >>>>>> kernel@vger.kernel.org; Tom Rix <trix@redhat.com> >>>>>> Subject: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and >>>>>> fpga_region >>>>>> >>>>>> From: Tom Rix <trix@redhat.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@redhat.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@redhat.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
| |