[PATCH net-next v2] ibmvnic: Bail from ibmvnic_open if driver is already open

[PATCH net-next v2] ibmvnic: Bail from ibmvnic_open if driver is already open

[John Allen]

If the driver is already in the "open" state, don't attempt the procedure
for opening the driver.

Signed-off-by: John Allen <jallen@linux.vnet.ibm.com>
---
v2: Unlock reset_lock mutex before returning.

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 7be4b06..9a5e8ac 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1057,6 +1057,11 @@ static int ibmvnic_open(struct net_device *netdev)

 	mutex_lock(&adapter->reset_lock);

+	if (adapter->state == VNIC_OPEN) {
+		mutex_unlock(&adapter->reset_lock);
+		return 0;
+	}
+
 	if (adapter->state != VNIC_CLOSED) {
 		rc = ibmvnic_login(netdev);
 		if (rc) {

Re: [PATCH net-next v2] ibmvnic: Bail from ibmvnic_open if driver is already open

[Andrew Lunn]

On Mon, Mar 12, 2018 at 02:19:52PM -0500, John Allen wrote:
> If the driver is already in the "open" state, don't attempt the procedure
> for opening the driver.
> 
> Signed-off-by: John Allen <jallen@linux.vnet.ibm.com>
> ---
> v2: Unlock reset_lock mutex before returning.
> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index 7be4b06..9a5e8ac 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -1057,6 +1057,11 @@ static int ibmvnic_open(struct net_device *netdev)
> 
>  	mutex_lock(&adapter->reset_lock);
> 
> +	if (adapter->state == VNIC_OPEN) {
> +		mutex_unlock(&adapter->reset_lock);
> +		return 0;
> +	}

Hi John

This looks better.

But how did you get ibmvnic_open() to be called twice? Is the actual
issue that __ibmvnic_close() does not kill off the
adapter->ibmvnic_reset workqueue, so that a reset can happen after
close() has been called?

	   Andrew

Re: [PATCH net-next v2] ibmvnic: Bail from ibmvnic_open if driver is already open

[John Allen]

On 03/12/2018 02:33 PM, Andrew Lunn wrote:
> On Mon, Mar 12, 2018 at 02:19:52PM -0500, John Allen wrote:
>> If the driver is already in the "open" state, don't attempt the procedure
>> for opening the driver.
>>
>> Signed-off-by: John Allen <jallen@linux.vnet.ibm.com>
>> ---
>> v2: Unlock reset_lock mutex before returning.
>>
>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
>> index 7be4b06..9a5e8ac 100644
>> --- a/drivers/net/ethernet/ibm/ibmvnic.c
>> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
>> @@ -1057,6 +1057,11 @@ static int ibmvnic_open(struct net_device *netdev)
>>
>>  	mutex_lock(&adapter->reset_lock);
>>
>> +	if (adapter->state == VNIC_OPEN) {
>> +		mutex_unlock(&adapter->reset_lock);
>> +		return 0;
>> +	}
> 
> Hi John
> 
> This looks better.
> 
> But how did you get ibmvnic_open() to be called twice? Is the actual
> issue that __ibmvnic_close() does not kill off the
> adapter->ibmvnic_reset workqueue, so that a reset can happen after
> close() has been called?

The problem here is that our routine to change the mtu does a full reset on
the driver meaning that in the process we go from effectively "open" to
"closed" to "open" again.

Consider the scenario where we change the mtu by running "ifdown <interface>",
editing the ifcfg file with the new mtu, and finally running "ifup <interface".
In this case, we call ibmvnic_close from the ifdown and as a result of the ifup,
we do the reset for the mtu change (which puts us back in the "open" state) and
call ibmvnic_open. After the reset, we are already in the "open" state and the
following call to open is redundant. The check in ibmvnic_open to see if we're
in the open state is really just insurance to guard against any case like this
where we're in the open state from the driver perspective and something decides
to trigger the drivers open routine.

-John

> 
> 	   Andrew
> 
> 

Re: [PATCH net-next v2] ibmvnic: Bail from ibmvnic_open if driver is already open

[Andrew Lunn]

> The problem here is that our routine to change the mtu does a full reset on
> the driver meaning that in the process we go from effectively "open" to
> "closed" to "open" again.
> 
> Consider the scenario where we change the mtu by running "ifdown <interface>",
> editing the ifcfg file with the new mtu, and finally running "ifup <interface".
> In this case, we call ibmvnic_close from the ifdown and as a result of the ifup,
> we do the reset for the mtu change (which puts us back in the "open" state) and
> call ibmvnic_open. After the reset, we are already in the "open" state and the
> following call to open is redundant.

Hi John

So you are saying "ip link set mtu 4242 eth1" on a down interface will
put it up. That i would say is broken. You should be fixing this,
rather than papering over the cracks.

      Andrew

Re: [PATCH net-next v2] ibmvnic: Bail from ibmvnic_open if driver is already open

[John Allen]

On 03/12/2018 03:10 PM, Andrew Lunn wrote:
>> The problem here is that our routine to change the mtu does a full reset on
>> the driver meaning that in the process we go from effectively "open" to
>> "closed" to "open" again.
>>
>> Consider the scenario where we change the mtu by running "ifdown <interface>",
>> editing the ifcfg file with the new mtu, and finally running "ifup <interface".
>> In this case, we call ibmvnic_close from the ifdown and as a result of the ifup,
>> we do the reset for the mtu change (which puts us back in the "open" state) and
>> call ibmvnic_open. After the reset, we are already in the "open" state and the
>> following call to open is redundant.
> 
> Hi John
> 
> So you are saying "ip link set mtu 4242 eth1" on a down interface will
> put it up. That i would say is broken. You should be fixing this,
> rather than papering over the cracks.

That's a good point. I'll work on a fix to address that.

-John

> 
>       Andrew
> 
>