Re: [PATCH platform-next v2 05/10] platform/mellanox: mlxreg-hotplug: Add support for new flavor of capability registers

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

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@nvidia.com>
> Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
> ---
>  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

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