Re: [PATCH net] ibmvnic: device remove has higher precedence over reset

2021-01-21 Thread Sukadev Bhattiprolu 
Lijun Pan [lijunp...@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(>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

Re: [PATCH net] ibmvnic: device remove has higher precedence over reset

2021-01-21 Thread Lijun Pan 
> > 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(>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

Re: [PATCH net] ibmvnic: device remove has higher precedence over reset

2021-01-21 Thread Lijun Pan 
On Thu, Jan 21, 2021 at 12:42 PM Dany Madden  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.dpwayq6wdgfdz...@pengutronix.de/T/#m48f5befd96bc9842ece2a3ad14f4c27747206a53
> > Reported-by: Uwe Kleine-König 
> > Fixes: 7d7195a026ba ("ibmvnic: Do not process device remove during
> > device reset")
> > Signed-off-by: Lijun Pan 
> > ---
> > 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(>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(>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(>state_lock, flags);
> > - if (test_bit(0, >resetting)) {
> > - spin_unlock_irqrestore(>state_lock, flags);
> > - return -EBUSY;
> > - }
> > -
> >   adapter->state = VNIC_REMOVING;
> >   spin_unlock_irqrestore(>state_lock, flags);

Re: [PATCH net] ibmvnic: device remove has higher precedence over reset

2021-01-21 Thread Dany Madden 

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.dpwayq6wdgfdz...@pengutronix.de/T/#m48f5befd96bc9842ece2a3ad14f4c27747206a53
Reported-by: Uwe Kleine-König 
Fixes: 7d7195a026ba ("ibmvnic: Do not process device remove during
device reset")
Signed-off-by: Lijun Pan 
---
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(>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(>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(>state_lock, flags);
-   if (test_bit(0, >resetting)) {
-   spin_unlock_irqrestore(>state_lock, flags);
-   return -EBUSY;
-   }
-
adapter->state = VNIC_REMOVING;
spin_unlock_irqrestore(>state_lock, flags);