Re: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and fpga_region
From: Tom Rix
Date: Thu Jul 22 2021 - 09:58:26 EST
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@xxxxxxxxxx <trix@xxxxxxxxxx>
Sent: Friday, July 9, 2021 9:42 PM
To: mdf@xxxxxxxxxx; corbet@xxxxxxx; Wu, Hao <hao.wu@xxxxxxxxx>
Cc: linux-fpga@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx; linux-
kernel@xxxxxxxxxxxxxxx; Tom Rix <trix@xxxxxxxxxx>
Subject: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and
fpga_region
From: Tom Rix <trix@xxxxxxxxxx>
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@xxxxxxxxxx/
Tom
Thanks
Hao
Printing out the compat_id is done with the fpga_region
compat_id_show() op.
Signed-off-by: Tom Rix <trix@xxxxxxxxxx>
---
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