RE: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and fpga_region

[Wu, Hao]

> 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.

> >
> >> 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