RE: [PATCH v5 05/12] platform/mellanox: mlxreg-hotplug: Add support for new flavor of capability registers

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

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

On Mon, 10 Feb 2025, Vadim Pasternak wrote:
> > -----Original Message-----
> > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Sent: Monday, 3 February 2025 15:47
> > 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 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@nvidia.com>
> > > Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
> > > ---
> > > 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.

This does not address all my concerns. Before this change, GENMASK() was 
calculated from regval directly. After this change, it is calculated from 
the value that is subset of bits. Why were the bits relevant before this 
change?

Your patch 1 says "Register contains bitmask of attributes or number of
attributtes, which should be handled." So if it's the first case, bitmask 
of attributes, how can this code be correct since that value is passed to 
GENMASK()??

TBH, even that pre-existing item->mask recalculation looks really odd.

> > 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.


-- 
 i.