> On Fri, Jun 18, 2021 at 01:39:42PM -0700, Tom Rix wrote:
> >
> > On 6/18/21 10:58 AM, Russ Weight wrote:
> > >
> > > On 6/18/21 9:03 AM, Russ Weight wrote:
> > > > On 6/18/21 8:45 AM, Xu Yilun wrote:
> > > > > On Wed, Jun 16, 2021 at 03:57:38PM -0700, Russ Weight wrote:
> > > > > > The FPGA manager class driver data structure is being treated as a
> > > > > > managed resource instead of using standard dev_release call-back
> > > > > > to release the class data structure. This change removes the
> > > > > > managed resource code for the freeing of the class data structure
> > > > > > and combines the create() and register() functions into a single
> > > > > > register() function.
> > > > > >
> > > > > > The devm_fpga_mgr_register() function is retained.
> > > > > >
> > > > > > Signed-off-by: Russ Weight <russell.h.weight@xxxxxxxxx>
> > > > > > ---
> > > > > > v5:
> > > > > > - Rebased on top of recently accepted patches.
> > > > > > - Removed compat_id from the fpga_mgr_register() parameter list
> > > > > > and added it to the fpga_manager_ops structure. This also required
> > > > > My reason for this change is, we could avoid further change of the
> > > > > fpga_mgr_register() API if we add new input parameters later on.
> > > > With this patchset, changes are only required for the callers
> > > > that use the new parameters.
> > > >
> > > > > > dynamically allocating the dfl-fme-ops structure in order to add
> > > > > > the appropriate compat_id.
> > > > > But enforcing the dynamical allocation of the parameters is not prefered
> > > > > to me. How about a dedicated structure that wraps all the needed
> > > > > parameters:
> > > > >
> > > > > struct fpga_mgr_info {
> > > > > const char *name;
> > > > > const struct fpga_manager_ops *mops;
> > > > > const struct fpga_compat_id *compat_id;
> > > > > const void *priv;
> > > > > };
> > > > >
> > > > > Then We can simply define a local variable of this struct for
> > > > > fpga_mgr_register().
> > > > >
> > > > > more details inline.
> > > > I agree the at the dynamic allocation is not preferred, but it is only
> > > > required if compat_id is used. In all other cases, the static structure
> > > > can continue to be used. In otherwords, only one caller is affected.
> > > > > > v4:
> > > > > > - Added the compat_id parameter to fpga_mgr_register() and
> > > > > > devm_fpga_mgr_register() to ensure that the compat_id is set
> before
> > > > > > the device_register() call.
> > > > > > v3:
> > > > > > - Cleaned up comment header for fpga_mgr_register()
> > > > > > - Fix error return on ida_simple_get() failure
> > > > > > v2:
> > > > > > - Restored devm_fpga_mgr_register() functionality, adapted for the
> combined
> > > > > > create/register functionality.
> > > > > > - All previous callers of devm_fpga_mgr_register() will continue to call
> > > > > > devm_fpga_mgr_register().
> > > > > > - replaced unnecessary ternary operators in return statements with
> standard
> > > > > > if conditions.
> > > > > > ---
> > > > > > drivers/fpga/altera-cvp.c | 12 +--
> > > > > > drivers/fpga/altera-pr-ip-core.c | 8 +-
> > > > > > drivers/fpga/altera-ps-spi.c | 10 +-
> > > > > > drivers/fpga/dfl-fme-mgr.c | 52 ++++++----
> > > > > > drivers/fpga/dfl-fme-region.c | 2 +-
> > > > > > drivers/fpga/fpga-mgr.c | 163 ++++++++-----------------------
> > > > > > drivers/fpga/ice40-spi.c | 10 +-
> > > > > > drivers/fpga/machxo2-spi.c | 10 +-
> > > > > > drivers/fpga/socfpga-a10.c | 16 ++-
> > > > > > drivers/fpga/socfpga.c | 10 +-
> > > > > > drivers/fpga/stratix10-soc.c | 16 +--
> > > > > > drivers/fpga/ts73xx-fpga.c | 10 +-
> > > > > > drivers/fpga/xilinx-spi.c | 12 +--
> > > > > > drivers/fpga/zynq-fpga.c | 16 ++-
> > > > > > drivers/fpga/zynqmp-fpga.c | 10 +-
> > > > > > include/linux/fpga/fpga-mgr.h | 43 ++++----
> > > > > > 16 files changed, 153 insertions(+), 247 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> > > > > > index ccf4546eff29..4ffb9da537d8 100644
> > > > > > --- a/drivers/fpga/altera-cvp.c
> > > > > > +++ b/drivers/fpga/altera-cvp.c
> > > > > > @@ -652,19 +652,15 @@ static int altera_cvp_probe(struct pci_dev
> *pdev,
> > > > > > snprintf(conf->mgr_name, sizeof(conf->mgr_name), "%s @%s",
> > > > > > ALTERA_CVP_MGR_NAME, pci_name(pdev));
> > > > > > - mgr = devm_fpga_mgr_create(&pdev->dev, conf->mgr_name,
> > > > > > - &altera_cvp_ops, conf);
> > > > > > - if (!mgr) {
> > > > > > - ret = -ENOMEM;
> > > > > > + mgr = fpga_mgr_register(&pdev->dev, conf->mgr_name,
> > > > > > + &altera_cvp_ops, conf);
> > > > > > + if (IS_ERR(mgr)) {
> > > > > > + ret = PTR_ERR(mgr);
> > > > > > goto err_unmap;
> > > > > > }
> > > > > > pci_set_drvdata(pdev, mgr);
> > > > > > - ret = fpga_mgr_register(mgr);
> > > > > > - if (ret)
> > > > > > - goto err_unmap;
> > > > > > -
> > > > > > return 0;
> > > > > > err_unmap:
> > > > > > diff --git a/drivers/fpga/altera-pr-ip-core.c b/drivers/fpga/altera-pr-ip-
> core.c
> > > > > > index dfdf21ed34c4..17babf974852 100644
> > > > > > --- a/drivers/fpga/altera-pr-ip-core.c
> > > > > > +++ b/drivers/fpga/altera-pr-ip-core.c
> > > > > > @@ -191,11 +191,11 @@ int alt_pr_register(struct device *dev, void
> __iomem *reg_base)
> > > > > > (val & ALT_PR_CSR_STATUS_MSK) >>
> ALT_PR_CSR_STATUS_SFT,
> > > > > > (int)(val & ALT_PR_CSR_PR_START));
> > > > > > - mgr = devm_fpga_mgr_create(dev, dev_name(dev),
> &alt_pr_ops, priv);
> > > > > > - if (!mgr)
> > > > > > - return -ENOMEM;
> > > > > > + mgr = devm_fpga_mgr_register(dev, dev_name(dev),
> &alt_pr_ops, priv);
> > > > > > + if (IS_ERR(mgr))
> > > > > > + return PTR_ERR(mgr);
> > > > > > - return devm_fpga_mgr_register(dev, mgr);
> > > > > > + return 0;
> > > > > > }
> > > > > > EXPORT_SYMBOL_GPL(alt_pr_register);
> > > > > > diff --git a/drivers/fpga/altera-ps-spi.c b/drivers/fpga/altera-ps-spi.c
> > > > > > index 23bfd4d1ad0f..d3f77b0312b2 100644
> > > > > > --- a/drivers/fpga/altera-ps-spi.c
> > > > > > +++ b/drivers/fpga/altera-ps-spi.c
> > > > > > @@ -302,12 +302,12 @@ static int altera_ps_probe(struct spi_device
> *spi)
> > > > > > snprintf(conf->mgr_name, sizeof(conf->mgr_name), "%s %s",
> > > > > > dev_driver_string(&spi->dev), dev_name(&spi->dev));
> > > > > > - mgr = devm_fpga_mgr_create(&spi->dev, conf->mgr_name,
> > > > > > - &altera_ps_ops, conf);
> > > > > > - if (!mgr)
> > > > > > - return -ENOMEM;
> > > > > > + mgr = devm_fpga_mgr_register(&spi->dev, conf->mgr_name,
> > > > > > + &altera_ps_ops, conf);
> > > > > > + if (IS_ERR(mgr))
> > > > > > + return PTR_ERR(mgr);
> > > > > > - return devm_fpga_mgr_register(&spi->dev, mgr);
> > > > > > + return 0;
> > > > > > }
> > > > > > static const struct spi_device_id altera_ps_spi_ids[] = {
> > > > > > diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
> > > > > > index d5861d13b306..1b93bc292dbe 100644
> > > > > > --- a/drivers/fpga/dfl-fme-mgr.c
> > > > > > +++ b/drivers/fpga/dfl-fme-mgr.c
> > > > > > @@ -264,14 +264,6 @@ static u64 fme_mgr_status(struct
> fpga_manager *mgr)
> > > > > > return pr_error_to_mgr_status(priv->pr_error);
> > > > > > }
> > > > > > -static const struct fpga_manager_ops fme_mgr_ops = {
> > > > > > - .write_init = fme_mgr_write_init,
> > > > > > - .write = fme_mgr_write,
> > > > > > - .write_complete = fme_mgr_write_complete,
> > > > > > - .state = fme_mgr_state,
> > > > > > - .status = fme_mgr_status,
> > > > > > -};
> > > > > > -
> > > > > > static void fme_mgr_get_compat_id(void __iomem *fme_pr,
> > > > > > struct fpga_compat_id *id)
> > > > > > {
> > > > > > @@ -279,10 +271,34 @@ static void fme_mgr_get_compat_id(void
> __iomem *fme_pr,
> > > > > > id->id_h = readq(fme_pr + FME_PR_INTFC_ID_H);
> > > > > > }
> > > > > > +static struct fpga_manager_ops *fme_mgr_get_ops(struct device
> *dev,
> > > > > > + struct fme_mgr_priv
> *priv)
> > > > > > +{
> > > > > > + struct fpga_manager_ops *ops;
> > > > > > +
> > > > > > + ops = devm_kzalloc(dev, sizeof(*ops), GFP_KERNEL);
> > > > > > + if (!ops)
> > > > > > + return NULL;
> > > > > > +
> > > > > > + ops->compat_id = devm_kzalloc(dev, sizeof(struct
> fpga_compat_id),
> > > > > > + GFP_KERNEL);
> > > > > > + if (!ops->compat_id)
> > > > > > + return NULL;
> > > > > > +
> > > > > > + fme_mgr_get_compat_id(priv->ioaddr, ops->compat_id);
> > > > > > + ops->write_init = fme_mgr_write_init;
> > > > > > + ops->write = fme_mgr_write;
> > > > > > + ops->write_complete = fme_mgr_write_complete;
> > > > > > + ops->state = fme_mgr_state;
> > > > > > + ops->status = fme_mgr_status;
> > > > > > +
> > > > > > + return ops;
> > > > > > +}
> > > What do other's think? Is it better to dynamically allocate the ops structure
> > > for users of compat_id (just one user at this time)? Or better to create an
> > > info structure on the stack for all callers? See above for an example of a
> > > dynamically allocated the ops structure.
> > >
> > > To me, using the ops structure seems more standard, and the dynamic
> allocation,
> > > while not optimal, does not require much more space or complexity than the
> static
> > > allocation. At this time it only affects one caller.
> > >
> > > Adding the info structure as a parameter to the register() functions adds a
> > > little more complexity to all callers, whether or not they use the dynamic
> > > elements of the structure.
> >
> > Looks like dfl is the only user of compat_id.
> >
> > A board specific value does not belong in a common structure, it belongs in
> > a board structure
> >
> > Move compat_id out of fpga-mgr.h and into dfl.h
> >
> > In dfl- you can do whatever you want.
>
> Agreed. You'll have to deal with DFL specific region, that directly
> accesses mgr->compat_id.
>
> But yes, this should move to DFL, nobody else is using it.
Actually compat_id is used for hardware compatible checking, we consider this
as a common need previously. E.g. PR application can get this compat_id from
a common sysfs interface and compare it with headers of bitstream image before
start PR. We know some hardware doesn't need this kind of checking as it can be
recovered by from bad condition easily but some hardware can't. This is why
compatible checking is important for them. In early submission of DFL, this
interface was private, but moved out later as a common one per suggestion
from community.
Currently compat_id is defined as per-region item, and in DFL, all regions under
the same fpga-mgr shares the same compat_id of the fpga-mgr. If we really want
to remove compat_id from fpga-region and fpga-mgr data structure, then we
may need to add new ops to fpga-mgr / region to get compat_id (dfl provides
those callbacks, and move compact_id into private data). But I am not sure if
we really have to do this, if anything just let me know.
Thanks
Hao
>
> - Moritz
