[PATCH 0/2] octeon_ep: Fix the error handling path of octep_request_irqs()

[PATCH 0/2] octeon_ep: Fix the error handling path of octep_request_irqs()

[Christophe JAILLET]

I send a small serie to ease review and because I'm sighly less confident with
the 2nd patch.

They are related to the same Fixes: tag, so they obviously could be merged if
it is preferred.

Christophe JAILLET (2):
  octeon_ep: Fix a memory leak in the error handling path of
    octep_request_irqs()
  octeon_ep: Fix irq releasing in the error handling path of
    octep_request_irqs()

 drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH 0/2] octeon_ep: Fix the error handling path of octep_request_irqs()

[Christophe JAILLET]

I send a small serie to ease review and because I'm sighly less confident with
the 2nd patch.

They are related to the same Fixes: tag, so they obviously could be merged if
it is preferred.

Christophe JAILLET (2):
  octeon_ep: Fix a memory leak in the error handling path of
    octep_request_irqs()
  octeon_ep: Fix irq releasing in the error handling path of
    octep_request_irqs()

 drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH 1/2] octeon_ep: Fix a memory leak in the error handling path of octep_request_irqs()

[Christophe JAILLET]

'oct->non_ioq_irq_names' is not freed in the error handling path of
octep_request_irqs().

Add the missing kfree().

Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt support")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
index e020c81f3455..6b60a03574a0 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
@@ -267,6 +267,8 @@ static int octep_request_irqs(struct octep_device *oct)
 		--i;
 		free_irq(oct->msix_entries[i].vector, oct);
 	}
+	kfree(oct->non_ioq_irq_names);
+	oct->non_ioq_irq_names = NULL;
 alloc_err:
 	return -1;
 }
-- 
2.34.1

RE: [EXT] [PATCH 1/2] octeon_ep: Fix a memory leak in the error handling path of octep_request_irqs()

[Veerasenareddy Burru]

> -----Original Message-----
> From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Sent: Sunday, May 15, 2022 8:57 AM
> To: Veerasenareddy Burru <vburru@marvell.com>; Abhijit Ayarekar
> <aayarekar@marvell.com>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> Paolo Abeni <pabeni@redhat.com>; Satananda Burla <sburla@marvell.com>
> Cc: linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org;
> Christophe JAILLET <christophe.jaillet@wanadoo.fr>;
> netdev@vger.kernel.org
> Subject: [EXT] [PATCH 1/2] octeon_ep: Fix a memory leak in the error
> handling path of octep_request_irqs()
> 
> External Email
> 
> ----------------------------------------------------------------------
> 'oct->non_ioq_irq_names' is not freed in the error handling path of
> octep_request_irqs().
> 
> Add the missing kfree().
> 
> Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt
> support")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> index e020c81f3455..6b60a03574a0 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> @@ -267,6 +267,8 @@ static int octep_request_irqs(struct octep_device
> *oct)
>  		--i;
>  		free_irq(oct->msix_entries[i].vector, oct);
>  	}
> +	kfree(oct->non_ioq_irq_names);
> +	oct->non_ioq_irq_names = NULL;
Ack
Thanks for the change.

Regards,
Veerasenareddy
>  alloc_err:
>  	return -1;
>  }
> --
> 2.34.1

[PATCH 2/2] octeon_ep: Fix irq releasing in the error handling path of octep_request_irqs()

[Christophe JAILLET]

For the error handling to work as expected, the index in the
'oct->msix_entries' array must be tweaked because, when the irq are
requested there is:
	msix_entry = &oct->msix_entries[i + num_non_ioq_msix];

So in the error handling path, 'i + num_non_ioq_msix' should be used
instead of 'i'.

The 2nd argument of free_irq() also needs to be adjusted.

Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt support")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
I think that the wording above is awful, but I'm sure you get it.
Feel free to rephrase everything to have it more readable.
---
 drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
index 6b60a03574a0..4dcae805422b 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
@@ -257,10 +257,12 @@ static int octep_request_irqs(struct octep_device *oct)
 
 	return 0;
 ioq_irq_err:
+	i += num_non_ioq_msix;
 	while (i > num_non_ioq_msix) {
 		--i;
 		irq_set_affinity_hint(oct->msix_entries[i].vector, NULL);
-		free_irq(oct->msix_entries[i].vector, oct->ioq_vector[i]);
+		free_irq(oct->msix_entries[i].vector,
+			 oct->ioq_vector[i - num_non_ioq_msix]);
 	}
 non_ioq_irq_err:
 	while (i) {
-- 
2.34.1

RE: [EXT] [PATCH 2/2] octeon_ep: Fix irq releasing in the error handling path of octep_request_irqs()

[Veerasenareddy Burru]

> -----Original Message-----
> From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Sent: Sunday, May 15, 2022 8:57 AM
> To: Veerasenareddy Burru <vburru@marvell.com>; Abhijit Ayarekar
> <aayarekar@marvell.com>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> Paolo Abeni <pabeni@redhat.com>; Satananda Burla <sburla@marvell.com>
> Cc: linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org;
> Christophe JAILLET <christophe.jaillet@wanadoo.fr>;
> netdev@vger.kernel.org
> Subject: [EXT] [PATCH 2/2] octeon_ep: Fix irq releasing in the error handling
> path of octep_request_irqs()
> 
> External Email
> 
> ----------------------------------------------------------------------
> For the error handling to work as expected, the index in the 'oct-
> >msix_entries' array must be tweaked because, when the irq are requested
> there is:
> 	msix_entry = &oct->msix_entries[i + num_non_ioq_msix];
> 
> So in the error handling path, 'i + num_non_ioq_msix' should be used instead
> of 'i'.
> 
> The 2nd argument of free_irq() also needs to be adjusted.
> 
> Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt
> support")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> I think that the wording above is awful, but I'm sure you get it.
> Feel free to rephrase everything to have it more readable.
> ---
>  drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> index 6b60a03574a0..4dcae805422b 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> @@ -257,10 +257,12 @@ static int octep_request_irqs(struct octep_device
> *oct)
> 
>  	return 0;
>  ioq_irq_err:
> +	i += num_non_ioq_msix;
>  	while (i > num_non_ioq_msix) {
>  		--i;
>  		irq_set_affinity_hint(oct->msix_entries[i].vector, NULL);
> -		free_irq(oct->msix_entries[i].vector, oct->ioq_vector[i]);
> +		free_irq(oct->msix_entries[i].vector,
> +			 oct->ioq_vector[i - num_non_ioq_msix]);
Ack.
Thanks for the fix.

Regards,
Veerasenareddy.
>  	}
>  non_ioq_irq_err:
>  	while (i) {
> --
> 2.34.1

Re: [PATCH 2/2] octeon_ep: Fix irq releasing in the error handling path of octep_request_irqs()

[Dan Carpenter]

On Sun, May 15, 2022 at 05:56:45PM +0200, Christophe JAILLET wrote:
> For the error handling to work as expected, the index in the
> 'oct->msix_entries' array must be tweaked because, when the irq are
> requested there is:
> 	msix_entry = &oct->msix_entries[i + num_non_ioq_msix];
> 
> So in the error handling path, 'i + num_non_ioq_msix' should be used
> instead of 'i'.
> 
> The 2nd argument of free_irq() also needs to be adjusted.
> 
> Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt support")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> I think that the wording above is awful, but I'm sure you get it.
> Feel free to rephrase everything to have it more readable.
> ---
>  drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> index 6b60a03574a0..4dcae805422b 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> @@ -257,10 +257,12 @@ static int octep_request_irqs(struct octep_device *oct)
>  
>  	return 0;
>  ioq_irq_err:
> +	i += num_non_ioq_msix;
>  	while (i > num_non_ioq_msix) {

This makes my mind hurt so badly.  Can we not just have two variables
for the two different loops instead of re-using i?

>  		--i;
>  		irq_set_affinity_hint(oct->msix_entries[i].vector, NULL);
> -		free_irq(oct->msix_entries[i].vector, oct->ioq_vector[i]);
> +		free_irq(oct->msix_entries[i].vector,
> +			 oct->ioq_vector[i - num_non_ioq_msix]);
>  	}

ioq_irq_err:
        while (--j >= 0) {
                ioq_vector = oct->ioq_vector[j];
                msix_entry = &oct->msix_entries[j + num_non_ioq_msix];

                irq_set_affinity_hint(msix_entry->vector, NULL);
                free_irq(msix_entry->vector, ioq_vector);
        }

regards,
dan carpenter

Re: [PATCH 2/2] octeon_ep: Fix irq releasing in the error handling path of octep_request_irqs()

[Paolo Abeni]

On Tue, 2022-05-17 at 08:28 +0300, Dan Carpenter wrote:
> On Sun, May 15, 2022 at 05:56:45PM +0200, Christophe JAILLET wrote:
> > For the error handling to work as expected, the index in the
> > 'oct->msix_entries' array must be tweaked because, when the irq are
> > requested there is:
> > 	msix_entry = &oct->msix_entries[i + num_non_ioq_msix];
> > 
> > So in the error handling path, 'i + num_non_ioq_msix' should be used
> > instead of 'i'.
> > 
> > The 2nd argument of free_irq() also needs to be adjusted.
> > 
> > Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt support")
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > ---
> > I think that the wording above is awful, but I'm sure you get it.
> > Feel free to rephrase everything to have it more readable.
> > ---
> >  drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > index 6b60a03574a0..4dcae805422b 100644
> > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > @@ -257,10 +257,12 @@ static int octep_request_irqs(struct octep_device *oct)
> >  
> >  	return 0;
> >  ioq_irq_err:
> > +	i += num_non_ioq_msix;
> >  	while (i > num_non_ioq_msix) {
> 
> This makes my mind hurt so badly.  Can we not just have two variables
> for the two different loops instead of re-using i?
> 
> >  		--i;
> >  		irq_set_affinity_hint(oct->msix_entries[i].vector, NULL);
> > -		free_irq(oct->msix_entries[i].vector, oct->ioq_vector[i]);
> > +		free_irq(oct->msix_entries[i].vector,
> > +			 oct->ioq_vector[i - num_non_ioq_msix]);
> >  	}
> 
> ioq_irq_err:
>         while (--j >= 0) {
>                 ioq_vector = oct->ioq_vector[j];
>                 msix_entry = &oct->msix_entries[j + num_non_ioq_msix];
> 
>                 irq_set_affinity_hint(msix_entry->vector, NULL);
>                 free_irq(msix_entry->vector, ioq_vector);
>         }
> 
> regards,
> dan carpenter

I agree the above would be more readable. @Christophe: could you please
refactor the code as per Dan's suggestion?

Thanks!

Paolo

Re: [PATCH 2/2] octeon_ep: Fix irq releasing in the error handling path of octep_request_irqs()

[Christophe JAILLET]

Le 17/05/2022 à 10:35, Paolo Abeni a écrit :
> On Tue, 2022-05-17 at 08:28 +0300, Dan Carpenter wrote:
>> On Sun, May 15, 2022 at 05:56:45PM +0200, Christophe JAILLET wrote:
>>> For the error handling to work as expected, the index in the
>>> 'oct->msix_entries' array must be tweaked because, when the irq are
>>> requested there is:
>>> 	msix_entry = &oct->msix_entries[i + num_non_ioq_msix];
>>>
>>> So in the error handling path, 'i + num_non_ioq_msix' should be used
>>> instead of 'i'.
>>>
>>> The 2nd argument of free_irq() also needs to be adjusted.
>>>
>>> Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt support")
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>> ---
>>> I think that the wording above is awful, but I'm sure you get it.
>>> Feel free to rephrase everything to have it more readable.
>>> ---
>>>   drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
>>> index 6b60a03574a0..4dcae805422b 100644
>>> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
>>> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
>>> @@ -257,10 +257,12 @@ static int octep_request_irqs(struct octep_device *oct)
>>>   
>>>   	return 0;
>>>   ioq_irq_err:
>>> +	i += num_non_ioq_msix;
>>>   	while (i > num_non_ioq_msix) {
>>
>> This makes my mind hurt so badly.  Can we not just have two variables
>> for the two different loops instead of re-using i?
>>
>>>   		--i;
>>>   		irq_set_affinity_hint(oct->msix_entries[i].vector, NULL);
>>> -		free_irq(oct->msix_entries[i].vector, oct->ioq_vector[i]);
>>> +		free_irq(oct->msix_entries[i].vector,
>>> +			 oct->ioq_vector[i - num_non_ioq_msix]);
>>>   	}
>>
>> ioq_irq_err:
>>          while (--j >= 0) {
>>                  ioq_vector = oct->ioq_vector[j];
>>                  msix_entry = &oct->msix_entries[j + num_non_ioq_msix];
>>
>>                  irq_set_affinity_hint(msix_entry->vector, NULL);
>>                  free_irq(msix_entry->vector, ioq_vector);
>>          }
>>
>> regards,
>> dan carpenter
> 
> I agree the above would be more readable. @Christophe: could you please
> refactor the code as per Dan's suggestion?

Will do.

I was sure that Dan would comment on this unusual pattern :)

CJ

> 
> Thanks!
> 
> Paolo
> 
>