On Thu, 23 Jan 2025, Vadim Pasternak wrote:
> Hi Ilpo!
>
> > -----Original Message-----
> > From: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > Sent: Friday, 17 January 2025 18:59
> > To: Vadim Pasternak <vadimp@xxxxxxxxxx>
> > Cc: Hans de Goede <hdegoede@xxxxxxxxxx>; Michael Shych
> > <michaelsh@xxxxxxxxxx>; Ciju Rajan K <crajank@xxxxxxxxxx>; Felix Radensky
> > <fradensky@xxxxxxxxxx>; Oleksandr Shamray <oleksandrs@xxxxxxxxxx>;
> > platform-driver-x86@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxx>
> > > Signed-off-by: Vadim Pasternak <vadimp@xxxxxxxxxx>
> > > ---
> > > 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.
