Re: [PATCH v13 net-next 11/11] net/nebula-matrix: add common dev start/stop 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.
---
net/nebula-matrix: add common dev start/stop operation

This commit adds start/stop operations for the common device: it
configures the MSI-X map table, allocates and enables MSI-X vectors,
registers the mailbox ISR and enables the mailbox IRQ. It also switches
the module from module_pci_driver() to explicit init/exit that creates
and destroys a workqueue around pci_register_driver().

> 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
> index f10bb9460774..e814ffbb978d 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c

[ ... ]

> +static int nbl_dev_request_mailbox_irq(struct nbl_dev_mgt *dev_mgt)
> +{
> +	struct nbl_dev_common *dev_common = dev_mgt->common_dev;
> +	struct nbl_msix_info *msix_info = &dev_common->msix_info;
> +	struct nbl_common_info *common = dev_mgt->common;
> +	u16 local_vec_id;
> +	u32 irq_num;
> +	int err;
> +
> +	if (!msix_info->serv_info[NBL_MSIX_MAILBOX_TYPE].num)
> +		return 0;
> +
> +	local_vec_id =
> +		msix_info->serv_info[NBL_MSIX_MAILBOX_TYPE].base_vector_id;
> +	irq_num = pci_irq_vector(common->pdev, local_vec_id);

Can the return value of pci_irq_vector() be handled here?  It is
declared as:

    int pci_irq_vector(struct pci_dev *dev, unsigned int nr);

and can return a negative errno (for example -EINVAL, including the
CONFIG_PCI=n stub).  The value is stored into a u32 irq_num and passed
directly to request_irq() without a range check, so a negative return
would be silently converted into a very large positive number.

Would declaring irq_num as int and testing for irq_num < 0 before
calling request_irq() be more appropriate?

> +
> +	snprintf(dev_common->mailbox_name, sizeof(dev_common->mailbox_name),
> +		 "nbl_mailbox@pci:%s", pci_name(common->pdev));
> +	err = request_irq(irq_num, nbl_dev_clean_mailbox, 0,
> +			  dev_common->mailbox_name, dev_mgt);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static void nbl_dev_free_mailbox_irq(struct nbl_dev_mgt *dev_mgt)
> +{
> +	struct nbl_dev_common *dev_common = dev_mgt->common_dev;
> +	struct nbl_msix_info *msix_info = &dev_common->msix_info;
> +	struct nbl_common_info *common = dev_mgt->common;
> +	u16 local_vec_id;
> +	u32 irq_num;
> +
> +	if (!msix_info->serv_info[NBL_MSIX_MAILBOX_TYPE].num)
> +		return;
> +
> +	local_vec_id =
> +		msix_info->serv_info[NBL_MSIX_MAILBOX_TYPE].base_vector_id;
> +	irq_num = pci_irq_vector(common->pdev, local_vec_id);

Same question as above: pci_irq_vector() can return a negative errno,
and irq_num is a u32.  Should this be validated before calling
free_irq()?

> +
> +	free_irq(irq_num, dev_mgt);
> +}

[ ... ]

> +static int nbl_dev_disable_mailbox_irq(struct nbl_dev_mgt *dev_mgt)
> +{
> +	struct nbl_dispatch_ops *disp_ops = dev_mgt->disp_ops_tbl->ops;
> +	struct nbl_channel_ops *chan_ops = dev_mgt->chan_ops_tbl->ops;
> +	struct nbl_dev_common *dev_common = dev_mgt->common_dev;
> +	struct nbl_msix_info *msix_info = &dev_common->msix_info;
> +	u16 local_vec_id;
> +
> +	if (!msix_info->serv_info[NBL_MSIX_MAILBOX_TYPE].num)
> +		return 0;
> +
> +	flush_work(&dev_common->clean_mbx_task);
> +	local_vec_id =
> +		msix_info->serv_info[NBL_MSIX_MAILBOX_TYPE].base_vector_id;
> +	chan_ops->set_queue_state(dev_mgt->chan_ops_tbl->priv,
> +				  NBL_CHAN_INTERRUPT_READY,
> +				  NBL_CHAN_TYPE_MAILBOX, false);
> +
> +	return disp_ops->enable_mailbox_irq(dev_mgt->disp_ops_tbl->priv,
> +					    local_vec_id, false);
> +}

Is the teardown sequence here in the right order?

flush_work() runs first, but the hardware interrupt source is still
live (set_queue_state(..., false) and enable_mailbox_irq(..., false)
happen after the flush), and free_irq() is not called until the
separate nbl_dev_free_mailbox_irq() step in nbl_dev_stop() (and in the
enable_mailbox_irq_err unwind of nbl_dev_start()).

Between flush_work() returning and the hardware disable step, the
mailbox MSI-X can still fire:

    static irqreturn_t nbl_dev_clean_mailbox(int __always_unused irq, void *data)
    {
            struct nbl_dev_mgt *dev_mgt = (struct nbl_dev_mgt *)data;

            nbl_dev_clean_mailbox_schedule(dev_mgt);
            return IRQ_HANDLED;
    }

    static int nbl_dev_clean_mailbox_schedule(struct nbl_dev_mgt *dev_mgt)
    {
            struct nbl_dev_common *common_dev = dev_mgt->common_dev;

            nbl_common_queue_work(&common_dev->clean_mbx_task);
            return 0;
    }

so the work can be re-queued after the flush.

Would the usual order be: first disable the hardware interrupt source,
then free_irq()/synchronize_irq() to guarantee the handler has
finished, then flush_work() / cancel_work_sync() to drain any
already-queued instance?

The worker gates its actual work on NBL_CHAN_INTERRUPT_READY, and
nbl_dev_remove_common_dev() calls cancel_work_sync() later, which
softens the impact, but the ordering still looks inverted versus the
common pattern.

> +
> +static int nbl_dev_configure_msix_map(struct nbl_dev_mgt *dev_mgt)
> +{
> +	struct nbl_dispatch_ops *disp_ops = dev_mgt->disp_ops_tbl->ops;
> +	struct nbl_dev_common *dev_common = dev_mgt->common_dev;
> +	struct nbl_msix_info *msix_info = &dev_common->msix_info;
> +	bool mask_en = msix_info->serv_info[NBL_MSIX_NET_TYPE].hw_self_mask_en;
> +	u16 msix_net_num = msix_info->serv_info[NBL_MSIX_NET_TYPE].num;
> +	u16 msix_not_net_num = 0;
> +	int err, i;
> +
> +	for (i = NBL_MSIX_NET_TYPE; i < NBL_MSIX_TYPE_MAX; i++)
> +		msix_info->serv_info[i].base_vector_id =
> +			msix_info->serv_info[i - 1].base_vector_id +
> +			msix_info->serv_info[i - 1].num;

Is this loop safe against being re-entered?

It recomputes serv_info[i].base_vector_id from serv_info[i - 1], but
serv_info[0] (NBL_MSIX_VIRTIO_TYPE) is never explicitly reset to 0.
On a first call from fresh zero-initialised state that is fine; on a
subsequent call (for example if nbl_dev_start() is ever re-invoked
without reinitialising common_dev->msix_info) the previous
base_vector_id values would accumulate instead of being recomputed
from zero.

Today nbl_dev_start() is only called once from nbl_probe() and
nbl_dev_stop() does not clear these fields, so this is latent, but
would it be worth setting serv_info[0].base_vector_id = 0 before the
loop?

[ ... ]

> @@ -218,9 +401,41 @@ void nbl_dev_remove(struct nbl_adapter *adapter)
>  /* ----------  Dev start process  ---------- */
>  int nbl_dev_start(struct nbl_adapter *adapter)
>  {
> +	struct nbl_dev_mgt *dev_mgt = adapter->core.dev_mgt;
> +	int ret;
> +
> +	ret = nbl_dev_configure_msix_map(dev_mgt);
> +	if (ret)
> +		goto config_msix_map_err;
> +
> +	ret = nbl_dev_init_interrupt_scheme(dev_mgt);
> +	if (ret)
> +		goto init_interrupt_scheme_err;
> +	ret = nbl_dev_request_mailbox_irq(dev_mgt);
> +	if (ret)
> +		goto mailbox_request_irq_err;
> +	ret = nbl_dev_enable_mailbox_irq(dev_mgt);
> +	if (ret)
> +		goto enable_mailbox_irq_err;
> +
>  	return 0;
> +enable_mailbox_irq_err:
> +	nbl_dev_disable_mailbox_irq(dev_mgt);
> +	nbl_dev_free_mailbox_irq(dev_mgt);
> +mailbox_request_irq_err:
> +	nbl_dev_clear_interrupt_scheme(dev_mgt);
> +init_interrupt_scheme_err:
> +	nbl_dev_destroy_msix_map(dev_mgt);
> +config_msix_map_err:
> +	return ret;
>  }

Is the error unwind here symmetric with nbl_dev_stop()?

If nbl_dev_configure_msix_map() fails, control jumps to
config_msix_map_err and returns immediately, without calling
nbl_dev_destroy_msix_map().  The success teardown path in
nbl_dev_stop() always pairs configure/destroy.  Depending on what
partial state the disp_ops->configure_msix_map() implementation can
leave behind on failure, should the error path mirror the stop path,
or is there a documented contract that configure_msix_map() fully
unwinds its own partial state on error?

The same ordering question applies to the enable_mailbox_irq_err
label, which calls nbl_dev_disable_mailbox_irq() (and therefore
flush_work()) before nbl_dev_free_mailbox_irq() — mirroring the
nbl_dev_stop() sequence discussed above.

[ ... ]