> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> Sent: Monday, 3 February 2025 15:47
> 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 v5 05/12] platform/mellanox: mlxreg-hotplug: Add
> support for new flavor of capability registers
>
> On Fri, 24 Jan 2025, Vadim Pasternak wrote:
>
> > Hotplug platform data is common across the various systems, while
> > hotplug driver should be able to configure only the instances relevant
> > to specific system.
> >
> > For example, platform hoptplug data might contain descriptions for
> > fan1, fan2, ..., fan{n}, while some systems equipped with all 'n'
> > fans, others with less.
> > Same for power units, power controllers, ASICs and so on.
> >
> > For detection of the real number of equipped devices capability
> > registers are used.
> > These registers used to indicate presence of hotplug devices through
> > the bitmap.
>
> Hi,
>
> Don't leave non-full lines in middle of a paragraph.
>
> > For some new big modular systems, these registers will provide
> > presence by counters.
> >
> > Use slot parameter to determine whether capability register contains
> > bitmask or counter.
> >
> > Some 'capability' registers can be shared between different resources.
> > Use fields 'capability_bit' and 'capability_mask' for getting only
> > relevant capability bits.
> >
> > Reviewed-by: Felix Radensky <fradensky@xxxxxxxxxx>
> > Signed-off-by: Vadim Pasternak <vadimp@xxxxxxxxxx>
> > ---
> > v2->v3
> > Comments pointed out by Ilpo:
> > - Change rol32() to shift left.
> > ---
> > drivers/platform/mellanox/mlxreg-hotplug.c | 22
> > ++++++++++++++++++++--
> > 1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/platform/mellanox/mlxreg-hotplug.c
> > b/drivers/platform/mellanox/mlxreg-hotplug.c
> > index 0ce9fff1f7d4..c525b8754d48 100644
> > --- a/drivers/platform/mellanox/mlxreg-hotplug.c
> > +++ b/drivers/platform/mellanox/mlxreg-hotplug.c
> > @@ -274,6 +274,12 @@ static int mlxreg_hotplug_attr_init(struct
> mlxreg_hotplug_priv_data *priv)
> > if (ret)
> > return ret;
> >
> > + if (!regval)
> > + continue;
> > +
> > + /* Remove non-relevant bits. */
> > + if (item->capability_mask)
> > + regval = (regval & item->capability_mask) <<
> > +item->capability_bit;
After dropping 'capability_bit', it'll be just masking of unused bits:
/* Remove non-relevant bits. */
if (item->capability_mask)
regval = (regval & item->capability_mask);
>
> What's in regval at this point? What it was before this patch?
>
> > item->mask = GENMASK((regval & item->mask) - 1, 0);
>
> I'm sorry but that comment didn't really help me understand what's going on
> here with the double field mask generation.
>
> Is the code correct both before the addition of the extra step and after it?
> Because I cannot wrap my head around what this code attempts to do and
> how could it be correct both pre and post this change.
>
> FYI, I've taken patches 1-3 of this series into review-ilpo-next as they seemed
> trivial changes.
>
> --
> i.
>
> > }
> >
> > @@ -294,7 +300,19 @@ static int mlxreg_hotplug_attr_init(struct
> mlxreg_hotplug_priv_data *priv)
> > if (ret)
> > return ret;
> >
> > - if (!(regval & data->bit)) {
> > + /*
> > + * In case slot field is provided, capability
> > + * register contains counter, otherwise
> bitmask.
> > + * Skip non-relevant entries if slot set and
> > + * exceeds counter. Othewise validate entry by
> > + * matching bitmask.
> > + */
> > + if (data->capability_mask)
> > + regval = (regval & item-
> >capability_mask) <<
> > + item->capability_bit;
> > + if (data->slot > regval) {
> > + break;
> > + } else if (!(regval & data->bit) && !data->slot) {
> > data++;
> > continue;
> > }
> > @@ -611,7 +629,7 @@ static int mlxreg_hotplug_set_irq(struct
> mlxreg_hotplug_priv_data *priv)
> > if (ret)
> > goto out;
> >
> > - if (!(regval & data->bit))
> > + if (!(regval & data->bit) && !data->slot)
> > item->mask &= ~BIT(j);
> > }
> > }
> >
