Re: [PATCH v6 2/9] platform/mellanox mlxreg-hotplug: Add support for new flavor of capability registers

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

On Tue, 11 Feb 2025, Vadim Pasternak wrote:

Hi Vadim,

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

hotplug

> fan2, ..., fan{n}, while some systems equipped with all 'n' fans,
> others with less. Same for power units, power controllers, ASICs and so
> on.
> 
> New 'capability_mask' is introduced to allow sharing of same hoptplug
> structure between different systems, equipped with different number of
> hotplug devices. It contains superset mask for all systems sharing the
> same configuration.
> 
> The purpose is to reduce unnecessary duplication of hoptplug structures

hotplug

> between different systems - same structure is to be used for example for
> system equipped fir for 4, 6 or 8 fans.
> 
> For detection of the real number of equipped devices capability
> registers are used. These registers used to indicate presence of
> hotplug devices. On some systems presence is porvided through the

provided

> 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.
> 
> Reviewed-by: Felix Radensky <fradensky@nvidia.com>
> Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
> ---
> v5->v6
> Revised after comments pointed out by Ilpo:
> - Drop 'capability_bit' from structure 'mlxreg_core_data', since it is
>   not used.
> ---
>  drivers/platform/mellanox/mlxreg-hotplug.c | 25 ++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/mellanox/mlxreg-hotplug.c b/drivers/platform/mellanox/mlxreg-hotplug.c
> index 0ce9fff1f7d4..93bdd20fd71a 100644
> --- a/drivers/platform/mellanox/mlxreg-hotplug.c
> +++ b/drivers/platform/mellanox/mlxreg-hotplug.c
> @@ -274,6 +274,16 @@ static int mlxreg_hotplug_attr_init(struct mlxreg_hotplug_priv_data *priv)
>  			if (ret)
>  				return ret;
>  
> +			if (!regval)
> +				continue;
> +
> +			/*
> +			 * Remove non-relevant bits: 'regval' contains bitmask of attributes or
> +			 * number of attributtes, which should be handled. While 'capability mask'

attributes

> +			 * is superset mask.
> +			 */
> +			if (item->capability_mask)
> +				regval = (regval & item->capability_mask);
>  			item->mask = GENMASK((regval & item->mask) - 1, 0);

I still don't understand how this can be correct.

As a last step, the code here is taking GENMASK(<num_of_bits>-1, 0) I 
assume. That would mean regval has a field that is indicated by item->mask 
that the number of bits (or "number of attributes" as mentioned in the 
comment). Is that correct?

Now, in your comment and the commit message, you also say 'regval' 
might contain "bitmask of attributes' and you mask part of the bits away
from what I assume is a bitmask in the newly introduced code. A bitmask, 
however, is not something that seems directly compatible with GENMASK() 
that inputs bit _indexes_, so how can that be passed directly into GENMASK 
without anything to convert it into number of bits/bit index first???

-- 
 i.

>  		}
>  
> @@ -294,7 +304,18 @@ static int mlxreg_hotplug_attr_init(struct mlxreg_hotplug_priv_data *priv)
>  				if (ret)
>  					return ret;
>  
> -				if (!(regval & data->bit)) {
> +				if (data->capability_mask)
> +					regval = (regval & data->capability_mask);
> +
> +				/*
> +				 * In case slot field is provided, capability register contains
> +				 * counter, otherwise bitmask. Skip non-relevant entries if slot
> +				 * is set and exceeds counter. Othewise validate entry by matching
> +				 * bitmask.
> +				 */
> +				if (data->slot > regval)
> +					break;
> +				if (!(regval & data->bit) && !data->slot) {
>  					data++;
>  					continue;
>  				}
> @@ -611,7 +632,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);
>  			}
>  		}
>