[rds-devel] [External] : Re: [PATCH rdma-next v7] RDMA: Change capability fields in ib_device_attr from int to u32

[Andy Shevchenko]

On Sat, Jun 06, 2026 at 12:07:15AM -0700, Erni Sri Satya Vennela wrote:
> The capability counter fields in struct ib_device_attr are declared
> as signed int, but these values are inherently non-negative. Drivers
> maintain their cached caps as u32 and assign them directly into these
> int fields; if a cap exceeds INT_MAX the implicit narrowing yields a
> negative value visible to the IB core.
> 
> Change the signed int capability fields to u32 to match the
> underlying nature of the data. Also update consumers across the IB
> core, ULPs, NVMe-oF target, RDS, and NFS/RDMA so the new u32 values
> are not forced back through signed int or u8 via min()/min_t() or
> narrowing local variables.

...

>  	attr->max_qp_init_rd_atom =
>  	    1 << (fls(qattr->max_qp_req_rd_atomic_resc) - 1);

FWIW, this one and below looks like reinvention of rounddown_pow_of_two().

>  	attr->max_qp_rd_atom =
> -	    min(1 << (fls(qattr->max_qp_resp_rd_atomic_resc) - 1),
> +	    min(1U << (fls(qattr->max_qp_resp_rd_atomic_resc) - 1),
>  		attr->max_qp_init_rd_atom);

...

>  int ipoib_cm_dev_init(struct net_device *dev)
>  {
>  	struct ipoib_dev_priv *priv = ipoib_priv(dev);
> -	int max_srq_sge, i;
> +	int i;
> +	u32 max_srq_sge;
>  	u8 addr;

It seems the order is reversed xmas tree, why not preserving it?

...

> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c

>  		max_send_wr =
> -			min_t(int, wr_limit, SERVICE_CON_QUEUE_DEPTH * 2 + 2);
> +			min(wr_limit, SERVICE_CON_QUEUE_DEPTH * 2 + 2);

Now perfectly a single line

		max_send_wr = min(wr_limit, SERVICE_CON_QUEUE_DEPTH * 2 + 2);

>  		max_recv_wr = max_send_wr;

...

> -		max_send_wr = min_t(int, wr_limit,
> -			      /* QD * (REQ + RSP + FR REGS or INVS) + drain */
> -			      clt_path->queue_depth * 4 + 1);
> -		max_recv_wr = min_t(int, wr_limit,
> -			      clt_path->queue_depth * 3 + 1);
> +		max_send_wr = min_t(u32, wr_limit,
> +				    /* QD * (REQ + RSP + FR REGS or INVS) + drain */
> +				    clt_path->queue_depth * 4 + 1);
> +		max_recv_wr = min_t(u32, wr_limit,
> +				    clt_path->queue_depth * 3 + 1);

Can we rather update the type of one of them and use min() instead?

...

> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c

Ditto.

...

> -static int srpt_srq_size = DEFAULT_SRPT_SRQ_SIZE;
> -module_param(srpt_srq_size, int, 0444);
> +static unsigned int srpt_srq_size = DEFAULT_SRPT_SRQ_SIZE;
> +module_param(srpt_srq_size, uint, 0444);

Theoretically this might break ABI (if somebody uses negative values for
anything. I don't think it's the case, but just be informed.

>  MODULE_PARM_DESC(srpt_srq_size,
>  		 "Shared receive queue (SRQ) size.");

...

> --- a/drivers/nvme/target/rdma.c
> +++ b/drivers/nvme/target/rdma.c

> -	ndev->srq_size = min(ndev->device->attrs.max_srq_wr,
> -			     nvmet_rdma_srq_size);
> -	ndev->srq_count = min(ndev->device->num_comp_vectors,
> -			      ndev->device->attrs.max_srq);
> +	ndev->srq_size = min_t(u32, ndev->device->attrs.max_srq_wr,
> +			       nvmet_rdma_srq_size);
> +	ndev->srq_count = min_t(u32, ndev->device->num_comp_vectors,
> +				ndev->device->attrs.max_srq);

Same question, can we change type type of variables instead?

>  	mutex_lock(&device_list_mutex);

...

>  	inline_page_count = num_pages(nport->inline_data_size);
>  	inline_sge_count = max(cm_id->device->attrs.max_sge_rd,
> -				cm_id->device->attrs.max_recv_sge) - 1;
> +				cm_id->device->attrs.max_recv_sge);
> +	inline_sge_count = inline_sge_count ? inline_sge_count - 1 : 0;

Simple conditional might be better

	if (inline_sge_count)
		inline_sge_count--;
	OR
		inline_sge_count -= 1;

...

> +++ b/include/rdma/ib_verbs.h

> -	int			max_qp;
> -	int			max_qp_wr;
> +	u32			max_qp;
> +	u32			max_qp_wr;

Nice, but please check that none of these (and beyond) were not used in signed
multiplication or (which is more disasterous) division. Otherwise it might be
subtle issues that will be hard to debug.

...

>  	conn_param->responder_resources =
> -		min_t(u32, rds_ibdev->max_responder_resources, max_responder_resources);
> +		min3(rds_ibdev->max_responder_resources,
> +		     max_responder_resources, U8_MAX);
>  	conn_param->initiator_depth =
> -		min_t(u32, rds_ibdev->max_initiator_depth, max_initiator_depth);
> +		min3(rds_ibdev->max_initiator_depth,
> +		     max_initiator_depth, U8_MAX);

I believe we can go a few characters over and leave them to be single lines.

>  	conn_param->retry_count = min_t(unsigned int, rds_ib_retry_count, 7);

What about this one?

>  	conn_param->rnr_retry_count = 7;

...

>  int frwr_query_device(struct rpcrdma_ep *ep, const struct ib_device *device)
>  {
>  	const struct ib_device_attr *attrs = &device->attrs;
> -	int max_qp_wr, depth, delta;
> +	u32 max_qp_wr;
> +	int depth, delta;
>  	unsigned int max_sge;

Reversed xmas tree order.

-- 
With Best Regards,
Andy Shevchenko