| Message ID | a1b6f082fff4e68007914577961113bc452c8030.1652629833.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | octeon_ep: Fix the error handling path of octep_request_irqs() | expand |
> -----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
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
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
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 > >
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) {
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(-)