| Message ID | 20250113084337.24763-6-vadimp@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/mellanox: Add support for new systems, amendments, relocate mlx-platform module | expand |
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); > } > } >
Hi Ilpo, Thank you for review. > -----Original Message----- > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Sent: Monday, 13 January 2025 18:25 > 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 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@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 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.
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); 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); + 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); } }
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(-)