On Tue, 2024-09-24 at 17:01 +0800, Xu Yilun wrote:
> On Thu, Sep 19, 2024 at 04:34:30PM -0400, Peter Colberg wrote:
> > From: Xu Yilun <yilun.xu@xxxxxxxxx>
> >
>
> Please describe what this patch does at the beginning. And below
> background descriptions follow.
The description has been fully revised in the new last patch "fpga:
dfl: destroy/recreate feature platform device on port release/assign".
>
> > With the Intel FPGA PAC D5005, DFL ports are registered as platform
> > devices in PF mode. The port device must be removed from the host when
> > the user wants to configure the port as a VF for use by a user-space
> > driver, e.g., for pass-through to a virtual machine. The FME device
> > ioctls DFL_FPGA_FME_PORT_RELEASE/ASSIGN are assigned for this purpose.
> >
> > In the previous implementation, the port platform device is not
> > completely destroyed on port release: it is removed from the system by
> > platform_device_del(), but the platform device instance is retained.
> > When DFL_FPGA_FME_PORT_ASSIGN is called, the platform device is added
> > back with platform_device_add(), which conflicts with this comment of
> > device_add(): "Do not call this routine more than once for any device
> > structure", and would previously cause a kernel warning at runtime.
> >
> > This patch completely unregisters the port platform device on release
> > and registers a new device on assign. But the main work is to remove
>
> The main work of this series, not this patch.
>
> > the dependency on struct dfl_feature_platform_data for many internal DFL
> > APIs. This structure holds many DFL enumeration infos for feature
> > devices. Many DFL APIs are expected to work with these infos even when
> > the port platform device is unregistered. But after this change, the
> > platform_data will be freed on port release. Hence this patch introduces
> > a new structure dfl_feature_dev_data, which acts similarly to the
> > previous dfl_feature_platform_data. dfl_feature_platform_data then only
> > needs a pointer to dfl_feature_dev_data to query DFL enumeration infos.
> >
> > Signed-off-by: Xu Yilun <yilun.xu@xxxxxxxxx>
> > Signed-off-by: Russ Weight <russell.h.weight@xxxxxxxxx>
> > Signed-off-by: Peter Colberg <peter.colberg@xxxxxxxxx>
> > Reviewed-by: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
> > ---
> > drivers/fpga/dfl-fme-br.c | 2 -
> > drivers/fpga/dfl.c | 207 ++++++++++++++++++--------------------
> > drivers/fpga/dfl.h | 57 +++++++----
> > 3 files changed, 133 insertions(+), 133 deletions(-)
> >
> > diff --git a/drivers/fpga/dfl-fme-br.c b/drivers/fpga/dfl-fme-br.c
> > index 5c60a38ec76c..a298a041877b 100644
> > --- a/drivers/fpga/dfl-fme-br.c
> > +++ b/drivers/fpga/dfl-fme-br.c
> > @@ -85,8 +85,6 @@ static void fme_br_remove(struct platform_device *pdev)
> >
> > fpga_bridge_unregister(br);
> >
> > - if (priv->port_fdata)
> > - put_device(&priv->port_fdata->dev->dev);
>
> I can't remember why all the get_device/put_device() are not needed anymore.
This is now detailed in the new patch "fpga: dfl: destroy/recreate
feature platform device on port release/assign".
>
> > if (priv->port_ops)
> > dfl_fpga_port_ops_put(priv->port_ops);
> > }
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index 52f58d029ca4..a77d7692b170 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -160,7 +160,7 @@ struct dfl_fpga_port_ops *dfl_fpga_port_ops_get(struct dfl_feature_dev_data *fda
> >
> > list_for_each_entry(ops, &dfl_port_ops_list, node) {
> > /* match port_ops using the name of platform device */
> > - if (!strcmp(fdata->dev->name, ops->name)) {
> > + if (!strcmp(fdata->pdev_name, ops->name)) {
> > if (!try_module_get(ops->owner))
> > ops = NULL;
> > goto done;
> > @@ -681,7 +681,6 @@ EXPORT_SYMBOL_GPL(dfl_fpga_dev_ops_unregister);
> > * @nr_irqs: number of irqs for all feature devices.
> > * @irq_table: Linux IRQ numbers for all irqs, indexed by local irq index of
> > * this device.
> > - * @feature_dev: current feature device.
> > * @type: the current FIU type.
> > * @ioaddr: header register region address of current FIU in enumeration.
> > * @start: register resource start of current FIU.
> > @@ -695,7 +694,6 @@ struct build_feature_devs_info {
> > unsigned int nr_irqs;
> > int *irq_table;
> >
> > - struct platform_device *feature_dev;
> > enum dfl_id_type type;
> > void __iomem *ioaddr;
> > resource_size_t start;
> > @@ -736,7 +734,6 @@ static void dfl_fpga_cdev_add_port_data(struct dfl_fpga_cdev *cdev,
> > {
> > mutex_lock(&cdev->lock);
> > list_add(&fdata->node, &cdev->port_dev_list);
> > - get_device(&fdata->dev->dev);
> > mutex_unlock(&cdev->lock);
> > }
> >
> > @@ -744,7 +741,6 @@ static struct dfl_feature_dev_data *
> > binfo_create_feature_dev_data(struct build_feature_devs_info *binfo)
> > {
> > enum dfl_id_type type = binfo->type;
> > - struct platform_device *fdev = binfo->feature_dev;
> > struct dfl_feature_info *finfo, *p;
> > struct dfl_feature_dev_data *fdata;
> > int ret, index = 0, res_idx = 0;
> > @@ -752,18 +748,27 @@ binfo_create_feature_dev_data(struct build_feature_devs_info *binfo)
> > if (WARN_ON_ONCE(type >= DFL_ID_MAX))
> > return ERR_PTR(-EINVAL);
> >
> > - /*
> > - * we do not need to care for the memory which is associated with
> > - * the platform device. After calling platform_device_unregister(),
> > - * it will be automatically freed by device's release() callback,
> > - * platform_device_release().
> > - */
> > - fdata = kzalloc(struct_size(fdata, features, binfo->feature_num), GFP_KERNEL);
> > + fdata = devm_kzalloc(binfo->dev, sizeof(*fdata), GFP_KERNEL);
> > if (!fdata)
> > return ERR_PTR(-ENOMEM);
> >
> > - fdata->dev = fdev;
> > + fdata->features = devm_kcalloc(binfo->dev, binfo->feature_num,
> > + sizeof(*fdata->features), GFP_KERNEL);
> > + if (!fdata->features)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + fdata->resources = devm_kcalloc(binfo->dev, binfo->feature_num,
> > + sizeof(*fdata->resources), GFP_KERNEL);
> > + if (!fdata->resources)
> > + return ERR_PTR(-ENOMEM);
> > +
> > fdata->type = type;
> > +
> > + fdata->pdev_id = dfl_id_alloc(type, binfo->dev);
> > + if (fdata->pdev_id < 0)
> > + return ERR_PTR(fdata->pdev_id);
> > +
> > + fdata->pdev_name = dfl_devs[type].name;
> > fdata->num = binfo->feature_num;
> > fdata->dfl_cdev = binfo->cdev;
> > fdata->id = FEATURE_DEV_ID_UNUSED;
> > @@ -779,15 +784,6 @@ binfo_create_feature_dev_data(struct build_feature_devs_info *binfo)
> > */
> > WARN_ON(fdata->disable_count);
> >
> > - fdev->dev.platform_data = fdata;
> > -
> > - /* each sub feature has one MMIO resource */
> > - fdev->num_resources = binfo->feature_num;
> > - fdev->resource = kcalloc(binfo->feature_num, sizeof(*fdev->resource),
> > - GFP_KERNEL);
> > - if (!fdev->resource)
> > - return ERR_PTR(-ENOMEM);
> > -
> > /* fill features and resource information for feature dev */
> > list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
> > struct dfl_feature *feature = &fdata->features[index++];
> > @@ -795,7 +791,6 @@ binfo_create_feature_dev_data(struct build_feature_devs_info *binfo)
> > unsigned int i;
> >
> > /* save resource information for each feature */
> > - feature->dev = fdev;
> > feature->id = finfo->fid;
> > feature->revision = finfo->revision;
> > feature->dfh_version = finfo->dfh_version;
> > @@ -804,8 +799,10 @@ binfo_create_feature_dev_data(struct build_feature_devs_info *binfo)
> > feature->params = devm_kmemdup(binfo->dev,
> > finfo->params, finfo->param_size,
> > GFP_KERNEL);
> > - if (!feature->params)
> > - return ERR_PTR(-ENOMEM);
> > + if (!feature->params) {
> > + ret = -ENOMEM;
> > + goto err_free_id;
> > + }
> >
> > feature->param_size = finfo->param_size;
> > }
> > @@ -823,19 +820,20 @@ binfo_create_feature_dev_data(struct build_feature_devs_info *binfo)
> > &finfo->mmio_res);
> > if (IS_ERR(feature->ioaddr)) {
> > ret = PTR_ERR(feature->ioaddr);
> > - return ERR_PTR(ret);
> > + goto err_free_id;
> > }
> > } else {
> > feature->resource_index = res_idx;
> > - fdev->resource[res_idx++] = finfo->mmio_res;
> > + fdata->resources[res_idx++] = finfo->mmio_res;
> > }
> >
> > if (finfo->nr_irqs) {
> > ctx = devm_kcalloc(binfo->dev, finfo->nr_irqs,
> > sizeof(*ctx), GFP_KERNEL);
> > - if (!ctx)
> > - return ERR_PTR(-ENOMEM);
> > -
> > + if (!ctx) {
> > + ret = -ENOMEM;
> > + goto err_free_id;
> > + }
> > for (i = 0; i < finfo->nr_irqs; i++)
> > ctx[i].irq =
> > binfo->irq_table[finfo->irq_base + i];
> > @@ -848,36 +846,67 @@ binfo_create_feature_dev_data(struct build_feature_devs_info *binfo)
> > kfree(finfo);
> > }
> >
> > + fdata->resource_num = res_idx;
> > +
> > return fdata;
> > +
> > +err_free_id:
> > + dfl_id_free(type, fdata->pdev_id);
> > +
> > + return ERR_PTR(ret);
> > }
> >
> > -static int
> > -build_info_create_dev(struct build_feature_devs_info *binfo)
> > +/*
> > + * register current feature device, it is called when we need to switch to
> > + * another feature parsing or we have parsed all features on given device
> > + * feature list.
> > + */
> > +static int feature_dev_register(struct dfl_feature_dev_data *fdata)
> > {
> > - enum dfl_id_type type = binfo->type;
> > + struct dfl_feature_platform_data pdata = {};
> > struct platform_device *fdev;
> > + struct dfl_feature *feature;
> > + int ret;
> >
> > - /*
> > - * we use -ENODEV as the initialization indicator which indicates
> > - * whether the id need to be reclaimed
> > - */
> > - fdev = platform_device_alloc(dfl_devs[type].name, -ENODEV);
> > + fdev = platform_device_alloc(fdata->pdev_name, fdata->pdev_id);
>
> I see a part of the change is to delay the platform_device_alloc() so
> that the feature platform device registration work could be gathered in
> one function. Not sure if it is necessary for the port platform release
> issue, or could be an independent enhancement patch.
This has been factored out into a separate patch "fpga: dfl: allocate
platform device after feature device data". Delaying the call to
platform_device_alloc() to feature_dev_register() is essential to the
overall series since the last patch reuses feature_dev_register() for
the port reassign. If platform_device_alloc() were not part of
feature_dev_register(), the call to platform_device_alloc() would have
to be repeated in dfl_fpga_cdev_assign_port().
>
> I raise this cause I still feel hard to understand all these changes
> in this patch and look for any chance to further split it.
The smaller patches of the v4 series are hopefully much more straight-
forward to follow now. The refactoring uncovered a leak of the feature
device id for released ports that has been fixed in the new patch
"fpga: dfl: store platform device id in feature device data".
>
> BTW: Overall this series is much improved than the last version.
>
> Thanks,
> Yilun
Thank you for your quick review and guidance for this series.
Thanks,
Peter
