| Message ID | 20250327163855.48294-2-vadimp@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for new systems, amendments | expand |
On Thu, 27 Mar 2025, Vadim Pasternak wrote: > The hotplug platform data is common across various systems, but the > hotplug driver should only configure instances relevant to specific > systems. > > For example, platform hotplug data might contain descriptions for fan1, > fan2, ..., fan{n}, while some systems are equipped with all 'n' fans, > others with less. This applies to power controllers, ASICs, and other > components as well. > > The 'capability' register in 'mlxreg_core_item' structure specifies the > total number of elements. All registers are 8 bits wide. When the number > of attributes exceeds 8 bits, they are distributed across multiple > hotplug register sets. The 'capability' register provides the total > count across all sets. > Example for 20 attributes: > - Set 1: attributes 1-8. > - Set 2: attributes 9-16. > - Set 3: attributes 17-20. > > The content of the 'capability' register in 'mlxreg_core_data' structure > depends on presence of the 'slot' field in this structure: > - If both 'capability' and 'slot' fields are provided: register contains > count of elements. > - Otherwise: register contains bitmask. > > Use slot parameter to determine whether capability register contains > bitmask or counter. Thanks, this is much more understandable now that you updated this, however, I still find it in mismatch with the code. > This change reduces unnecessary duplication of hotplug structures > between different systems - the same structure can be used for systems > equipped with 4, 12, or 18 fans. > > Reviewed-by: Felix Radensky <fradensky@nvidia.com> > Signed-off-by: Vadim Pasternak <vadimp@nvidia.com> > --- > v6->v7 > Revised after comment pointed out by Ilpo: > - Renove capability_mask field. > - Modify comments and commit text. > 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 | 29 +++++++++++++++++++--- > 1 file changed, 26 insertions(+), 3 deletions(-) > > diff --git a/drivers/platform/mellanox/mlxreg-hotplug.c b/drivers/platform/mellanox/mlxreg-hotplug.c > index 0ce9fff1f7d4..bec12bc73e67 100644 > --- a/drivers/platform/mellanox/mlxreg-hotplug.c > +++ b/drivers/platform/mellanox/mlxreg-hotplug.c > @@ -274,7 +274,22 @@ static int mlxreg_hotplug_attr_init(struct mlxreg_hotplug_priv_data *priv) > if (ret) > return ret; > > - item->mask = GENMASK((regval & item->mask) - 1, 0); > + if (!regval) > + continue; > + > + /* > + * The 'regval' contains a bitmask or count of attributes to be handled. > + * When the 'capability' register is configured, for 'item' it specifies the > + * total number of elements. All registers are 8 bits wide. If the number of > + * attributes exceeds 8 bits, they are distributed across multiple hotplug > + * register sets. The 'capability' register provides the total count across > + * all sets. > + * Example for 20 attributes: > + * - Set 1: attributes 1-8. > + * - Set 2: attributes 9-16. > + * - Set 3: attributes 17-20. > + */ > + item->mask = GENMASK(((regval % 8) & item->mask) - 1, 0); Okay so this matches the case for Set 3 but what code handles Set 1 & 2 that should be set to 0xff ? How does it even determine which of those sets the item belongs to? I don't see those two things done anywhere in the code so please help me, is my understanding of the code lacking or is this code missing something? Note that I'm also a bit unsure if one iteration of the main for loop in this function handles one of those sets on each iteration or if there would be additional loop required to handle the multi-set case within this if block (you don't call any variable "set" in this function so I'm not able to make the connection fully between the description and the code). (I'm really sorry I feel like I'm just dragging this on and on but I cannot shake the feeling something is still wrong with this code). > } > > data = item->data; > @@ -294,7 +309,15 @@ 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 > + * 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 +634,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); > } > } >
> -----Original Message----- > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Sent: Friday, 28 March 2025 15:45 > 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 v7 1/8] platform/mellanox mlxreg-hotplug: > Add support for new flavor of capability registers > > On Thu, 27 Mar 2025, Vadim Pasternak wrote: > > > The hotplug platform data is common across various systems, but the > > hotplug driver should only configure instances relevant to specific > > systems. > > > > For example, platform hotplug data might contain descriptions for > > fan1, fan2, ..., fan{n}, while some systems are equipped with all 'n' > > fans, others with less. This applies to power controllers, ASICs, and > > other components as well. > > > > The 'capability' register in 'mlxreg_core_item' structure specifies > > the total number of elements. All registers are 8 bits wide. When the > > number of attributes exceeds 8 bits, they are distributed across > > multiple hotplug register sets. The 'capability' register provides the > > total count across all sets. > > Example for 20 attributes: > > - Set 1: attributes 1-8. > > - Set 2: attributes 9-16. > > - Set 3: attributes 17-20. > > > > The content of the 'capability' register in 'mlxreg_core_data' > > structure depends on presence of the 'slot' field in this structure: > > - If both 'capability' and 'slot' fields are provided: register contains > > count of elements. > > - Otherwise: register contains bitmask. > > > > Use slot parameter to determine whether capability register contains > > bitmask or counter. > > Thanks, this is much more understandable now that you updated this, however, I > still find it in mismatch with the code. > > > This change reduces unnecessary duplication of hotplug structures > > between different systems - the same structure can be used for systems > > equipped with 4, 12, or 18 fans. > > > > Reviewed-by: Felix Radensky <fradensky@nvidia.com> > > Signed-off-by: Vadim Pasternak <vadimp@nvidia.com> > > --- > > v6->v7 > > Revised after comment pointed out by Ilpo: > > - Renove capability_mask field. > > - Modify comments and commit text. > > 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 | 29 > > +++++++++++++++++++--- > > 1 file changed, 26 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/platform/mellanox/mlxreg-hotplug.c > > b/drivers/platform/mellanox/mlxreg-hotplug.c > > index 0ce9fff1f7d4..bec12bc73e67 100644 > > --- a/drivers/platform/mellanox/mlxreg-hotplug.c > > +++ b/drivers/platform/mellanox/mlxreg-hotplug.c > > @@ -274,7 +274,22 @@ static int mlxreg_hotplug_attr_init(struct > mlxreg_hotplug_priv_data *priv) > > if (ret) > > return ret; > > > > - item->mask = GENMASK((regval & item->mask) - 1, 0); > > + if (!regval) > > + continue; > > + > > + /* > > + * The 'regval' contains a bitmask or count of attributes > to be handled. > > + * When the 'capability' register is configured, for 'item' > it specifies the > > + * total number of elements. All registers are 8 bits > wide. If the number of > > + * attributes exceeds 8 bits, they are distributed across > multiple hotplug > > + * register sets. The 'capability' register provides the > total count across > > + * all sets. > > + * Example for 20 attributes: > > + * - Set 1: attributes 1-8. > > + * - Set 2: attributes 9-16. > > + * - Set 3: attributes 17-20. > > + */ > > + item->mask = GENMASK(((regval % 8) & item->mask) - > 1, 0); > > Okay so this matches the case for Set 3 but what code handles Set 1 & 2 that > should be set to 0xff ? > > How does it even determine which of those sets the item belongs to? > > I don't see those two things done anywhere in the code so please help me, is my > understanding of the code lacking or is this code missing something? Hi Ilpo, Yes, you are right, it still not good - need identification of the set. Actually, we don't have such hardware yet, which requires extension which I am trying to provide in this patch. The intension is to add common infrastructure for the case when system is equipped is for example with 6, 12, 20, 24 fans. For covering 20 fans, register space should have 3 register sets (status/event/mask) for each eight. The 'capability' register value will be 20. The superset configuration will be like below. static struct mlxreg_core_data mlxplat_mlxcpld_xdr_fan_items_data[] = { { .label = "fan1", .reg = MLXPLAT_CPLD_LPC_REG_FAN_OFFSET, .mask = BIT(0), .slot = 1, .capability = MLXPLAT_CPLD_LPC_REG_FAN_DRW_CAP_OFFSET, .bit = BIT(0), .hpdev.nr = MLXPLAT_CPLD_NR_NONE, }, ... { .label = "fan8", .reg = MLXPLAT_CPLD_LPC_REG_FAN_OFFSET, .mask = BIT(7), .slot = 8, .capability = MLXPLAT_CPLD_LPC_REG_FAN_DRW_CAP_OFFSET, .bit = BIT(7), .hpdev.nr = MLXPLAT_CPLD_NR_NONE, }, }; static struct mlxreg_core_data mlxplat_mlxcpld_xdr_fan2_items_data[] = { { .label = "fan9", .reg = MLXPLAT_CPLD_LPC_REG_FAN2_OFFSET, .mask = BIT(0), .slot = 9, .capability = MLXPLAT_CPLD_LPC_REG_FAN_DRW_CAP_OFFSET, .bit = BIT(0), .hpdev.nr = MLXPLAT_CPLD_NR_NONE, }, ... { .label = "fan16", .reg = MLXPLAT_CPLD_LPC_REG_FAN2_OFFSET, .mask = BIT(7), .slot = 16, .capability = MLXPLAT_CPLD_LPC_REG_FAN_DRW_CAP_OFFSET, .bit = BIT(7), .hpdev.nr = MLXPLAT_CPLD_NR_NONE, }, }; static struct mlxreg_core_data mlxplat_mlxcpld_xdr_fan3_items_data[] = { { .label = "fan17", .reg = MLXPLAT_CPLD_LPC_REG_FAN2_OFFSET, .mask = BIT(0), .slot = 17, .capability = MLXPLAT_CPLD_LPC_REG_FAN_DRW_CAP_OFFSET, .bit = BIT(0), .hpdev.nr = MLXPLAT_CPLD_NR_NONE, }, ... { .label = "fan24", .reg = MLXPLAT_CPLD_LPC_REG_FAN2_OFFSET, .mask = BIT(7), .slot = 24, .capability = MLXPLAT_CPLD_LPC_REG_FAN_DRW_CAP_OFFSET, .bit = BIT(7), .hpdev.nr = MLXPLAT_CPLD_NR_NONE, }, }; For 'item' structure need to add additional filed to identify the group. Let's say the filed 'last_slot' with the last slot number in this group. static struct mlxreg_core_item mlxplat_mlxcpld_xdr_items[] = { ... { .data = mlxplat_mlxcpld_xdr_fan_items_data, .aggr_mask = MLXPLAT_CPLD_AGGR_MASK_NG_DEF, .reg = MLXPLAT_CPLD_LPC_REG_FAN_OFFSET, .mask = MLXPLAT_CPLD_FAN_XDR_MASK, .capability = MLXPLAT_CPLD_LPC_REG_FAN_DRW_CAP_OFFSET, .last_slot = 8, -> need to add filed to identify the last slot in item .count = ARRAY_SIZE(mlxplat_mlxcpld_xdr_fan_items_data), .inversed = 1, .health = false, }, { .data = mlxplat_mlxcpld_xdr_fan2_items_data, .aggr_mask = MLXPLAT_CPLD_AGGR_MASK_NG_DEF, .reg = MLXPLAT_CPLD_LPC_REG_FAN2_OFFSET, .mask = MLXPLAT_CPLD_FAN_XDR_MASK, .capability = MLXPLAT_CPLD_LPC_REG_FAN_DRW_CAP_OFFSET, .last_slot = 16, -> need to add filed to identify the last slot in item .count = ARRAY_SIZE(mlxplat_mlxcpld_xdr_fan2_items_data), .inversed = 1, .health = false, }, { .data = mlxplat_mlxcpld_xdr_fan3_items_data, .aggr_mask = MLXPLAT_CPLD_AGGR_MASK_NG_DEF, .reg = MLXPLAT_CPLD_LPC_REG_FAN3_OFFSET, .mask = MLXPLAT_CPLD_FAN_XDR_MASK, .capability = MLXPLAT_CPLD_LPC_REG_FAN_DRW_CAP_OFFSET, .last_slot = 24, -> need to add filed to identify the last slot in item .count = ARRAY_SIZE(mlxplat_mlxcpld_xdr_fan3_items_data), .inversed = 1, .health = false, }, Then to treat it as: regval = regval >= item->last_slot ? 8 : item->last_slot - regval; item->mask = GENMASK((regval & item->mask) - 1, 0); So, for 20 elements, value of 'capability' is 20, 'item->mask' for all groups 0xff. For the 1-st group: 'regval' is set 8 and 'item->mask' 0xff. For the 2-nd group: 'regval' is set 8 and 'item->mask' 0xff. For the 3-rd group: 'regval' is set 24 - 20 and and 'item->mask' 0x0f. Probably I'll remove this patch from the current patch set and send this as a separate patch, including only the above change. What do you think? Thank you very much and sorry. Vadim. > > Note that I'm also a bit unsure if one iteration of the main for loop in this > function handles one of those sets on each iteration or if there would be > additional loop required to handle the multi-set case within this if block (you > don't call any variable "set" in this function so I'm not able to make the > connection fully between the description and the code). > > > (I'm really sorry I feel like I'm just dragging this on and on but I cannot shake the > feeling something is still wrong with this code). > > > } > > > > data = item->data; > > @@ -294,7 +309,15 @@ 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 > > + * 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 +634,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..bec12bc73e67 100644 --- a/drivers/platform/mellanox/mlxreg-hotplug.c +++ b/drivers/platform/mellanox/mlxreg-hotplug.c @@ -274,7 +274,22 @@ static int mlxreg_hotplug_attr_init(struct mlxreg_hotplug_priv_data *priv) if (ret) return ret; - item->mask = GENMASK((regval & item->mask) - 1, 0); + if (!regval) + continue; + + /* + * The 'regval' contains a bitmask or count of attributes to be handled. + * When the 'capability' register is configured, for 'item' it specifies the + * total number of elements. All registers are 8 bits wide. If the number of + * attributes exceeds 8 bits, they are distributed across multiple hotplug + * register sets. The 'capability' register provides the total count across + * all sets. + * Example for 20 attributes: + * - Set 1: attributes 1-8. + * - Set 2: attributes 9-16. + * - Set 3: attributes 17-20. + */ + item->mask = GENMASK(((regval % 8) & item->mask) - 1, 0); } data = item->data; @@ -294,7 +309,15 @@ 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 + * 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 +634,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); } }
The hotplug platform data is common across various systems, but the hotplug driver should only configure instances relevant to specific systems. For example, platform hotplug data might contain descriptions for fan1, fan2, ..., fan{n}, while some systems are equipped with all 'n' fans, others with less. This applies to power controllers, ASICs, and other components as well. The 'capability' register in 'mlxreg_core_item' structure specifies the total number of elements. All registers are 8 bits wide. When the number of attributes exceeds 8 bits, they are distributed across multiple hotplug register sets. The 'capability' register provides the total count across all sets. Example for 20 attributes: - Set 1: attributes 1-8. - Set 2: attributes 9-16. - Set 3: attributes 17-20. The content of the 'capability' register in 'mlxreg_core_data' structure depends on presence of the 'slot' field in this structure: - If both 'capability' and 'slot' fields are provided: register contains count of elements. - Otherwise: register contains bitmask. Use slot parameter to determine whether capability register contains bitmask or counter. This change reduces unnecessary duplication of hotplug structures between different systems - the same structure can be used for systems equipped with 4, 12, or 18 fans. Reviewed-by: Felix Radensky <fradensky@nvidia.com> Signed-off-by: Vadim Pasternak <vadimp@nvidia.com> --- v6->v7 Revised after comment pointed out by Ilpo: - Renove capability_mask field. - Modify comments and commit text. 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 | 29 +++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-)