| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 12 Jul 2021 08:53:54 -0700
From: Tom Rix <trix@...hat.com>
To: "Wu, Hao" <hao.wu@...el.com>, "mdf@...nel.org" <mdf@...nel.org>,
"corbet@....net" <corbet@....net>,
Russ Weight <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/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.
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.
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@...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