Re: [PATCH] RDMA/mlx5: hide unused code

[Patrisious Haddad]

On 3/31/2025 12:30 PM, Mark Bloch wrote:
> On 28/03/2025 16:15, Jason Gunthorpe wrote:
> > On Fri, Mar 28, 2025 at 02:10:17PM +0100, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@xxxxxxxx>
> > >
> > > After a recent rework, a few 'static const' objects have become unused:
> > >
> > > In file included from drivers/infiniband/hw/mlx5/fs.c:27:
> > > drivers/infiniband/hw/mlx5/fs.c:26:28: error: 'mlx5_ib_object_MLX5_IB_OBJECT_STEERING_ANCHOR' defined but not used [-Werror=unused-const-variable=]
> > > include/rdma/uverbs_named_ioctl.h:52:47: note: in expansion of macro 'UVERBS_OBJECT'
> > >     52 |         static const struct uverbs_object_def UVERBS_OBJECT(_object_id) = {    \
> > >        |                                               ^~~~~~~~~~~~~
> > > drivers/infiniband/hw/mlx5/fs.c:3457:1: note: in expansion of macro 'DECLARE_UVERBS_NAMED_OBJECT'
> > >   3457 | DECLARE_UVERBS_NAMED_OBJECT(
> > >        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > drivers/infiniband/hw/mlx5/fs.c:26:28: error: 'mlx5_ib_object_MLX5_IB_OBJECT_FLOW_MATCHER' defined but not used [-Werror=unused-const-variable=]
> > > include/rdma/uverbs_named_ioctl.h:52:47: note: in expansion of macro 'UVERBS_OBJECT'
> > >     52 |         static const struct uverbs_object_def UVERBS_OBJECT(_object_id) = {    \
> > >        |                                               ^~~~~~~~~~~~~
> > > drivers/infiniband/hw/mlx5/fs.c:3429:1: note: in expansion of macro 'DECLARE_UVERBS_NAMED_OBJECT'
> > >   3429 | DECLARE_UVERBS_NAMED_OBJECT(MLX5_IB_OBJECT_FLOW_MATCHER,
> > >        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > These come from a complex set of macros, and it would be possible to
> > > shut up the warnings here by adding __maybe_unused annotations inside
> > > of the macros, it seems cleaner in this case to have a large #ifdef block
> > > around all the unused parts of the file, in order to still be able to
> > > catch unused ones elsewhere.
> > IDK, I'm tempted to revert 36e0d433672f ("RDMA/mlx5: Compile fs.c
> > regardless of INFINIBAND_USER_ACCESS config")
> I believe the issue arises because uverbs_destroy_def_handler() is
> declared in ib_verbs.h, but if uverbs isn't built, there is no
> corresponding implementation of this function.
>
> One possible solution is to provide an empty implementation when USER_ACCESS
> is not set, similar to how rdma_user_mmap_disassociate() is handled.
>
> Alternatively, since uverbs_destroy_def_handler() currently does nothing
> (always returning 0), we could simply define it as a static inline
> function inside ib_verbs.h and resolve the issue that way.
>
> An example of the first approach would be:
>
> diff --git a/drivers/infiniband/hw/mlx5/fs.c b/drivers/infiniband/hw/mlx5/fs.c
> index 251246c73b33..0ff9f18a71e8 100644
> --- a/drivers/infiniband/hw/mlx5/fs.c
> +++ b/drivers/infiniband/hw/mlx5/fs.c
> @@ -3461,7 +3461,6 @@ DECLARE_UVERBS_NAMED_OBJECT(
>          &UVERBS_METHOD(MLX5_IB_METHOD_STEERING_ANCHOR_DESTROY));
>
>   const struct uapi_definition mlx5_ib_flow_defs[] = {
> -#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS)
>          UAPI_DEF_CHAIN_OBJ_TREE_NAMED(
>                  MLX5_IB_OBJECT_FLOW_MATCHER),
>          UAPI_DEF_CHAIN_OBJ_TREE(
> @@ -3472,7 +3471,6 @@ const struct uapi_definition mlx5_ib_flow_defs[] = {
>          UAPI_DEF_CHAIN_OBJ_TREE_NAMED(
>                  MLX5_IB_OBJECT_STEERING_ANCHOR,
>                  UAPI_DEF_IS_OBJ_SUPPORTED(mlx5_ib_shared_ft_allowed)),
> -#endif
>          {},
>   };
>
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index d42eae69d9a8..901353796fbb 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -4790,7 +4790,14 @@ void roce_del_all_netdev_gids(struct ib_device *ib_dev,
>
>   struct ib_ucontext *ib_uverbs_get_ucontext_file(struct ib_uverbs_file *ufile);
>
> +#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS)
>   int uverbs_destroy_def_handler(struct uverbs_attr_bundle *attrs);
> +#else
> +static inline int uverbs_destroy_def_handler(struct uverbs_attr_bundle *attrs)
> +{
> +       return 0;
> +}
> +#endif
>
>   struct net_device *rdma_alloc_netdev(struct ib_device *device, u32 port_num,
>                                       enum rdma_netdev_t type, const char *name,
>
> > I don't think that was so well thought out. The entire file was
> > designed to be USER_ACCESS only because it uses all this mechanism.

But not really the entire file , as you said below half of it is 
actually UVERBS , but it has the other half is unrelated to uverbs and 
its compilation shouldnt be dependent on it (that half is used by 
iproute2 mainly), which is the reason for this change.

(Also separation into two files, IMO isn't really the best way to 
resolve it, since in the end they are both still flow steering code 
which leaves us with mark solution unless there is objections to it I 
was planning to send it next week).

> > #ifdefing away half the file seems ugly.
agreed, which is why I think mark bloch suggestion makes more sense, do 
you think it is okay ? or you think there is issues with it ?
> > Jason