Re: [PATCH net-next v12 05/12] enic: add admin CQ service with MSI-X interrupt and workqueue polling

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [5]: [PATCH net-next v12 05/12] enic: add admin CQ service with MSI-X interrupt and workqueue polling ---
commit 91aacafcb0c0e3c10237617f2d69726711e41d5c
Author: Satish Kharat <satishkh@xxxxxxxxx>

enic: add admin CQ service with MSI-X interrupt and workqueue polling

This commit adds a completion queue service for the admin channel work
queue and receive queue. It sets up an MSI-X interrupt that schedules
a workqueue poller to drain completions and dispatch received messages.

> diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
> index b2be420921064..9358165263d98 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_admin.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_admin.c
[ ... ]
> +unsigned int enic_admin_rq_cq_service(struct enic *enic)
> +{
[ ... ]
> +		enic_admin_msg_enqueue(enic, buf->os_buf, bytes_written);
> +
> +next_desc:
> +		enic_admin_rq_buf_clean(rq, rq->to_clean);
> +		rq->to_clean = rq->to_clean->next;
> +		rq->ring.desc_avail++;
> +
> +		vnic_cq_inc_to_clean(cq);
> +		work++;
> +		desc = vnic_cq_to_clean(cq);
> +	}
> +
> +	if (enic_admin_rq_fill(enic, GFP_KERNEL) && net_ratelimit())
> +		netdev_warn(enic->netdev,
> +			    "admin RQ refill failed\n");

[Severity: High]
What happens if enic_admin_rq_fill() fails to allocate memory here?

If it fails under memory pressure and returns without posting new buffers
to the hardware, does the admin channel permanently stall? It looks like
there is no retry timer or fallback mechanism to refill the queue later.
Without available receive buffers, the hardware won't be able to post further
receive completions, meaning the MSI-X interrupt won't fire again to recover
the channel.

> +
> +	return work;
> +}
[ ... ]
> @@ -220,6 +500,19 @@ int enic_admin_channel_open(struct enic *enic)
>  		return err;
>  	}
>  
> +	spin_lock_init(&enic->admin_msg_lock);
> +	INIT_LIST_HEAD(&enic->admin_msg_list);
> +	INIT_WORK(&enic->admin_msg_work, enic_admin_msg_work_handler);
> +	INIT_WORK(&enic->admin_poll_work, enic_admin_poll_work_handler);
> +
> +	err = enic_admin_setup_intr(enic);
> +	if (err) {
> +		netdev_err(enic->netdev,
> +			   "Admin channel requires MSI-X, SR-IOV unavailable: %d\n",
> +			   err);
> +		goto free_resources;
> +	}
> +
> +	enic_admin_init_resources(enic);

[Severity: High]
Since enic_admin_setup_intr() calls request_irq(), is it possible for an
interrupt to fire before enic_admin_init_resources() initializes the
hardware queues?

If a spurious interrupt arrives immediately after request_irq(), it would
schedule admin_poll_work which then calls enic_admin_rq_cq_service().
Because the ring pointers (such as rq->to_use) haven't been initialized by
enic_admin_init_resources() yet, could this lead to a NULL pointer
dereference or lockless data corruption?

>  
>  	vnic_wq_enable(&enic->admin_wq);
> @@ -239,17 +532,31 @@ int enic_admin_channel_open(struct enic *enic)