All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net] ibmvnic: device remove has higher precedence over reset
@ 2021-01-21  6:20 ` Lijun Pan
  0 siblings, 0 replies; 10+ messages in thread
From: Lijun Pan @ 2021-01-21  6:20 UTC (permalink / raw)
  To: netdev
  Cc: linuxppc-dev, drt, sukadev, mpe, julietk, benh, paulus, davem,
	kuba, gregkh, kernel, Lijun Pan, Uwe Kleine-König

Returning -EBUSY in ibmvnic_remove() does not actually hold the
removal procedure since driver core doesn't care for the return
value (see __device_release_driver() in drivers/base/dd.c
calling dev->bus->remove()) though vio_bus_remove
(in arch/powerpc/platforms/pseries/vio.c) records the
return value and passes it on. [1]

During the device removal precedure, we should not schedule
any new reset (ibmvnic_reset check for REMOVING and exit),
and should rely on the flush_work and flush_delayed_work
to complete the pending resets, specifically we need to
let __ibmvnic_reset() keep running while in REMOVING state since
flush_work and flush_delayed_work shall call __ibmvnic_reset finally.
So we skip the checking for REMOVING in __ibmvnic_reset.

[1] https://lore.kernel.org/linuxppc-dev/20210117101242.dpwayq6wdgfdzirl@pengutronix.de/T/#m48f5befd96bc9842ece2a3ad14f4c27747206a53
Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Fixes: 7d7195a026ba ("ibmvnic: Do not process device remove during device reset")
Signed-off-by: Lijun Pan <ljp@linux.ibm.com>
---
v1 versus RFC: 
  1/ articulate why remove the REMOVING checking in __ibmvnic_reset
  and why keep the current checking for REMOVING in ibmvnic_reset.
  2/ The locking issue mentioned by Uwe are being addressed separately 
     by	https://lists.openwall.net/netdev/2021/01/08/89
  3/ This patch does not have merge conflict with 2/

 drivers/net/ethernet/ibm/ibmvnic.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index aed985e08e8a..11f28fd03057 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -2235,8 +2235,7 @@ static void __ibmvnic_reset(struct work_struct *work)
 	while (rwi) {
 		spin_lock_irqsave(&adapter->state_lock, flags);
 
-		if (adapter->state == VNIC_REMOVING ||
-		    adapter->state == VNIC_REMOVED) {
+		if (adapter->state == VNIC_REMOVED) {
 			spin_unlock_irqrestore(&adapter->state_lock, flags);
 			kfree(rwi);
 			rc = EBUSY;
@@ -5372,11 +5371,6 @@ static int ibmvnic_remove(struct vio_dev *dev)
 	unsigned long flags;
 
 	spin_lock_irqsave(&adapter->state_lock, flags);
-	if (test_bit(0, &adapter->resetting)) {
-		spin_unlock_irqrestore(&adapter->state_lock, flags);
-		return -EBUSY;
-	}
-
 	adapter->state = VNIC_REMOVING;
 	spin_unlock_irqrestore(&adapter->state_lock, flags);
 
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net] ibmvnic: device remove has higher precedence over reset
@ 2021-01-21  6:20 ` Lijun Pan
  0 siblings, 0 replies; 10+ messages in thread
From: Lijun Pan @ 2021-01-21  6:20 UTC (permalink / raw)
  To: netdev
  Cc: Lijun Pan, gregkh, julietk, Uwe Kleine-König, paulus,
	kernel, drt, kuba, sukadev, linuxppc-dev, davem

Returning -EBUSY in ibmvnic_remove() does not actually hold the
removal procedure since driver core doesn't care for the return
value (see __device_release_driver() in drivers/base/dd.c
calling dev->bus->remove()) though vio_bus_remove
(in arch/powerpc/platforms/pseries/vio.c) records the
return value and passes it on. [1]

During the device removal precedure, we should not schedule
any new reset (ibmvnic_reset check for REMOVING and exit),
and should rely on the flush_work and flush_delayed_work
to complete the pending resets, specifically we need to
let __ibmvnic_reset() keep running while in REMOVING state since
flush_work and flush_delayed_work shall call __ibmvnic_reset finally.
So we skip the checking for REMOVING in __ibmvnic_reset.

[1] https://lore.kernel.org/linuxppc-dev/20210117101242.dpwayq6wdgfdzirl@pengutronix.de/T/#m48f5befd96bc9842ece2a3ad14f4c27747206a53
Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Fixes: 7d7195a026ba ("ibmvnic: Do not process device remove during device reset")
Signed-off-by: Lijun Pan <ljp@linux.ibm.com>
---
v1 versus RFC: 
  1/ articulate why remove the REMOVING checking in __ibmvnic_reset
  and why keep the current checking for REMOVING in ibmvnic_reset.
  2/ The locking issue mentioned by Uwe are being addressed separately 
     by	https://lists.openwall.net/netdev/2021/01/08/89
  3/ This patch does not have merge conflict with 2/

 drivers/net/ethernet/ibm/ibmvnic.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index aed985e08e8a..11f28fd03057 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -2235,8 +2235,7 @@ static void __ibmvnic_reset(struct work_struct *work)
 	while (rwi) {
 		spin_lock_irqsave(&adapter->state_lock, flags);
 
-		if (adapter->state == VNIC_REMOVING ||
-		    adapter->state == VNIC_REMOVED) {
+		if (adapter->state == VNIC_REMOVED) {
 			spin_unlock_irqrestore(&adapter->state_lock, flags);
 			kfree(rwi);
 			rc = EBUSY;
@@ -5372,11 +5371,6 @@ static int ibmvnic_remove(struct vio_dev *dev)
 	unsigned long flags;
 
 	spin_lock_irqsave(&adapter->state_lock, flags);
-	if (test_bit(0, &adapter->resetting)) {
-		spin_unlock_irqrestore(&adapter->state_lock, flags);
-		return -EBUSY;
-	}
-
 	adapter->state = VNIC_REMOVING;
 	spin_unlock_irqrestore(&adapter->state_lock, flags);
 
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH net] ibmvnic: device remove has higher precedence over reset
  2021-01-21  6:20 ` Lijun Pan
@ 2021-01-21 18:24   ` Dany Madden
  -1 siblings, 0 replies; 10+ messages in thread
From: Dany Madden @ 2021-01-21 18:24 UTC (permalink / raw)
  To: Lijun Pan
  Cc: netdev, linuxppc-dev, sukadev, mpe, julietk, benh, paulus, davem,
	kuba, gregkh, kernel, Uwe Kleine-König

On 2021-01-20 22:20, Lijun Pan wrote:
> Returning -EBUSY in ibmvnic_remove() does not actually hold the
> removal procedure since driver core doesn't care for the return
> value (see __device_release_driver() in drivers/base/dd.c
> calling dev->bus->remove()) though vio_bus_remove
> (in arch/powerpc/platforms/pseries/vio.c) records the
> return value and passes it on. [1]
> 
> During the device removal precedure, we should not schedule
> any new reset (ibmvnic_reset check for REMOVING and exit),
> and should rely on the flush_work and flush_delayed_work
> to complete the pending resets, specifically we need to
> let __ibmvnic_reset() keep running while in REMOVING state since
> flush_work and flush_delayed_work shall call __ibmvnic_reset finally.
> So we skip the checking for REMOVING in __ibmvnic_reset.
> 
> [1]
> https://lore.kernel.org/linuxppc-dev/20210117101242.dpwayq6wdgfdzirl@pengutronix.de/T/#m48f5befd96bc9842ece2a3ad14f4c27747206a53
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Fixes: 7d7195a026ba ("ibmvnic: Do not process device remove during
> device reset")
> Signed-off-by: Lijun Pan <ljp@linux.ibm.com>
> ---
> v1 versus RFC:
>   1/ articulate why remove the REMOVING checking in __ibmvnic_reset
>   and why keep the current checking for REMOVING in ibmvnic_reset.
>   2/ The locking issue mentioned by Uwe are being addressed separately
>      by	https://lists.openwall.net/netdev/2021/01/08/89
>   3/ This patch does not have merge conflict with 2/
> 
>  drivers/net/ethernet/ibm/ibmvnic.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> b/drivers/net/ethernet/ibm/ibmvnic.c
> index aed985e08e8a..11f28fd03057 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -2235,8 +2235,7 @@ static void __ibmvnic_reset(struct work_struct 
> *work)
>  	while (rwi) {
>  		spin_lock_irqsave(&adapter->state_lock, flags);
> 
> -		if (adapter->state == VNIC_REMOVING ||
> -		    adapter->state == VNIC_REMOVED) {
> +		if (adapter->state == VNIC_REMOVED) {

If we do get here, we would crash because ibmvnic_remove() happened. It 
frees the adapter struct already.

>  			spin_unlock_irqrestore(&adapter->state_lock, flags);
>  			kfree(rwi);
>  			rc = EBUSY;
> @@ -5372,11 +5371,6 @@ static int ibmvnic_remove(struct vio_dev *dev)
>  	unsigned long flags;
> 
>  	spin_lock_irqsave(&adapter->state_lock, flags);
> -	if (test_bit(0, &adapter->resetting)) {
> -		spin_unlock_irqrestore(&adapter->state_lock, flags);
> -		return -EBUSY;
> -	}
> -
>  	adapter->state = VNIC_REMOVING;
>  	spin_unlock_irqrestore(&adapter->state_lock, flags);

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net] ibmvnic: device remove has higher precedence over reset
@ 2021-01-21 18:24   ` Dany Madden
  0 siblings, 0 replies; 10+ messages in thread
From: Dany Madden @ 2021-01-21 18:24 UTC (permalink / raw)
  To: Lijun Pan
  Cc: gregkh, julietk, netdev, Uwe Kleine-König, paulus, kernel,
	kuba, sukadev, linuxppc-dev, davem

On 2021-01-20 22:20, Lijun Pan wrote:
> Returning -EBUSY in ibmvnic_remove() does not actually hold the
> removal procedure since driver core doesn't care for the return
> value (see __device_release_driver() in drivers/base/dd.c
> calling dev->bus->remove()) though vio_bus_remove
> (in arch/powerpc/platforms/pseries/vio.c) records the
> return value and passes it on. [1]
> 
> During the device removal precedure, we should not schedule
> any new reset (ibmvnic_reset check for REMOVING and exit),
> and should rely on the flush_work and flush_delayed_work
> to complete the pending resets, specifically we need to
> let __ibmvnic_reset() keep running while in REMOVING state since
> flush_work and flush_delayed_work shall call __ibmvnic_reset finally.
> So we skip the checking for REMOVING in __ibmvnic_reset.
> 
> [1]
> https://lore.kernel.org/linuxppc-dev/20210117101242.dpwayq6wdgfdzirl@pengutronix.de/T/#m48f5befd96bc9842ece2a3ad14f4c27747206a53
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Fixes: 7d7195a026ba ("ibmvnic: Do not process device remove during
> device reset")
> Signed-off-by: Lijun Pan <ljp@linux.ibm.com>
> ---
> v1 versus RFC:
>   1/ articulate why remove the REMOVING checking in __ibmvnic_reset
>   and why keep the current checking for REMOVING in ibmvnic_reset.
>   2/ The locking issue mentioned by Uwe are being addressed separately
>      by	https://lists.openwall.net/netdev/2021/01/08/89
>   3/ This patch does not have merge conflict with 2/
> 
>  drivers/net/ethernet/ibm/ibmvnic.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> b/drivers/net/ethernet/ibm/ibmvnic.c
> index aed985e08e8a..11f28fd03057 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -2235,8 +2235,7 @@ static void __ibmvnic_reset(struct work_struct 
> *work)
>  	while (rwi) {
>  		spin_lock_irqsave(&adapter->state_lock, flags);
> 
> -		if (adapter->state == VNIC_REMOVING ||
> -		    adapter->state == VNIC_REMOVED) {
> +		if (adapter->state == VNIC_REMOVED) {

If we do get here, we would crash because ibmvnic_remove() happened. It 
frees the adapter struct already.

>  			spin_unlock_irqrestore(&adapter->state_lock, flags);
>  			kfree(rwi);
>  			rc = EBUSY;
> @@ -5372,11 +5371,6 @@ static int ibmvnic_remove(struct vio_dev *dev)
>  	unsigned long flags;
> 
>  	spin_lock_irqsave(&adapter->state_lock, flags);
> -	if (test_bit(0, &adapter->resetting)) {
> -		spin_unlock_irqrestore(&adapter->state_lock, flags);
> -		return -EBUSY;
> -	}
> -
>  	adapter->state = VNIC_REMOVING;
>  	spin_unlock_irqrestore(&adapter->state_lock, flags);

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net] ibmvnic: device remove has higher precedence over reset
  2021-01-21 18:24   ` Dany Madden
@ 2021-01-21 18:46     ` Lijun Pan
  -1 siblings, 0 replies; 10+ messages in thread
From: Lijun Pan @ 2021-01-21 18:46 UTC (permalink / raw)
  To: Dany Madden
  Cc: Lijun Pan, netdev, linuxppc-dev, sukadev, mpe, julietk, benh,
	paulus, davem, Jakub Kicinski, gregkh, kernel,
	Uwe Kleine-König

On Thu, Jan 21, 2021 at 12:42 PM Dany Madden <drt@linux.ibm.com> wrote:
>
> On 2021-01-20 22:20, Lijun Pan wrote:
> > Returning -EBUSY in ibmvnic_remove() does not actually hold the
> > removal procedure since driver core doesn't care for the return
> > value (see __device_release_driver() in drivers/base/dd.c
> > calling dev->bus->remove()) though vio_bus_remove
> > (in arch/powerpc/platforms/pseries/vio.c) records the
> > return value and passes it on. [1]
> >
> > During the device removal precedure, we should not schedule
> > any new reset (ibmvnic_reset check for REMOVING and exit),
> > and should rely on the flush_work and flush_delayed_work
> > to complete the pending resets, specifically we need to
> > let __ibmvnic_reset() keep running while in REMOVING state since
> > flush_work and flush_delayed_work shall call __ibmvnic_reset finally.
> > So we skip the checking for REMOVING in __ibmvnic_reset.
> >
> > [1]
> > https://lore.kernel.org/linuxppc-dev/20210117101242.dpwayq6wdgfdzirl@pengutronix.de/T/#m48f5befd96bc9842ece2a3ad14f4c27747206a53
> > Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Fixes: 7d7195a026ba ("ibmvnic: Do not process device remove during
> > device reset")
> > Signed-off-by: Lijun Pan <ljp@linux.ibm.com>
> > ---
> > v1 versus RFC:
> >   1/ articulate why remove the REMOVING checking in __ibmvnic_reset
> >   and why keep the current checking for REMOVING in ibmvnic_reset.
> >   2/ The locking issue mentioned by Uwe are being addressed separately
> >      by       https://lists.openwall.net/netdev/2021/01/08/89
> >   3/ This patch does not have merge conflict with 2/
> >
> >  drivers/net/ethernet/ibm/ibmvnic.c | 8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> > b/drivers/net/ethernet/ibm/ibmvnic.c
> > index aed985e08e8a..11f28fd03057 100644
> > --- a/drivers/net/ethernet/ibm/ibmvnic.c
> > +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> > @@ -2235,8 +2235,7 @@ static void __ibmvnic_reset(struct work_struct
> > *work)
> >       while (rwi) {
> >               spin_lock_irqsave(&adapter->state_lock, flags);
> >
> > -             if (adapter->state == VNIC_REMOVING ||
> > -                 adapter->state == VNIC_REMOVED) {
> > +             if (adapter->state == VNIC_REMOVED) {
>
> If we do get here, we would crash because ibmvnic_remove() happened. It
> frees the adapter struct already.

Not exactly. viodev is gone; netdev is done; ibmvnic_adapter is still there.

Lijun
>
> >                       spin_unlock_irqrestore(&adapter->state_lock, flags);
> >                       kfree(rwi);
> >                       rc = EBUSY;
> > @@ -5372,11 +5371,6 @@ static int ibmvnic_remove(struct vio_dev *dev)
> >       unsigned long flags;
> >
> >       spin_lock_irqsave(&adapter->state_lock, flags);
> > -     if (test_bit(0, &adapter->resetting)) {
> > -             spin_unlock_irqrestore(&adapter->state_lock, flags);
> > -             return -EBUSY;
> > -     }
> > -
> >       adapter->state = VNIC_REMOVING;
> >       spin_unlock_irqrestore(&adapter->state_lock, flags);

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net] ibmvnic: device remove has higher precedence over reset
@ 2021-01-21 18:46     ` Lijun Pan
  0 siblings, 0 replies; 10+ messages in thread
From: Lijun Pan @ 2021-01-21 18:46 UTC (permalink / raw)
  To: Dany Madden
  Cc: gregkh, julietk, netdev, Uwe Kleine-König, Jakub Kicinski,
	Lijun Pan, kernel, paulus, sukadev, linuxppc-dev, davem

On Thu, Jan 21, 2021 at 12:42 PM Dany Madden <drt@linux.ibm.com> wrote:
>
> On 2021-01-20 22:20, Lijun Pan wrote:
> > Returning -EBUSY in ibmvnic_remove() does not actually hold the
> > removal procedure since driver core doesn't care for the return
> > value (see __device_release_driver() in drivers/base/dd.c
> > calling dev->bus->remove()) though vio_bus_remove
> > (in arch/powerpc/platforms/pseries/vio.c) records the
> > return value and passes it on. [1]
> >
> > During the device removal precedure, we should not schedule
> > any new reset (ibmvnic_reset check for REMOVING and exit),
> > and should rely on the flush_work and flush_delayed_work
> > to complete the pending resets, specifically we need to
> > let __ibmvnic_reset() keep running while in REMOVING state since
> > flush_work and flush_delayed_work shall call __ibmvnic_reset finally.
> > So we skip the checking for REMOVING in __ibmvnic_reset.
> >
> > [1]
> > https://lore.kernel.org/linuxppc-dev/20210117101242.dpwayq6wdgfdzirl@pengutronix.de/T/#m48f5befd96bc9842ece2a3ad14f4c27747206a53
> > Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Fixes: 7d7195a026ba ("ibmvnic: Do not process device remove during
> > device reset")
> > Signed-off-by: Lijun Pan <ljp@linux.ibm.com>
> > ---
> > v1 versus RFC:
> >   1/ articulate why remove the REMOVING checking in __ibmvnic_reset
> >   and why keep the current checking for REMOVING in ibmvnic_reset.
> >   2/ The locking issue mentioned by Uwe are being addressed separately
> >      by       https://lists.openwall.net/netdev/2021/01/08/89
> >   3/ This patch does not have merge conflict with 2/
> >
> >  drivers/net/ethernet/ibm/ibmvnic.c | 8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> > b/drivers/net/ethernet/ibm/ibmvnic.c
> > index aed985e08e8a..11f28fd03057 100644
> > --- a/drivers/net/ethernet/ibm/ibmvnic.c
> > +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> > @@ -2235,8 +2235,7 @@ static void __ibmvnic_reset(struct work_struct
> > *work)
> >       while (rwi) {
> >               spin_lock_irqsave(&adapter->state_lock, flags);
> >
> > -             if (adapter->state == VNIC_REMOVING ||
> > -                 adapter->state == VNIC_REMOVED) {
> > +             if (adapter->state == VNIC_REMOVED) {
>
> If we do get here, we would crash because ibmvnic_remove() happened. It
> frees the adapter struct already.

Not exactly. viodev is gone; netdev is done; ibmvnic_adapter is still there.

Lijun
>
> >                       spin_unlock_irqrestore(&adapter->state_lock, flags);
> >                       kfree(rwi);
> >                       rc = EBUSY;
> > @@ -5372,11 +5371,6 @@ static int ibmvnic_remove(struct vio_dev *dev)
> >       unsigned long flags;
> >
> >       spin_lock_irqsave(&adapter->state_lock, flags);
> > -     if (test_bit(0, &adapter->resetting)) {
> > -             spin_unlock_irqrestore(&adapter->state_lock, flags);
> > -             return -EBUSY;
> > -     }
> > -
> >       adapter->state = VNIC_REMOVING;
> >       spin_unlock_irqrestore(&adapter->state_lock, flags);

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net] ibmvnic: device remove has higher precedence over reset
  2021-01-21 18:24   ` Dany Madden
@ 2021-01-21 19:48     ` Lijun Pan
  -1 siblings, 0 replies; 10+ messages in thread
From: Lijun Pan @ 2021-01-21 19:48 UTC (permalink / raw)
  To: Dany Madden
  Cc: Lijun Pan, netdev, linuxppc-dev, sukadev, mpe, julietk, benh,
	paulus, davem, Jakub Kicinski, gregkh, kernel,
	Uwe Kleine-König

> > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> > b/drivers/net/ethernet/ibm/ibmvnic.c
> > index aed985e08e8a..11f28fd03057 100644
> > --- a/drivers/net/ethernet/ibm/ibmvnic.c
> > +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> > @@ -2235,8 +2235,7 @@ static void __ibmvnic_reset(struct work_struct
> > *work)
> >       while (rwi) {
> >               spin_lock_irqsave(&adapter->state_lock, flags);
> >
> > -             if (adapter->state == VNIC_REMOVING ||
> > -                 adapter->state == VNIC_REMOVED) {
> > +             if (adapter->state == VNIC_REMOVED) {
>
> If we do get here, we would crash because ibmvnic_remove() happened. It
> frees the adapter struct already.

Not exactly. viodev is gone; netdev is gone; ibmvnic_adapter is still there.

Lijun

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net] ibmvnic: device remove has higher precedence over reset
@ 2021-01-21 19:48     ` Lijun Pan
  0 siblings, 0 replies; 10+ messages in thread
From: Lijun Pan @ 2021-01-21 19:48 UTC (permalink / raw)
  To: Dany Madden
  Cc: gregkh, julietk, netdev, Uwe Kleine-König, Jakub Kicinski,
	Lijun Pan, kernel, paulus, sukadev, linuxppc-dev, davem

> > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> > b/drivers/net/ethernet/ibm/ibmvnic.c
> > index aed985e08e8a..11f28fd03057 100644
> > --- a/drivers/net/ethernet/ibm/ibmvnic.c
> > +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> > @@ -2235,8 +2235,7 @@ static void __ibmvnic_reset(struct work_struct
> > *work)
> >       while (rwi) {
> >               spin_lock_irqsave(&adapter->state_lock, flags);
> >
> > -             if (adapter->state == VNIC_REMOVING ||
> > -                 adapter->state == VNIC_REMOVED) {
> > +             if (adapter->state == VNIC_REMOVED) {
>
> If we do get here, we would crash because ibmvnic_remove() happened. It
> frees the adapter struct already.

Not exactly. viodev is gone; netdev is gone; ibmvnic_adapter is still there.

Lijun

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net] ibmvnic: device remove has higher precedence over reset
  2021-01-21 19:48     ` Lijun Pan
@ 2021-01-21 22:38       ` Sukadev Bhattiprolu
  -1 siblings, 0 replies; 10+ messages in thread
From: Sukadev Bhattiprolu @ 2021-01-21 22:38 UTC (permalink / raw)
  To: Lijun Pan
  Cc: Dany Madden, Lijun Pan, netdev, linuxppc-dev, mpe, julietk, benh,
	paulus, davem, Jakub Kicinski, gregkh, kernel,
	Uwe Kleine-König

Lijun Pan [lijunp213@gmail.com] wrote:
> > > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> > > b/drivers/net/ethernet/ibm/ibmvnic.c
> > > index aed985e08e8a..11f28fd03057 100644
> > > --- a/drivers/net/ethernet/ibm/ibmvnic.c
> > > +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> > > @@ -2235,8 +2235,7 @@ static void __ibmvnic_reset(struct work_struct
> > > *work)
> > >       while (rwi) {
> > >               spin_lock_irqsave(&adapter->state_lock, flags);
> > >
> > > -             if (adapter->state == VNIC_REMOVING ||
> > > -                 adapter->state == VNIC_REMOVED) {
> > > +             if (adapter->state == VNIC_REMOVED) {

If the adapter is in REMOVING state, there is no point going
through the reset process. We could just bail out here. We
should also drain any other resets in the queue (something
my other patch set was addressing).

Sukadev

> >
> > If we do get here, we would crash because ibmvnic_remove() happened. It
> > frees the adapter struct already.
> 
> Not exactly. viodev is gone; netdev is gone; ibmvnic_adapter is still there.
> 
> Lijun


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net] ibmvnic: device remove has higher precedence over reset
@ 2021-01-21 22:38       ` Sukadev Bhattiprolu
  0 siblings, 0 replies; 10+ messages in thread
From: Sukadev Bhattiprolu @ 2021-01-21 22:38 UTC (permalink / raw)
  To: Lijun Pan
  Cc: gregkh, julietk, netdev, Uwe Kleine-König, Jakub Kicinski,
	Lijun Pan, kernel, Dany Madden, paulus, linuxppc-dev, davem

Lijun Pan [lijunp213@gmail.com] wrote:
> > > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> > > b/drivers/net/ethernet/ibm/ibmvnic.c
> > > index aed985e08e8a..11f28fd03057 100644
> > > --- a/drivers/net/ethernet/ibm/ibmvnic.c
> > > +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> > > @@ -2235,8 +2235,7 @@ static void __ibmvnic_reset(struct work_struct
> > > *work)
> > >       while (rwi) {
> > >               spin_lock_irqsave(&adapter->state_lock, flags);
> > >
> > > -             if (adapter->state == VNIC_REMOVING ||
> > > -                 adapter->state == VNIC_REMOVED) {
> > > +             if (adapter->state == VNIC_REMOVED) {

If the adapter is in REMOVING state, there is no point going
through the reset process. We could just bail out here. We
should also drain any other resets in the queue (something
my other patch set was addressing).

Sukadev

> >
> > If we do get here, we would crash because ibmvnic_remove() happened. It
> > frees the adapter struct already.
> 
> Not exactly. viodev is gone; netdev is gone; ibmvnic_adapter is still there.
> 
> Lijun


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-01-21 22:43 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-21  6:20 [PATCH net] ibmvnic: device remove has higher precedence over reset Lijun Pan
2021-01-21  6:20 ` Lijun Pan
2021-01-21 18:24 ` Dany Madden
2021-01-21 18:24   ` Dany Madden
2021-01-21 18:46   ` Lijun Pan
2021-01-21 18:46     ` Lijun Pan
2021-01-21 19:48   ` Lijun Pan
2021-01-21 19:48     ` Lijun Pan
2021-01-21 22:38     ` Sukadev Bhattiprolu
2021-01-21 22:38       ` Sukadev Bhattiprolu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.