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

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;

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);
>  			}
>  		}
>