[PATCH net 1/1] nfp: correct number of MSI vectors requests returned

[PATCH net 1/1] nfp: correct number of MSI vectors requests returned

[Louis Peens]

From: Xiaoyu Li <xiaoyu.li@corigine.com>

Before the referenced commit, when we requested a
certain number of interrupts, if we could not meet
the requirements, the number of interrupts supported
by the hardware would be returned. But after the
referenced commit, if the hardware failed to meet
the requirements, the error of invalid argument
would be directly returned, which caused a regression
in the nfp driver preventing probing to complete.

Fixes: bab65e48cb06 ("PCI/MSI: Sanitize MSI-X checks")
Cc: stable@vger.kernel.org
Signed-off-by: Xiaoyu Li <xiaoyu.li@corigine.com>
Acked-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
---
 drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 62f0bf91d1e1..0e4cab38f075 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -370,6 +370,12 @@ nfp_net_irqs_alloc(struct pci_dev *pdev, struct msix_entry *irq_entries,
 {
 	unsigned int i;
 	int got_irqs;
+	int max_irqs;
+
+	max_irqs = pci_msix_vec_count(pdev);
+	if (max_irqs < 0)
+		return max_irqs;
+	wanted_irqs = min_t(unsigned int, max_irqs, wanted_irqs);
 
 	for (i = 0; i < wanted_irqs; i++)
 		irq_entries[i].entry = i;
-- 
2.34.1

Re: [PATCH net 1/1] nfp: correct number of MSI vectors requests returned

[Leon Romanovsky]

On Wed, Mar 15, 2023 at 02:17:33PM +0200, Louis Peens wrote:
> From: Xiaoyu Li <xiaoyu.li@corigine.com>
> 
> Before the referenced commit, when we requested a
> certain number of interrupts, if we could not meet
> the requirements, the number of interrupts supported
> by the hardware would be returned. But after the
> referenced commit, if the hardware failed to meet
> the requirements, the error of invalid argument
> would be directly returned, which caused a regression
> in the nfp driver preventing probing to complete.

Please don't break lines. You have upto 80 chars per-line.

> 
> Fixes: bab65e48cb06 ("PCI/MSI: Sanitize MSI-X checks")
> Cc: stable@vger.kernel.org
> Signed-off-by: Xiaoyu Li <xiaoyu.li@corigine.com>
> Acked-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Louis Peens <louis.peens@corigine.com>
> ---
>  drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> index 62f0bf91d1e1..0e4cab38f075 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> @@ -370,6 +370,12 @@ nfp_net_irqs_alloc(struct pci_dev *pdev, struct msix_entry *irq_entries,
>  {
>  	unsigned int i;
>  	int got_irqs;
> +	int max_irqs;
> +
> +	max_irqs = pci_msix_vec_count(pdev);
> +	if (max_irqs < 0)
> +		return max_irqs;
> +	wanted_irqs = min_t(unsigned int, max_irqs, wanted_irqs);

1. It looks like you need to fix your nfp_net_irqs_alloc() to provide
valid wanted_irqs from the beginning.
2. Both wanted_irqs and min_irqs are wrong type and should be int and
not unsigned int.

Thanks

>  
>  	for (i = 0; i < wanted_irqs; i++)
>  		irq_entries[i].entry = i;
> -- 
> 2.34.1
> 

Re: [PATCH net 1/1] nfp: correct number of MSI vectors requests returned

[Jakub Kicinski]

On Wed, 15 Mar 2023 14:17:33 +0200 Louis Peens wrote:
> From: Xiaoyu Li <xiaoyu.li@corigine.com>
> 
> Before the referenced commit, when we requested a
> certain number of interrupts, if we could not meet
> the requirements, the number of interrupts supported
> by the hardware would be returned. But after the
> referenced commit, if the hardware failed to meet
> the requirements, the error of invalid argument
> would be directly returned, which caused a regression
> in the nfp driver preventing probing to complete.
> 
> Fixes: bab65e48cb06 ("PCI/MSI: Sanitize MSI-X checks")
> Cc: stable@vger.kernel.org
> Signed-off-by: Xiaoyu Li <xiaoyu.li@corigine.com>

Thomas, is this an expected side effect?

The commit message of bab65e48cb06 ("PCI/MSI: Sanitize MSI-X checks")
makes it sound like only a harmless refactoring was intended.

> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> index 62f0bf91d1e1..0e4cab38f075 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> @@ -370,6 +370,12 @@ nfp_net_irqs_alloc(struct pci_dev *pdev, struct msix_entry *irq_entries,
>  {
>  	unsigned int i;
>  	int got_irqs;
> +	int max_irqs;
> +
> +	max_irqs = pci_msix_vec_count(pdev);
> +	if (max_irqs < 0)
> +		return max_irqs;
> +	wanted_irqs = min_t(unsigned int, max_irqs, wanted_irqs);
>  
>  	for (i = 0; i < wanted_irqs; i++)
>  		irq_entries[i].entry = i;

Re: [PATCH net 1/1] nfp: correct number of MSI vectors requests returned

[Jakub Kicinski]

On Thu, 16 Mar 2023 13:09:43 +0200 Leon Romanovsky wrote:
> On Wed, Mar 15, 2023 at 02:17:33PM +0200, Louis Peens wrote:
> > From: Xiaoyu Li <xiaoyu.li@corigine.com>
> > 
> > Before the referenced commit, when we requested a
> > certain number of interrupts, if we could not meet
> > the requirements, the number of interrupts supported
> > by the hardware would be returned. But after the
> > referenced commit, if the hardware failed to meet
> > the requirements, the error of invalid argument
> > would be directly returned, which caused a regression
> > in the nfp driver preventing probing to complete.  
> 
> Please don't break lines. You have upto 80 chars per-line.

72 I think, git adds an indentation. Not that I personally care
about "not using full lines".

> > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> > index 62f0bf91d1e1..0e4cab38f075 100644
> > --- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> > +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> > @@ -370,6 +370,12 @@ nfp_net_irqs_alloc(struct pci_dev *pdev, struct msix_entry *irq_entries,
> >  {
> >  	unsigned int i;
> >  	int got_irqs;
> > +	int max_irqs;
> > +
> > +	max_irqs = pci_msix_vec_count(pdev);
> > +	if (max_irqs < 0)
> > +		return max_irqs;
> > +	wanted_irqs = min_t(unsigned int, max_irqs, wanted_irqs);  
> 
> 1. It looks like you need to fix your nfp_net_irqs_alloc() to provide
> valid wanted_irqs from the beginning.

Right, why do you have this problem in the first place?
Could you provide some concrete numbers?

Re: [PATCH net 1/1] nfp: correct number of MSI vectors requests returned

[Leon Romanovsky]

On Thu, Mar 16, 2023 at 02:27:10PM -0700, Jakub Kicinski wrote:
> On Thu, 16 Mar 2023 13:09:43 +0200 Leon Romanovsky wrote:
> > On Wed, Mar 15, 2023 at 02:17:33PM +0200, Louis Peens wrote:
> > > From: Xiaoyu Li <xiaoyu.li@corigine.com>
> > > 
> > > Before the referenced commit, when we requested a
> > > certain number of interrupts, if we could not meet
> > > the requirements, the number of interrupts supported
> > > by the hardware would be returned. But after the
> > > referenced commit, if the hardware failed to meet
> > > the requirements, the error of invalid argument
> > > would be directly returned, which caused a regression
> > > in the nfp driver preventing probing to complete.  
> > 
> > Please don't break lines. You have upto 80 chars per-line.
> 
> 72 I think, git adds an indentation. Not that I personally care
> about "not using full lines".

I care about typography.

Thanks

Re: [PATCH net 1/1] nfp: correct number of MSI vectors requests returned

[Jakub Kicinski]

On Sun, 19 Mar 2023 13:19:44 +0200 Leon Romanovsky wrote:
> On Thu, Mar 16, 2023 at 02:27:10PM -0700, Jakub Kicinski wrote:
> > On Thu, 16 Mar 2023 13:09:43 +0200 Leon Romanovsky wrote:  
> > > Please don't break lines. You have upto 80 chars per-line.  
> > 
> > 72 I think, git adds an indentation. Not that I personally care
> > about "not using full lines".  
> 
> I care about typography.

Typography of commit messages? Let's make sure the code is right.

We may have some fundamental disagreements about line length
and identifier name length, looking at mlx5 code :(

Re: [PATCH net 1/1] nfp: correct number of MSI vectors requests returned

[Leon Romanovsky]

On Sun, Mar 19, 2023 at 11:48:15AM -0700, Jakub Kicinski wrote:
> On Sun, 19 Mar 2023 13:19:44 +0200 Leon Romanovsky wrote:
> > On Thu, Mar 16, 2023 at 02:27:10PM -0700, Jakub Kicinski wrote:
> > > On Thu, 16 Mar 2023 13:09:43 +0200 Leon Romanovsky wrote:  
> > > > Please don't break lines. You have upto 80 chars per-line.  
> > > 
> > > 72 I think, git adds an indentation. Not that I personally care
> > > about "not using full lines".  
> > 
> > I care about typography.
> 
> Typography of commit messages? 

Everything and commit messages too.

> Let's make sure the code is right.

I rarely give comments about "style" only. This specific comment
came together with attempt to make the code right.

> 
> We may have some fundamental disagreements about line length
> and identifier name length, looking at mlx5 code :(

I have very little influence on how mlx5_core/_en looks like.

Thanks