Re: [RFC PATCH net-next 04/10] net: hibmcge: Add interrupt supported in this module

[Joe Damato]

On Wed, Jul 31, 2024 at 05:42:39PM +0800, Jijie Shao wrote:
> The driver supports four interrupts: TX interrupt, RX interrupt,
> mdio interrupt, and error interrupt.
> 
> Actually, the driver does not use the mdio interrupt.
> Therefore, the driver does not request the mdio interrupt.

I might be reading this wrong, but the commit message seems a bit
confusing? If it's not used then why allocate it?

[...]

> ---
>  .../ethernet/hisilicon/hibmcge/hbg_common.h   |  20 ++
>  .../net/ethernet/hisilicon/hibmcge/hbg_hw.c   |  59 ++++++
>  .../net/ethernet/hisilicon/hibmcge/hbg_hw.h   |   8 +
>  .../net/ethernet/hisilicon/hibmcge/hbg_irq.c  | 189 ++++++++++++++++++
>  .../net/ethernet/hisilicon/hibmcge/hbg_irq.h  |  13 ++
>  .../net/ethernet/hisilicon/hibmcge/hbg_main.c |   9 +
>  .../net/ethernet/hisilicon/hibmcge/hbg_reg.h  |  34 ++++
>  .../hisilicon/hibmcge/hbg_reg_union.h         |  60 ++++++
>  8 files changed, 392 insertions(+)
>  create mode 100644 drivers/net/ethernet/hisilicon/hibmcge/hbg_irq.c
>  create mode 100644 drivers/net/ethernet/hisilicon/hibmcge/hbg_irq.h
 
[...]

> +
> +static const char *irq_names_map[HBG_VECTOR_NUM] = { "tx", "rx", "err", "mdio" };
> +
> +int hbg_irq_init(struct hbg_priv *priv)
> +{
> +	struct hbg_vector *vectors = &priv->vectors;
> +	struct hbg_irq *irq;
> +	int ret;
> +	int i;
> +
> +	ret = pci_alloc_irq_vectors(priv->pdev, HBG_VECTOR_NUM, HBG_VECTOR_NUM,
> +				    PCI_IRQ_MSI);

No MSI-X ?

This seems to request HBG_VECTOR_NUM (4) IRQs and errors out if ret
!= HBG_VECTOR_NUM, but ...

> +	if (ret < 0) {
> +		dev_err(&priv->pdev->dev,
> +			"failed to allocate MSI vectors, vectors = %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (ret != HBG_VECTOR_NUM) {
> +		dev_err(&priv->pdev->dev,
> +			"requested %u MSI, but allocated %d MSI\n",
> +			HBG_VECTOR_NUM, ret);
> +		ret = -EINVAL;
> +		goto free_vectors;
> +	}
> +
> +	vectors->irqs = devm_kcalloc(&priv->pdev->dev, HBG_VECTOR_NUM,
> +				     sizeof(struct hbg_irq), GFP_KERNEL);
> +	if (!vectors->irqs) {
> +		ret = -ENOMEM;
> +		goto free_vectors;
> +	}
> +
> +	/* mdio irq not request */
> +	vectors->irq_count = HBG_VECTOR_NUM - 1;

Here the comment says mdio is not requested? But it does seem like
the IRQ is allocated above, it's just unused?

Maybe above you should remove mdio completely if its not in use?

Or is it used later in some other patch or something?

> +	for (i = 0; i < vectors->irq_count; i++) {
> +		irq = &vectors->irqs[i];
> +		snprintf(irq->name, sizeof(irq->name) - 1, "%s-%s-%s",
> +			 HBG_DEV_NAME, pci_name(priv->pdev), irq_names_map[i]);
> +
> +		irq->id = pci_irq_vector(priv->pdev, i);
> +		irq_set_status_flags(irq->id, IRQ_NOAUTOEN);
> +		ret = request_irq(irq->id, hbg_irq_handle,
> +				  0, irq->name, priv);
> +		if (ret) {
> +			dev_err(&priv->pdev->dev,
> +				"failed to requset irq(%d), ret = %d\n",
> +				irq->id, ret);
> +			goto free_vectors;
> +		}
> +	}
> +
> +	vectors->info_array = hbg_irqs;
> +	vectors->info_array_len = ARRAY_SIZE(hbg_irqs);
> +	return 0;
> +
> +free_vectors:
> +	hbg_irq_uninit(priv);
> +	return ret;
> +}

[...]