Hi Ilpo,
Thank you for review.
> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> Sent: Monday, 13 January 2025 18:25
> 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 platform-next v2 05/10] platform/mellanox: mlxreg-
> hotplug: Add support for new flavor of capability registers
>
> Hi Vadim,
>
> I was no longer among the receipients despite being marked as M: for this
> file. Also lkml is not there despite,
>
> scripts/get_maintainer.pl -f drivers/platform/mellanox/mlxreg-hotplug.c
>
> returning both so there's still something wrong in the way the receipients are
> selected.
>
> On Mon, 13 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.
> >
> > 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>
> > ---
> > drivers/platform/mellanox/mlxreg-hotplug.c | 23
> > ++++++++++++++++++++--
> > 1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/platform/mellanox/mlxreg-hotplug.c
> > b/drivers/platform/mellanox/mlxreg-hotplug.c
> > index 0ce9fff1f7d4..3e480c322353 100644
> > --- a/drivers/platform/mellanox/mlxreg-hotplug.c
> > +++ b/drivers/platform/mellanox/mlxreg-hotplug.c
> > @@ -274,6 +274,13 @@ 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 = rol32(regval & item->capability_mask,
> > + item->capability_bit);
>
> Is the intention here to really do _rotate_ bits or is it just normal shifting going
> on? It might be the bits might never rotate past 32-bit boundary so it is
> effectively just shifting but labeling it as rotate is still wrong if bit rotate is not
> intended.
>
> I see there are also two pre-existing rol32() calls inside
> drivers/platform/mellanox/ with one of them talking about "shift" so I
> suspect they might be also wrongly using rol32() that does rotate
Yes, the intention was to shift.
I'll change to
regval = (regval & item->capability_mask) << item->capability_bit;
And later prepare fix for other places.
>
> > item->mask = GENMASK((regval & item->mask) - 1, 0);
> > }
> >
> > @@ -294,7 +301,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 = rol32(regval & data-
> >capability_mask,
> > + data->capability_bit);
>
> Another rol32() here?
>
> > + if (data->slot > regval) {
> > + break;
> > + } else if (!(regval & data->bit) && !data->slot) {
> > data++;
> > continue;
> > }
> > @@ -611,7 +630,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);
> > }
> > }
> >
>
> --
> i.
