Re: [PATCH net v6 2/7] net: mana: fix HWC RQ/SQ buffer size swap

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mana: fix HWC RQ/SQ buffer size swap

The patch swaps the message sizes used to allocate the HWC RQ and SQ so the
RQ (which receives responses) is sized by max_resp_msg_size and the SQ
(which sends requests) by max_req_msg_size.  It also stores
max_resp_msg_size in struct hw_channel_context and uses it as the RX slot
stride in mana_hwc_rx_event_handler().  The commit message notes both sizes
are 0x1000 today, so the previous swap had no observable effect.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 409e20caeccdc..3f011ebbe7b3e 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

> @@ -733,14 +733,14 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
>  		goto out;
>  	}
>  
> -	err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_req_msg_size,
> +	err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_resp_msg_size,
>  				 hwc->cq, &hwc->rxq);
>  	if (err) {
>  		dev_err(hwc->dev, "Failed to create HWC RQ: %d\n", err);
>  		goto out;
>  	}
>  
> -	err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_resp_msg_size,
> +	err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_req_msg_size,
>  				 hwc->cq, &hwc->txq);
>  	if (err) {
>  		dev_err(hwc->dev, "Failed to create HWC SQ: %d\n", err);
> @@ -749,6 +749,7 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
>  
>  	hwc->num_inflight_msg = q_depth;
>  	hwc->max_req_msg_size = max_req_msg_size;
> +	hwc->max_resp_msg_size = max_resp_msg_size;

[Severity: Medium]
This isn't a bug introduced by this patch, but while looking at how these
two fields are produced and consumed, the hardware-reported sizes appear to
be dead: is the negotiated-size half of this interface still expected to do
something?

mana_hwc_establish_channel() hands the device-reported values back to its
caller:

	*q_depth = hwc->hwc_init_q_depth_max;
	*max_req_msg_size = hwc->hwc_init_max_req_msg_size;
	*max_resp_msg_size = hwc->hwc_init_max_resp_msg_size;

and mana_hwc_create_channel() forwards them:

	err = mana_hwc_test_channel(gc->hwc.driver_data,
				    HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH,
				    max_req_msg_size, max_resp_msg_size);

but the body of mana_hwc_test_channel() references neither
max_req_msg_size nor max_resp_msg_size, and nothing else applies them to
the already-created queues or to hw_channel_context.  So the fields set
here in mana_hwc_init_queues() permanently hold the bootstrap constants
HW_CHANNEL_MAX_REQUEST_SIZE / HW_CHANNEL_MAX_RESPONSE_SIZE (both 0x1000),
which matches the commit message.

Two peer consumers do read hwc->max_req_msg_size as if it were the
hardware-negotiated request bound.  mana_gd_create_dma_region() in
gdma_main.c uses it to reject oversized commands:

	if (req_msg_size > hwc->max_req_msg_size)
		return -EINVAL;

and mana_ib_gd_create_dma_region() in drivers/infiniband/hw/mana/main.c
sizes both its request buffer and its page-list capacity from it:

	max_pgs_create_cmd =
		(hwc->max_req_msg_size - sizeof(*create_req)) / sizeof(u64);
	...
	request_buf = kzalloc(hwc->max_req_msg_size, GFP_KERNEL);

If a device ever reported HWC_INIT_DATA_MAX_REQUEST smaller than 0x1000
(recorded into hwc->hwc_init_max_req_msg_size by
mana_hwc_init_event_handler()), would those callers keep building up to 4KB
commands that the hardware then rejects, and would a larger reported value
simply be ignored?  There is no memory-safety consequence today since the
SQ buffer and that bound are the same 0x1000 value after this patch, but
would it be worth either applying the reported sizes or dropping the unused
mana_hwc_test_channel() parameters?