Re: [PATCH v22 net-next 11/12] net/nebula-matrix: add common/ctrl dev init/remove operation

[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 [11]: [PATCH v22 net-next 11/12] net/nebula-matrix: add common/ctrl dev init/remove operation ---
commit 16bce0d3af6a82a0084a93157844e7c5875d3c59
Author: illusion wang <illusion.wang@xxxxxxxxxxxxxxxxx>

net/nebula-matrix: add common/ctrl dev init/remove operation

This commit adds initialization and teardown helpers for common and
control devices. It hooks these helpers into the main init/remove
paths and configures mailbox message processing via a work_struct.

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c
> new file mode 100644
> index 0000000000000..eae6a00e6fccb
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c

[ ... ]

> +static int nbl_dev_setup_common_dev(struct nbl_adapter *adapter)
> +{

[ ... ]

> +	INIT_WORK(&common_dev->clean_mbx_task, nbl_dev_clean_mailbox_task);
> +	nbl_dev_register_chan_task(dev_mgt, NBL_CHAN_TYPE_MAILBOX,
> +				   &common_dev->clean_mbx_task);
> +	/*
> +	 * Even if has_ctrl=false (no dedicated control PF channel), we fetch
> +	 * VSI/ETH info via regular mailbox message  instead of
> +	 * dedicated control command.
> +	 */
> +	ret = disp_ops->get_vsi_id(priv, NBL_VSI_DATA, &common->vsi_id);

[Severity: High]
Will this mailbox message fail silently and read uninitialized zeros?

Since get_vsi_id() is called here before interrupts are enabled,
NBL_CHAN_INTERRUPT_READY is false. When nbl_chan_send_msg() processes
this message, it checks the interrupt ready flag:

if (test_bit(NBL_CHAN_INTERRUPT_READY, chan_info->state)) {
        spin_lock_irq(&wait_head->status_lock);
        while (!READ_ONCE(wait_head->acked)) {
                ...
        }
}
...
inflight_dec_out:
        atomic_dec(&chan_info->inflight_tx_cnt);
        return ret;

Because the flag is false, does it skip waiting for the ACK and immediately
return success (0)?

If so, this would leave the caller reading zeroed variables for the
VSI and ETH IDs, resulting in incorrect initialization and potentially leaving
a dangling pointer in the wait queue.

> +	if (ret)
> +		goto err_cleanup;
> +	ret = disp_ops->get_eth_id(priv, common->vsi_id, &common->eth_num,
> +			     &common->eth_id, &common->logic_eth_id);
> +	if (ret)
> +		goto err_cleanup;

[ ... ]