netdev - RE: [PATCH net v3 1/4] net: txgbe: initialize num_q

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: <075201daccea$a33739d0$e9a5ad70$@trustnetic.com>
Date: Wed, 3 Jul 2024 09:45:09 +0800
From: Jiawen Wu <jiawenwu@...stnetic.com>
To: "'Michal Kubiak'" <michal.kubiak@...el.com>
Cc: <davem@...emloft.net>,
	<edumazet@...gle.com>,
	<kuba@...nel.org>,
	<pabeni@...hat.com>,
	<horms@...nel.org>,
	<andrew@...n.ch>,
	<netdev@...r.kernel.org>,
	<przemyslaw.kitszel@...el.com>,
	<mengyuanlou@...-swift.com>,
	<duanqiangwen@...-swift.com>
Subject: RE: [PATCH net v3 1/4] net: txgbe: initialize num_q_vectors for MSI/INTx interrupts

On Tue, Jul 2, 2024 11:22 PM, Michal Kubiak wrote:
> On Mon, Jul 01, 2024 at 03:14:13PM +0800, Jiawen Wu wrote:
> > When using MSI/INTx interrupts, wx->num_q_vectors is uninitialized.
> > Thus there will be kernel panic in wx_alloc_q_vectors() to allocate
> > queue vectors.
> >
> > Fixes: 3f703186113f ("net: libwx: Add irq flow functions")
> > Signed-off-by: Jiawen Wu <jiawenwu@...stnetic.com>
> > ---
> >  drivers/net/ethernet/wangxun/libwx/wx_lib.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > index 68bde91b67a0..f53776877f71 100644
> > --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > @@ -1686,6 +1686,7 @@ static int wx_set_interrupt_capability(struct wx *wx)
> >  	}
> >
> >  	pdev->irq = pci_irq_vector(pdev, 0);
> > +	wx->num_q_vectors = 1;
> 
> I would suggest improving readability of that logic. TBH, initially it wasn't
> obvious to me why you assign 1 to num_q_vectors (instead of nvecs variable).
> Maybe you just want to exit with an error when nvecs != 1 and avoid some nesting.
> I think that should make that logic easier to read. For example:
> 
>         /* minmum one for queue, one for misc*/
>         nvecs = 1;
>         nvecs = pci_alloc_irq_vectors(pdev, nvecs,
>                                       nvecs, PCI_IRQ_MSI | PCI_IRQ_INTX);
>         if (nvecs != 1) {
>                 wx_err(wx, "Failed to allocate MSI/INTx interrupts. Error: %d\n", nvecs);
>                 return nvecs;
>         }
> 
>         if (pdev->msi_enabled)
>                 wx_err(wx, "Fallback to MSI.\n");
>         else
>                 wx_err(wx, "Fallback to INTx.\n");
> 
>         pdev->irq = pci_irq_vector(pdev, 0);
> 	wx->num_q_vectors = 1;
> 
> (Please consider it as a suggestion only).

Thanks for the suggestion, it can be considered as a commit for code optimization.