RE: [PATCH v3 06/10] platform/mellanox: mlxreg-dpu: Add initial support for Nvidia DPU

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 7474 bytes --]

On Thu, 23 Jan 2025, Vadim Pasternak wrote:

> Hi Ilpo!
> 
> > -----Original Message-----
> > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Sent: Friday, 17 January 2025 18:59
> > To: Vadim Pasternak <vadimp@nvidia.com>
> > Cc: Hans de Goede <hdegoede@redhat.com>; Michael Shych
> > <michaelsh@nvidia.com>; Ciju Rajan K <crajank@nvidia.com>; Felix Radensky
> > <fradensky@nvidia.com>; Oleksandr Shamray <oleksandrs@nvidia.com>;
> > platform-driver-x86@vger.kernel.org
> > Subject: Re: [PATCH v3 06/10] platform/mellanox: mlxreg-dpu: Add initial
> > support for Nvidia DPU
> > 
> > On Thu, 16 Jan 2025, Vadim Pasternak wrote:
> > 
> > > Provide platform support for Nvidia (DPU) Data Processor Unit for the
> > > Smart Switch SN4280.
> > >
> > > The Smart Switch equipped with:
> > > - Nvidia COME module based on AMD EPYC™ Embedded 3451 CPU.
> > > - Nvidia Spectrum-3 ASIC.
> > > - Four DPUs, each equipped with Nvidia BF3 ARM based processor and
> > >   with Lattice LFD2NX-40 FPGA device.
> > > - 28xQSFP-DD external ports.
> > > - Two power supplies.
> > > - Four cooling drawers.
> > >
> > > Drivers provides support for the platform management and monitoring of
> > > DPU components.
> > > It includes support for health events, resets and boot progress
> > > indications logic, implemented by FPGA device.
> > >
> > > Reviewed-by: Ciju Rajan K <crajank@nvidia.com>
> > > Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
> > > ---
> > > Comments pointed out by Ilpo:
> > > - Fix s/pltaform/platform.
> > > - Remove semicolon from structure description.
> > > - In routine mlxreg_dpu_copy_hotplug_data() use 'const struct' for the
> > >   third argument.
> > > - In mlxreg_dpu_copy_hotplug_data() remove redunadant
> > devm_kmemdup()
> > >   call.
> > > - Fix identifications in mlxreg_dpu_config_init().
> > > - Remove label 'fail_register_io" from error flow.
> > > - One line for devm_regmap_init_i2c() call in mlxreg_dpu_probe().
> > > ---
> > >  drivers/platform/mellanox/Kconfig      |  12 +
> > >  drivers/platform/mellanox/Makefile     |   1 +
> > >  drivers/platform/mellanox/mlxreg-dpu.c | 615
> > > +++++++++++++++++++++++++
> > >  3 files changed, 628 insertions(+)
> > >  create mode 100644 drivers/platform/mellanox/mlxreg-dpu.c


> > > +static int
> > > +mlxreg_dpu_copy_hotplug_data(struct device *dev, struct mlxreg_dpu
> > *mlxreg_dpu,
> > > +			     const struct mlxreg_core_hotplug_platform_data
> > *hotplug_data)
> > > +{
> > > +	struct mlxreg_core_item *item;
> > > +	int i;
> > > +
> > > +	mlxreg_dpu->hotplug_data = devm_kmemdup(dev, hotplug_data,
> > > +						sizeof(*mlxreg_dpu-
> > >hotplug_data), GFP_KERNEL);
> > > +	if (!mlxreg_dpu->hotplug_data)
> > > +		return -ENOMEM;
> > > +
> > > +	item = mlxreg_dpu->hotplug_data->items;
> > > +	for (i = 0; i < mlxreg_dpu->hotplug_data->counter; i++, item++) {
> > 
> > This still has the same issue wrt. what is assigned to item. The item related
> > code on the two lines above this point do nothing because the variable's value
> > will be overwritten by this:
> > 
> > > +		item = devm_kmemdup(dev, &hotplug_data->items[i],
> > sizeof(*item),
> > > +GFP_KERNEL);
> > 
> > What is the intent here? The allocated item will be left dangling, only thing that
> > holds pointer to it after this iteration complets is the devm machinery, is that
> > the intention here? Or did you perhaps want to put it into the ->items (which
> > btw is no longer allocated at all since you didn't replace the memdup with
> > kcalloc)?
> > 
> > If you don't understand what I'm trying to tell here, please try to explain what
> > this loop does, in terms of where you want the allocated item be stored into?
> 
> Then intension was to copy 'mlxreg_dpu_default_hotplug_data' to
> 'mlxreg_dpu->hotplug_data' and then copy to this structure all
> underling fields: mlxreg_dpu_default_hotplug_data.items[i] and
> for each item 'mlxreg_dpu_default_hotplug_data.items[i.data[]'.

It is what I assumed, the patch just didn't do that correctly.

I'm bit worried about the extra pointers both the struct 
mlxreg_core_hotplug_platform_data and mlxreg_core_data have but I assume 
you know what you're doing and those are either NULL or filled with a 
value that is not expected to change.

In any case, it's feels a bit heavy handed to copy the entire struct while 
I suspect you'll be only altering part of the fields within them but I 
didn't have time to track which fields are actually being changed (beyond 
checking that some fields were indeed changed so it couldn't just become a 
const pointer).

> I can do it with the below code:
> 
> 	mlxreg_dpu->hotplug_data = devm_kmemdup(dev, hotplug_data,
> 						sizeof(*mlxreg_dpu->hotplug_data), GFP_KERNEL);
> 	if (!mlxreg_dpu->hotplug_data)
> 		return -ENOMEM;
> 
> 	mlxreg_dpu->hotplug_data->items = kcalloc(hotplug_data->counter, sizeof(*item),
> 						  GFP_KERNEL);

You should use devm_* given the other is devm too.

While sizeof(*item) is not inccorect, it should be 
sizeof(*mlxreg_dpu->hotplug_data->items) to match the destination.

Another observation that is somewhat unrelated to this patch is that 
"counter" is probably not a very good name for the number of elements in 
the ->items array. So I suggested additional patch renaming that to 
"count" or "item_count".

> 	if (!mlxreg_dpu->hotplug_data->items)
> 		return -ENOMEM;
> 
> 	item = mlxreg_dpu->hotplug_data->items;
> 	for (i = 0; i < hotplug_data->counter; i++, item++) {
> 		memcpy(item, &hotplug_data->items[i], sizeof(*item));

Yes, this makes more sense than doing item = devm_kmemdup(...) here!
(This line is where the main problem with your earlier submissions was.)

But thinking this more now without the extra kmemdup hindering my 
thoughts, I think the devm_kmemdup() here (in v2) was just extra and you 
can do it like in v2 (error handling not shown):

	mlxreg_dpu->hotplug_data = devm_kmemdup(...);
	mlxreg_dpu->hotplug_data->items = devm_kmemdup(...);

	item = mlxreg_dpu->hotplug_data->items;
	for (i = 0; i < hotplug_data->counter; i++, item++) {
		/* Nothing here for item itself, item was already duplicated above */

		item->data = devm_kmemdup(...);
	}

This partially reverses my position I gave in the comment to v2 (back then 
I wasn't yet fully following what was going on here and unsure if I 
guessed your intent right). So, you can do devm_kmemdup() also for 
->items; you just don't want to duplicate it again within this loop.

> 		item->data = kcalloc(hotplug_data->items[i].count, sizeof(*item->data),
> 				     GFP_KERNEL);

devm_kcalloc(), although I think this could use devm_kmemdup() as 
mentioned above.

> 		if (!item->data)
> 			goto kcalloc_fail;
> 
> 		data = item->data;
> 		for (j = 0; j < hotplug_data->items[i].count; j++, data++)
> 			memcpy(data, &hotplug_data->items[i].data[j], sizeof(*data));
> 	}
> 
> Does it look OK?

It's better, makes sense now although I think it can be cleaned up as 
suggested above. :-)

> > > +		if (!item)
> > > +			return -ENOMEM;
> > > +		item->data = devm_kmemdup(dev, hotplug_data-
> > >items[i].data,
> > > +					  hotplug_data->items[i].count *
> > sizeof(item->data),
> > > +					  GFP_KERNEL);
> > > +		if (!item->data)
> > > +			return -ENOMEM;
> > > +	}
> > > +
> > > +	return 0;
> > > +}


-- 
 i.