diff mbox series

[5/7] ethdev: do nothing if all-multicast mode is applied again

Message ID 1568031190-16510-6-git-send-email-arybchenko@solarflare.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: change allmulticast controls to return status |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Andrew Rybchenko Sept. 9, 2019, 12:13 p.m. UTC 
  Since driver callbacks return status code now, there is no necessity
to enable or disable all-multicast mode once again if it is already
successfully enabled or disabled.

Configuration restore at startup tries to ensure that configured
all-multicast mode is applied and start will return error if it fails.

Also it avoids theoretical cases when already configured all-multicast
mode is applied once again and fails. In this cases it is unclear
which value should be reported on get (configured or opposite).

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_ethdev/rte_ethdev.c | 40 ++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 16 deletions(-)

Comments

Ferruh Yigit Sept. 24, 2019, 8:36 a.m. UTC | #1 
On 9/9/2019 1:13 PM, Andrew Rybchenko wrote:
> Since driver callbacks return status code now, there is no necessity
> to enable or disable all-multicast mode once again if it is already
> successfully enabled or disabled.
> 
> Configuration restore at startup tries to ensure that configured
> all-multicast mode is applied and start will return error if it fails.
> 
> Also it avoids theoretical cases when already configured all-multicast
> mode is applied once again and fails. In this cases it is unclear
> which value should be reported on get (configured or opposite).
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>  lib/librte_ethdev/rte_ethdev.c | 40 ++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 8115226c91..e1921e8225 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1416,16 +1416,22 @@ rte_eth_dev_config_restore(struct rte_eth_dev *dev,
>  	}
>  
>  	/* replay all multicast configuration */
> -	if (rte_eth_allmulticast_get(port_id) == 1) {
> -		ret = rte_eth_allmulticast_enable(port_id);
> +	/*
> +	 * use callbacks directly since we don't need port_id check and
> +	 * would like to bypass the same value set
> +	 */
> +	if (rte_eth_allmulticast_get(port_id) == 1 &&
> +	    *dev->dev_ops->allmulticast_enable != NULL) {
> +		ret = (*dev->dev_ops->allmulticast_enable)(dev);

I am for using the API here, it is more abstract instead of adding the dev_ops
null checks etc. Will there be any downside to use the API?

<...>

> @@ -1962,16 +1968,17 @@ int
>  rte_eth_allmulticast_enable(uint16_t port_id)
>  {
>  	struct rte_eth_dev *dev;
> -	uint8_t old_allmulticast;
> -	int diag;
> +	int diag = 0;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  	dev = &rte_eth_devices[port_id];
>  
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->allmulticast_enable, -ENOTSUP);
> -	old_allmulticast = dev->data->all_multicast;
> -	diag = (*dev->dev_ops->allmulticast_enable)(dev);
> -	dev->data->all_multicast = (diag == 0) ? 1 : old_allmulticast;
> +
> +	if (dev->data->all_multicast == 0) {

What about adding this check even before 'allmulticast_enable' check, so if the
multicast is already enabled why bother having dev_ops or not:

if (dev->data->all_multicast == 1)
	return eth_err(port_id, diag);

> +		diag = (*dev->dev_ops->allmulticast_enable)(dev);
> +		dev->data->all_multicast = (diag == 0) ? 1 : 0;
> +	}
>  
>  	return eth_err(port_id, diag);
>  }
> @@ -1980,18 +1987,19 @@ int
>  rte_eth_allmulticast_disable(uint16_t port_id)
>  {
>  	struct rte_eth_dev *dev;
> -	uint8_t old_allmulticast;
> -	int diag;
> +	int diag = 0;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  	dev = &rte_eth_devices[port_id];
>  
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->allmulticast_disable, -ENOTSUP);
> -	old_allmulticast = dev->data->all_multicast;
> -	dev->data->all_multicast = 0;
> -	diag = (*dev->dev_ops->allmulticast_disable)(dev);
> -	if (diag != 0)
> -		dev->data->all_multicast = old_allmulticast;
> +
> +	if (dev->data->all_multicast == 1) {

Same comment with above, can we move this check above..

> +		dev->data->all_multicast = 0;
> +		diag = (*dev->dev_ops->allmulticast_disable)(dev);
> +		if (diag != 0)
> +			dev->data->all_multicast = 1;
> +	}
>  
>  	return eth_err(port_id, diag);
>  }
>
Andrew Rybchenko Sept. 24, 2019, 8:54 a.m. UTC | #2 
On 9/24/19 11:36 AM, Ferruh Yigit wrote:
> On 9/9/2019 1:13 PM, Andrew Rybchenko wrote:
>> Since driver callbacks return status code now, there is no necessity
>> to enable or disable all-multicast mode once again if it is already
>> successfully enabled or disabled.
>>
>> Configuration restore at startup tries to ensure that configured
>> all-multicast mode is applied and start will return error if it fails.
>>
>> Also it avoids theoretical cases when already configured all-multicast
>> mode is applied once again and fails. In this cases it is unclear
>> which value should be reported on get (configured or opposite).
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>   lib/librte_ethdev/rte_ethdev.c | 40 ++++++++++++++++++++--------------
>>   1 file changed, 24 insertions(+), 16 deletions(-)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index 8115226c91..e1921e8225 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -1416,16 +1416,22 @@ rte_eth_dev_config_restore(struct rte_eth_dev *dev,
>>   	}
>>   
>>   	/* replay all multicast configuration */
>> -	if (rte_eth_allmulticast_get(port_id) == 1) {
>> -		ret = rte_eth_allmulticast_enable(port_id);
>> +	/*
>> +	 * use callbacks directly since we don't need port_id check and
>> +	 * would like to bypass the same value set
>> +	 */
>> +	if (rte_eth_allmulticast_get(port_id) == 1 &&
>> +	    *dev->dev_ops->allmulticast_enable != NULL) {
>> +		ret = (*dev->dev_ops->allmulticast_enable)(dev);
> I am for using the API here, it is more abstract instead of adding the dev_ops
> null checks etc. Will there be any downside to use the API?

API does not call operation if value matches and it will exactly match here
for sure since we just get it and applying once again.
Can't say that I like usage callback directly here, but it looks acceptable
for me. I've tried to clarify why it is done this way in the comment above.

> <...>
>
>> @@ -1962,16 +1968,17 @@ int
>>   rte_eth_allmulticast_enable(uint16_t port_id)
>>   {
>>   	struct rte_eth_dev *dev;
>> -	uint8_t old_allmulticast;
>> -	int diag;
>> +	int diag = 0;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->allmulticast_enable, -ENOTSUP);
>> -	old_allmulticast = dev->data->all_multicast;
>> -	diag = (*dev->dev_ops->allmulticast_enable)(dev);
>> -	dev->data->all_multicast = (diag == 0) ? 1 : old_allmulticast;
>> +
>> +	if (dev->data->all_multicast == 0) {
> What about adding this check even before 'allmulticast_enable' check, so if the
> multicast is already enabled why bother having dev_ops or not:
>
> if (dev->data->all_multicast == 1)
> 	return eth_err(port_id, diag);

Yes, it is a good idea. If so, similar fix up will be required for 
promiscuous mode.

>> +		diag = (*dev->dev_ops->allmulticast_enable)(dev);
>> +		dev->data->all_multicast = (diag == 0) ? 1 : 0;
>> +	}
>>   
>>   	return eth_err(port_id, diag);
>>   }
>> @@ -1980,18 +1987,19 @@ int
>>   rte_eth_allmulticast_disable(uint16_t port_id)
>>   {
>>   	struct rte_eth_dev *dev;
>> -	uint8_t old_allmulticast;
>> -	int diag;
>> +	int diag = 0;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->allmulticast_disable, -ENOTSUP);
>> -	old_allmulticast = dev->data->all_multicast;
>> -	dev->data->all_multicast = 0;
>> -	diag = (*dev->dev_ops->allmulticast_disable)(dev);
>> -	if (diag != 0)
>> -		dev->data->all_multicast = old_allmulticast;
>> +
>> +	if (dev->data->all_multicast == 1) {
> Same comment with above, can we move this check above..

Yes, will fix in the next version as well. Thanks for review.

>> +		dev->data->all_multicast = 0;
>> +		diag = (*dev->dev_ops->allmulticast_disable)(dev);
>> +		if (diag != 0)
>> +			dev->data->all_multicast = 1;
>> +	}
>>   
>>   	return eth_err(port_id, diag);
>>   }
>>
Ferruh Yigit Sept. 24, 2019, 11:03 a.m. UTC | #3 
On 9/24/2019 9:54 AM, Andrew Rybchenko wrote:
> On 9/24/19 11:36 AM, Ferruh Yigit wrote:
>> On 9/9/2019 1:13 PM, Andrew Rybchenko wrote:
>>> Since driver callbacks return status code now, there is no necessity
>>> to enable or disable all-multicast mode once again if it is already
>>> successfully enabled or disabled.
>>>
>>> Configuration restore at startup tries to ensure that configured
>>> all-multicast mode is applied and start will return error if it fails.
>>>
>>> Also it avoids theoretical cases when already configured all-multicast
>>> mode is applied once again and fails. In this cases it is unclear
>>> which value should be reported on get (configured or opposite).
>>>
>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> ---
>>>   lib/librte_ethdev/rte_ethdev.c | 40 ++++++++++++++++++++--------------
>>>   1 file changed, 24 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>> index 8115226c91..e1921e8225 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -1416,16 +1416,22 @@ rte_eth_dev_config_restore(struct rte_eth_dev *dev,
>>>   	}
>>>   
>>>   	/* replay all multicast configuration */
>>> -	if (rte_eth_allmulticast_get(port_id) == 1) {
>>> -		ret = rte_eth_allmulticast_enable(port_id);
>>> +	/*
>>> +	 * use callbacks directly since we don't need port_id check and
>>> +	 * would like to bypass the same value set
>>> +	 */
>>> +	if (rte_eth_allmulticast_get(port_id) == 1 &&
>>> +	    *dev->dev_ops->allmulticast_enable != NULL) {
>>> +		ret = (*dev->dev_ops->allmulticast_enable)(dev);
>> I am for using the API here, it is more abstract instead of adding the dev_ops
>> null checks etc. Will there be any downside to use the API?
> 
> API does not call operation if value matches and it will exactly match here
> for sure since we just get it and applying once again.

Ah, right, we need it here as you explained. Thx.

> Can't say that I like usage callback directly here, but it looks acceptable
> for me. I've tried to clarify why it is done this way in the comment above.
> 
>> <...>
>>
>>> @@ -1962,16 +1968,17 @@ int
>>>   rte_eth_allmulticast_enable(uint16_t port_id)
>>>   {
>>>   	struct rte_eth_dev *dev;
>>> -	uint8_t old_allmulticast;
>>> -	int diag;
>>> +	int diag = 0;
>>>   
>>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>   	dev = &rte_eth_devices[port_id];
>>>   
>>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->allmulticast_enable, -ENOTSUP);
>>> -	old_allmulticast = dev->data->all_multicast;
>>> -	diag = (*dev->dev_ops->allmulticast_enable)(dev);
>>> -	dev->data->all_multicast = (diag == 0) ? 1 : old_allmulticast;
>>> +
>>> +	if (dev->data->all_multicast == 0) {
>> What about adding this check even before 'allmulticast_enable' check, so if the
>> multicast is already enabled why bother having dev_ops or not:
>>
>> if (dev->data->all_multicast == 1)
>> 	return eth_err(port_id, diag);
> 
> Yes, it is a good idea. If so, similar fix up will be required for 
> promiscuous mode.
> 
>>> +		diag = (*dev->dev_ops->allmulticast_enable)(dev);
>>> +		dev->data->all_multicast = (diag == 0) ? 1 : 0;
>>> +	}
>>>   
>>>   	return eth_err(port_id, diag);
>>>   }
>>> @@ -1980,18 +1987,19 @@ int
>>>   rte_eth_allmulticast_disable(uint16_t port_id)
>>>   {
>>>   	struct rte_eth_dev *dev;
>>> -	uint8_t old_allmulticast;
>>> -	int diag;
>>> +	int diag = 0;
>>>   
>>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>   	dev = &rte_eth_devices[port_id];
>>>   
>>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->allmulticast_disable, -ENOTSUP);
>>> -	old_allmulticast = dev->data->all_multicast;
>>> -	dev->data->all_multicast = 0;
>>> -	diag = (*dev->dev_ops->allmulticast_disable)(dev);
>>> -	if (diag != 0)
>>> -		dev->data->all_multicast = old_allmulticast;
>>> +
>>> +	if (dev->data->all_multicast == 1) {
>> Same comment with above, can we move this check above..
> 
> Yes, will fix in the next version as well. Thanks for review.
> 
>>> +		dev->data->all_multicast = 0;
>>> +		diag = (*dev->dev_ops->allmulticast_disable)(dev);
>>> +		if (diag != 0)
>>> +			dev->data->all_multicast = 1;
>>> +	}
>>>   
>>>   	return eth_err(port_id, diag);
>>>   }
>>>
> 
>
Andrew Rybchenko Sept. 24, 2019, 11:50 a.m. UTC | #4 
On 9/24/19 2:03 PM, Ferruh Yigit wrote:
> On 9/24/2019 9:54 AM, Andrew Rybchenko wrote:
>> On 9/24/19 11:36 AM, Ferruh Yigit wrote:
>>> On 9/9/2019 1:13 PM, Andrew Rybchenko wrote:
>>>> Since driver callbacks return status code now, there is no necessity
>>>> to enable or disable all-multicast mode once again if it is already
>>>> successfully enabled or disabled.
>>>>
>>>> Configuration restore at startup tries to ensure that configured
>>>> all-multicast mode is applied and start will return error if it fails.
>>>>
>>>> Also it avoids theoretical cases when already configured all-multicast
>>>> mode is applied once again and fails. In this cases it is unclear
>>>> which value should be reported on get (configured or opposite).
>>>>
>>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> ---
>>>>    lib/librte_ethdev/rte_ethdev.c | 40 ++++++++++++++++++++--------------
>>>>    1 file changed, 24 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>>> index 8115226c91..e1921e8225 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>>> @@ -1416,16 +1416,22 @@ rte_eth_dev_config_restore(struct rte_eth_dev *dev,
>>>>    	}
>>>>    
>>>>    	/* replay all multicast configuration */
>>>> -	if (rte_eth_allmulticast_get(port_id) == 1) {
>>>> -		ret = rte_eth_allmulticast_enable(port_id);
>>>> +	/*
>>>> +	 * use callbacks directly since we don't need port_id check and
>>>> +	 * would like to bypass the same value set
>>>> +	 */
>>>> +	if (rte_eth_allmulticast_get(port_id) == 1 &&
>>>> +	    *dev->dev_ops->allmulticast_enable != NULL) {
>>>> +		ret = (*dev->dev_ops->allmulticast_enable)(dev);
>>> I am for using the API here, it is more abstract instead of adding the dev_ops
>>> null checks etc. Will there be any downside to use the API?
>> API does not call operation if value matches and it will exactly match here
>> for sure since we just get it and applying once again.
> Ah, right, we need it here as you explained. Thx.

In fact, I think eth_err() is required to handle callback return value.
I'll add it in v2.

>> Can't say that I like usage callback directly here, but it looks acceptable
>> for me. I've tried to clarify why it is done this way in the comment above.
>>
>>> <...>
>>>
>>>> @@ -1962,16 +1968,17 @@ int
>>>>    rte_eth_allmulticast_enable(uint16_t port_id)
>>>>    {
>>>>    	struct rte_eth_dev *dev;
>>>> -	uint8_t old_allmulticast;
>>>> -	int diag;
>>>> +	int diag = 0;
>>>>    
>>>>    	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>>    	dev = &rte_eth_devices[port_id];
>>>>    
>>>>    	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->allmulticast_enable, -ENOTSUP);
>>>> -	old_allmulticast = dev->data->all_multicast;
>>>> -	diag = (*dev->dev_ops->allmulticast_enable)(dev);
>>>> -	dev->data->all_multicast = (diag == 0) ? 1 : old_allmulticast;
>>>> +
>>>> +	if (dev->data->all_multicast == 0) {
>>> What about adding this check even before 'allmulticast_enable' check, so if the
>>> multicast is already enabled why bother having dev_ops or not:
>>>
>>> if (dev->data->all_multicast == 1)
>>> 	return eth_err(port_id, diag);
>> Yes, it is a good idea. If so, similar fix up will be required for
>> promiscuous mode.

I'd prefer to return 0 directly.

>>>> +		diag = (*dev->dev_ops->allmulticast_enable)(dev);
>>>> +		dev->data->all_multicast = (diag == 0) ? 1 : 0;
>>>> +	}
>>>>    
>>>>    	return eth_err(port_id, diag);
>>>>    }
>>>> @@ -1980,18 +1987,19 @@ int
>>>>    rte_eth_allmulticast_disable(uint16_t port_id)
>>>>    {
>>>>    	struct rte_eth_dev *dev;
>>>> -	uint8_t old_allmulticast;
>>>> -	int diag;
>>>> +	int diag = 0;
>>>>    
>>>>    	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>>    	dev = &rte_eth_devices[port_id];
>>>>    
>>>>    	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->allmulticast_disable, -ENOTSUP);
>>>> -	old_allmulticast = dev->data->all_multicast;
>>>> -	dev->data->all_multicast = 0;
>>>> -	diag = (*dev->dev_ops->allmulticast_disable)(dev);
>>>> -	if (diag != 0)
>>>> -		dev->data->all_multicast = old_allmulticast;
>>>> +
>>>> +	if (dev->data->all_multicast == 1) {
>>> Same comment with above, can we move this check above..
>> Yes, will fix in the next version as well. Thanks for review.
>>
>>>> +		dev->data->all_multicast = 0;
>>>> +		diag = (*dev->dev_ops->allmulticast_disable)(dev);
>>>> +		if (diag != 0)
>>>> +			dev->data->all_multicast = 1;
>>>> +	}
>>>>    
>>>>    	return eth_err(port_id, diag);
>>>>    }
>>>>
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 8115226c91..e1921e8225 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1416,16 +1416,22 @@  rte_eth_dev_config_restore(struct rte_eth_dev *dev,
 	}
 
 	/* replay all multicast configuration */
-	if (rte_eth_allmulticast_get(port_id) == 1) {
-		ret = rte_eth_allmulticast_enable(port_id);
+	/*
+	 * use callbacks directly since we don't need port_id check and
+	 * would like to bypass the same value set
+	 */
+	if (rte_eth_allmulticast_get(port_id) == 1 &&
+	    *dev->dev_ops->allmulticast_enable != NULL) {
+		ret = (*dev->dev_ops->allmulticast_enable)(dev);
 		if (ret != 0 && ret != -ENOTSUP) {
 			RTE_ETHDEV_LOG(ERR,
 				"Failed to enable allmulticast mode for device (port %u): %s\n",
 				port_id, rte_strerror(-ret));
 			return ret;
 		}
-	} else if (rte_eth_allmulticast_get(port_id) == 0) {
-		ret = rte_eth_allmulticast_disable(port_id);
+	} else if (rte_eth_allmulticast_get(port_id) == 0 &&
+		   *dev->dev_ops->allmulticast_disable != NULL) {
+		ret = (*dev->dev_ops->allmulticast_disable)(dev);
 		if (ret != 0 && ret != -ENOTSUP) {
 			RTE_ETHDEV_LOG(ERR,
 				"Failed to disable allmulticast mode for device (port %u): %s\n",
@@ -1962,16 +1968,17 @@  int
 rte_eth_allmulticast_enable(uint16_t port_id)
 {
 	struct rte_eth_dev *dev;
-	uint8_t old_allmulticast;
-	int diag;
+	int diag = 0;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->allmulticast_enable, -ENOTSUP);
-	old_allmulticast = dev->data->all_multicast;
-	diag = (*dev->dev_ops->allmulticast_enable)(dev);
-	dev->data->all_multicast = (diag == 0) ? 1 : old_allmulticast;
+
+	if (dev->data->all_multicast == 0) {
+		diag = (*dev->dev_ops->allmulticast_enable)(dev);
+		dev->data->all_multicast = (diag == 0) ? 1 : 0;
+	}
 
 	return eth_err(port_id, diag);
 }
@@ -1980,18 +1987,19 @@  int
 rte_eth_allmulticast_disable(uint16_t port_id)
 {
 	struct rte_eth_dev *dev;
-	uint8_t old_allmulticast;
-	int diag;
+	int diag = 0;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->allmulticast_disable, -ENOTSUP);
-	old_allmulticast = dev->data->all_multicast;
-	dev->data->all_multicast = 0;
-	diag = (*dev->dev_ops->allmulticast_disable)(dev);
-	if (diag != 0)
-		dev->data->all_multicast = old_allmulticast;
+
+	if (dev->data->all_multicast == 1) {
+		dev->data->all_multicast = 0;
+		diag = (*dev->dev_ops->allmulticast_disable)(dev);
+		if (diag != 0)
+			dev->data->all_multicast = 1;
+	}
 
 	return eth_err(port_id, diag);
 }