Re: [PATCH] ibmvnic: remove excessive irqsave
On Fri, 5 Mar 2021 06:49:14 +0100 Christophe Leroy wrote: > Le 05/03/2021 ?? 02:43, angkery a ??crit?0?2: > > From: Junlin Yang > > > > ibmvnic_remove locks multiple spinlocks while disabling interrupts: > > spin_lock_irqsave(>state_lock, flags); > > spin_lock_irqsave(>rwi_lock, flags); > > > > there is no need for the second irqsave,since interrupts are > > disabled at that point, so remove the second irqsave: > > The probl??me is not that there is no need. The problem is a lot more > serious: As reported by coccinella, the second _irqsave() overwrites > the value saved in 'flags' by the first _irqsave, therefore when the > second _irqrestore comes, the value in 'flags' is not valid, the > value saved by the first _irqsave has been lost. This likely leads to > IRQs remaining disabled, which is _THE_ problem really. > Thank you for explaining the real problem. I will update the commit information with your description. > > spin_lock_irqsave(>state_lock, flags); > > spin_lock(>rwi_lock); > > > > Generated by: ./scripts/coccinelle/locks/flags.cocci > > ./drivers/net/ethernet/ibm/ibmvnic.c:5413:1-18: > > ERROR: nested lock+irqsave that reuses flags from line 5404. > > > > Signed-off-by: Junlin Yang > > --- > > drivers/net/ethernet/ibm/ibmvnic.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c > > b/drivers/net/ethernet/ibm/ibmvnic.c index 2464c8a..a52668d 100644 > > --- a/drivers/net/ethernet/ibm/ibmvnic.c > > +++ b/drivers/net/ethernet/ibm/ibmvnic.c > > @@ -5408,9 +5408,9 @@ static void ibmvnic_remove(struct vio_dev > > *dev) > > * after setting state, so __ibmvnic_reset() which is > > called > > * from the flush_work() below, can make progress. > > */ > > - spin_lock_irqsave(>rwi_lock, flags); > > + spin_lock(>rwi_lock); > > adapter->state = VNIC_REMOVING; > > - spin_unlock_irqrestore(>rwi_lock, flags); > > + spin_unlock(>rwi_lock); > > > > spin_unlock_irqrestore(>state_lock, flags); > > > >
Re: [PATCH] ibmvnic: remove excessive irqsave
> On Mar 4, 2021, at 11:49 PM, Christophe Leroy > wrote: > > > > Le 05/03/2021 à 02:43, angkery a écrit : >> From: Junlin Yang >> ibmvnic_remove locks multiple spinlocks while disabling interrupts: >> spin_lock_irqsave(>state_lock, flags); >> spin_lock_irqsave(>rwi_lock, flags); >> there is no need for the second irqsave,since interrupts are disabled >> at that point, so remove the second irqsave: > > The problème is not that there is no need. The problem is a lot more serious: > As reported by coccinella, the second _irqsave() overwrites the value saved > in 'flags' by the first _irqsave, therefore when the second _irqrestore > comes, the value in 'flags' is not valid, the value saved by the first > _irqsave has been lost. This likely leads to IRQs remaining disabled, which > is _THE_ problem really. That does sounds like a more serious functional problem than coccinella check. Thanks for your explanation. > >> spin_lock_irqsave(>state_lock, flags); >> spin_lock(>rwi_lock); >> Generated by: ./scripts/coccinelle/locks/flags.cocci >> ./drivers/net/ethernet/ibm/ibmvnic.c:5413:1-18: >> ERROR: nested lock+irqsave that reuses flags from line 5404. >> Signed-off-by: Junlin Yang >> --- >> drivers/net/ethernet/ibm/ibmvnic.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c >> b/drivers/net/ethernet/ibm/ibmvnic.c >> index 2464c8a..a52668d 100644 >> --- a/drivers/net/ethernet/ibm/ibmvnic.c >> +++ b/drivers/net/ethernet/ibm/ibmvnic.c >> @@ -5408,9 +5408,9 @@ static void ibmvnic_remove(struct vio_dev *dev) >> * after setting state, so __ibmvnic_reset() which is called >> * from the flush_work() below, can make progress. >> */ >> -spin_lock_irqsave(>rwi_lock, flags); >> +spin_lock(>rwi_lock); >> adapter->state = VNIC_REMOVING; >> -spin_unlock_irqrestore(>rwi_lock, flags); >> +spin_unlock(>rwi_lock); >> spin_unlock_irqrestore(>state_lock, flags); >>
Re: [PATCH] ibmvnic: remove excessive irqsave
Le 05/03/2021 à 02:43, angkery a écrit : From: Junlin Yang ibmvnic_remove locks multiple spinlocks while disabling interrupts: spin_lock_irqsave(>state_lock, flags); spin_lock_irqsave(>rwi_lock, flags); there is no need for the second irqsave,since interrupts are disabled at that point, so remove the second irqsave: The problème is not that there is no need. The problem is a lot more serious: As reported by coccinella, the second _irqsave() overwrites the value saved in 'flags' by the first _irqsave, therefore when the second _irqrestore comes, the value in 'flags' is not valid, the value saved by the first _irqsave has been lost. This likely leads to IRQs remaining disabled, which is _THE_ problem really. spin_lock_irqsave(>state_lock, flags); spin_lock(>rwi_lock); Generated by: ./scripts/coccinelle/locks/flags.cocci ./drivers/net/ethernet/ibm/ibmvnic.c:5413:1-18: ERROR: nested lock+irqsave that reuses flags from line 5404. Signed-off-by: Junlin Yang --- drivers/net/ethernet/ibm/ibmvnic.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c index 2464c8a..a52668d 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.c +++ b/drivers/net/ethernet/ibm/ibmvnic.c @@ -5408,9 +5408,9 @@ static void ibmvnic_remove(struct vio_dev *dev) * after setting state, so __ibmvnic_reset() which is called * from the flush_work() below, can make progress. */ - spin_lock_irqsave(>rwi_lock, flags); + spin_lock(>rwi_lock); adapter->state = VNIC_REMOVING; - spin_unlock_irqrestore(>rwi_lock, flags); + spin_unlock(>rwi_lock); spin_unlock_irqrestore(>state_lock, flags);
Re: [PATCH] ibmvnic: remove excessive irqsave
angkery [angk...@163.com] wrote:
> From: Junlin Yang
>
> ibmvnic_remove locks multiple spinlocks while disabling interrupts:
> spin_lock_irqsave(>state_lock, flags);
> spin_lock_irqsave(>rwi_lock, flags);
>
> there is no need for the second irqsave,since interrupts are disabled
> at that point, so remove the second irqsave:
> spin_lock_irqsave(>state_lock, flags);
> spin_lock(>rwi_lock);
>
> Generated by: ./scripts/coccinelle/locks/flags.cocci
> ./drivers/net/ethernet/ibm/ibmvnic.c:5413:1-18:
> ERROR: nested lock+irqsave that reuses flags from line 5404.
>
Thanks. Please add
Fixes: 4a41c421f367 ("ibmvnic: serialize access to work queue on remove")
> Signed-off-by: Junlin Yang
Reviewed-by: Sukadev Bhattiprolu
