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

[Mark Bloch]

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.
> 
> #ifdefing away half the file seems ugly.
> 
> Jason