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

> 
> 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?

Thanks. You're right, and it's pre-existing — this patch only fixes which of the two (today equal) sizes maps to the RQ vs SQ and the RX slot stride.

 hwc->max_req_msg_size  /  hwc->max_resp_msg_size  are set from the bootstrap constants:  mana_hwc_create_channel()  calls  mana_hwc_init_queues()  with  HW_CHANNEL_MAX_REQUEST_SIZE  /  HW_CHANNEL_MAX_RESPONSE_SIZE  (both 0x1000). The device-reported values from  mana_hwc_establish_channel()  only reach  mana_hwc_test_channel() , which ignores them, so the consumers you found use the 0x1000 constant. With both equal there's no memory-safety consequence; a smaller reported size would get commands rejected by the device, a larger one would be capped conservatively — neither is reachable on current firmware.

I'd rather not fix this in this series: feeding the reported sizes back is a runtime behaviour change that needs its own justification and testing, and this series is scoped to the reliability fixes. It's a good candidate for a separate patch.

Long