RE: [PATCH 1/2] net: RDMA: explicitly enumerate ib_device attribute groups

Parav Pandit
2025-02-26T03:43:31Z

> From: Roman Gushchin <roman.gushchin@linux.dev>
> Sent: Wednesday, February 26, 2025 9:05 AM
>
> Explicitly enumerate ib_device's attribute groups.
>
> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Leon Romanovsky <leon@kernel.org>
> Cc: Maher Sanalla <msanalla@nvidia.com>
> Cc: Parav Pandit <parav@mellanox.com>
> Cc: linux-rdma@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> drivers/infiniband/core/device.c | 4 ++--
> include/rdma/ib_verbs.h | 14 ++++++++------
> 2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 0ded91f056f3..8dea307addf1 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -1404,8 +1404,8 @@ int ib_register_device(struct ib_device *device,
> const char *name,
> return ret;
> }
>
> - device->groups[0] = &ib_dev_attr_group;
> - device->groups[1] = device->ops.device_group;
> + device->groups[IB_ATTR_GROUP_DEV_ATTR] = &ib_dev_attr_group;
> + device->groups[IB_ATTR_GROUP_DRIVER_ATTR] = device-
> >ops.device_group;
> ret = ib_setup_device_attrs(device);
This function may initialize the hw_stats at the index IB_ATTR_GROUP_DRIVER_ATTR when it is optional.
It runs through a for loop to find the first free entry.
This needs to be captured in the comment above, and it will have effect on your next fix patch.

> if (ret)
> goto cache_cleanup;
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index
> b59bf30de430..9c4c4238e6fc 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -2728,6 +2728,13 @@ struct ib_core_device {
> struct ib_device *owner; /* reach back to owner ib_device */ };
>
> +enum ib_device_attr_group {
> + IB_ATTR_GROUP_DEV_ATTR = 0, /* Device attributes */
> + IB_ATTR_GROUP_DRIVER_ATTR = 1, /* Driver-provided attributes
> */
> + IB_ATTR_GROUP_HW_STATS = 2, /* hw_stats */
> + IB_ATTR_GROUP_LAST, /* NULL pointer terminating array */
> +};
> +
> struct rdma_restrack_root;
> struct ib_device {
> /* Do not access @dma_device directly from ULP nor from HW
> drivers. */ @@ -2761,12 +2768,7 @@ struct ib_device {
> struct ib_core_device coredev;
> };
>
> - /* First group is for device attributes,
> - * Second group is for driver provided attributes (optional).
This text 'optional' is missing in above enums, please add it.
Also please add the optional text comment for hw_stats too, as its optional too.

> - * Third group is for the hw_stats
> - * It is a NULL terminated array.
> - */
> - const struct attribute_group *groups[4];
> + const struct attribute_group *groups[IB_ATTR_GROUP_LAST + 1];
>
> u64 uverbs_cmd_mask;
>
> --
> 2.48.1.658.g4767266eb4-goog