linux-fpga.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Wu, Hao" <hao.wu@intel.com>
To: Tom Rix <trix@redhat.com>, "mdf@kernel.org" <mdf@kernel.org>,
	"corbet@lwn.net" <corbet@lwn.net>,
	"Weight, Russell H" <russell.h.weight@intel.com>
Cc: "linux-fpga@vger.kernel.org" <linux-fpga@vger.kernel.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and fpga_region
Date: Thu, 22 Jul 2021 00:18:38 +0000	[thread overview]
Message-ID: <DM6PR11MB38194D113950FA4B75C7F2C485E49@DM6PR11MB3819.namprd11.prod.outlook.com> (raw)
In-Reply-To: <2056f88d-7215-8f6a-d3c3-2692741c2c2d@redhat.com>

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

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


  reply	other threads:[~2021-07-22  0:18 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-09 13:42 [PATCH v2 0/4] fpga: fpga-mgr: move compat_id from fpga_mgr to dfl trix
2021-07-09 13:42 ` [PATCH v2 1/4] fpga: region: introduce fpga_region_ops trix
2021-07-09 13:42 ` [PATCH v2 2/4] fpga: region: introduce compat_id_show op trix
2021-07-09 13:42 ` [PATCH v2 3/4] fpga: dfl: implement the compat_id_show region op trix
2021-07-09 21:58   ` kernel test robot
2021-07-12  1:27   ` Wu, Hao
2021-07-09 13:42 ` [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and fpga_region trix
2021-07-12  1:40   ` Wu, Hao
2021-07-12 15:53     ` Tom Rix
2021-07-21  4:48       ` Wu, Hao
2021-07-21 14:56         ` Tom Rix
2021-07-22  0:18           ` Wu, Hao [this message]
2021-07-22 13:58             ` Tom Rix
2021-07-20 19:47 ` [PATCH v2 0/4] fpga: fpga-mgr: move compat_id from fpga_mgr to dfl Tom Rix

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DM6PR11MB38194D113950FA4B75C7F2C485E49@DM6PR11MB3819.namprd11.prod.outlook.com \
    --to=hao.wu@intel.com \
    --cc=corbet@lwn.net \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-fpga@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mdf@kernel.org \
    --cc=russell.h.weight@intel.com \
    --cc=trix@redhat.com \
    --subject='RE: [PATCH v2 4/4] fpga: remove compat_id from fpga_manager and fpga_region' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).