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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zqo4mGq88BajjLk_@LQ3V64L9R2>
Date: Wed, 31 Jul 2024 14:14:00 +0100
From: Joe Damato <jdamato@...tly.com>
To: Jijie Shao <shaojijie@...wei.com>
Cc: yisen.zhuang@...wei.com, salil.mehta@...wei.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	horms@...nel.org, shenjian15@...wei.com, wangpeiyang1@...wei.com,
	liuyonglong@...wei.com, sudongming1@...wei.com,
	xujunsheng@...wei.com, shiyongbang@...wei.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH net-next 04/10] net: hibmcge: Add interrupt supported
 in this module

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;
> +}

[...]