| Message ID | 20210129034711.518250-2-sukadev@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net,1/2] ibmvnic: fix a race between open and reset | expand |
On Thu, Jan 28, 2021 at 10:51 PM Sukadev Bhattiprolu <sukadev@linux.ibm.com> wrote: > > If two or more instances of 'ip link set' commands race and first one > already brings the interface up (or down), the subsequent instances > can simply return without redoing the up/down operation. > > Fixes: ed651a10875f ("ibmvnic: Updated reset handling") > Reported-by: Abdul Haleem <abdhalee@in.ibm.com> > Tested-by: Abdul Haleem <abdhalee@in.ibm.com> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com> Isn't this handled in the rtnetlink core based on IFF_UP? if ((old_flags ^ flags) & IFF_UP) { if (old_flags & IFF_UP) __dev_close(dev); else ret = __dev_open(dev, extack); }
Willem de Bruijn [willemdebruijn.kernel@gmail.com] wrote: > On Thu, Jan 28, 2021 at 10:51 PM Sukadev Bhattiprolu > <sukadev@linux.ibm.com> wrote: > > > > If two or more instances of 'ip link set' commands race and first one > > already brings the interface up (or down), the subsequent instances > > can simply return without redoing the up/down operation. > > > > Fixes: ed651a10875f ("ibmvnic: Updated reset handling") > > Reported-by: Abdul Haleem <abdhalee@in.ibm.com> > > Tested-by: Abdul Haleem <abdhalee@in.ibm.com> > > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com> > > Isn't this handled in the rtnetlink core based on IFF_UP? > > if ((old_flags ^ flags) & IFF_UP) { > if (old_flags & IFF_UP) > __dev_close(dev); > else > ret = __dev_open(dev, extack); > } Good question. During our testing we hit the "adapter already open" debug message a few times. Without this change, the core layer and dirver disagree on the state and the adapter becomes unsuable. I will debug this at the core layer also later this week. We are working on rewriting parts of driver surrounding locking/adapter state and not sure if that will reveal any other cause for this behavior. Sukadev
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c index cb7ddfefb03e..84b772921f35 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.c +++ b/drivers/net/ethernet/ibm/ibmvnic.c @@ -1219,6 +1219,13 @@ static int ibmvnic_open(struct net_device *netdev) goto out; } + /* If adapter is already open, we don't have to do anything. */ + if (adapter->state == VNIC_OPEN) { + netdev_dbg(netdev, "[S:%d] adapter already open\n", + adapter->state); + return 0; + } + if (adapter->state != VNIC_CLOSED) { rc = ibmvnic_login(netdev); if (rc) @@ -1392,6 +1399,12 @@ static int ibmvnic_close(struct net_device *netdev) return 0; } + /* If adapter is already closed, we don't have to do anything. */ + if (adapter->state == VNIC_CLOSED) { + netdev_dbg(netdev, "[S:%d] adapter already closed\n", + adapter->state); + return 0; + } rc = __ibmvnic_close(netdev); ibmvnic_cleanup(netdev);
If two or more instances of 'ip link set' commands race and first one already brings the interface up (or down), the subsequent instances can simply return without redoing the up/down operation. Fixes: ed651a10875f ("ibmvnic: Updated reset handling") Reported-by: Abdul Haleem <abdhalee@in.ibm.com> Tested-by: Abdul Haleem <abdhalee@in.ibm.com> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com> --- drivers/net/ethernet/ibm/ibmvnic.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)