[PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI

[PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI

[Wang Chen]

IFF_PROMISC should be set before IFF_ALLMULTI.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
 net/8021q/vlan_dev.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 5d055c2..14742e3 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -547,10 +547,14 @@ static int vlan_dev_open(struct net_device *dev)
 	}
 	memcpy(vlan->real_dev_addr, real_dev->dev_addr, ETH_ALEN);
 
-	if (dev->flags & IFF_ALLMULTI)
-		dev_set_allmulti(real_dev, 1);
+	/* NOTE: order of synchronization of IFF_PROMISC and IFF_ALLMULTI
+	   is important. Some (broken) drivers set IFF_PROMISC, when
+	   IFF_ALLMULTI is requested not asking us and not reporting.
+	 */
 	if (dev->flags & IFF_PROMISC)
 		dev_set_promiscuity(real_dev, 1);
+	if (dev->flags & IFF_ALLMULTI)
+		dev_set_allmulti(real_dev, 1);
 
 	return 0;
 }
@@ -561,10 +565,14 @@ static int vlan_dev_stop(struct net_device *dev)
 
 	dev_mc_unsync(real_dev, dev);
 	dev_unicast_unsync(real_dev, dev);
-	if (dev->flags & IFF_ALLMULTI)
-		dev_set_allmulti(real_dev, -1);
+	/* NOTE: order of synchronization of IFF_PROMISC and IFF_ALLMULTI
+	   is important. Some (broken) drivers set IFF_PROMISC, when
+	   IFF_ALLMULTI is requested not asking us and not reporting.
+	 */
 	if (dev->flags & IFF_PROMISC)
 		dev_set_promiscuity(real_dev, -1);
+	if (dev->flags & IFF_ALLMULTI)
+		dev_set_allmulti(real_dev, -1);
 
 	if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr))
 		dev_unicast_delete(real_dev, dev->dev_addr, dev->addr_len);
@@ -626,10 +634,15 @@ static void vlan_dev_change_rx_flags(struct net_device *dev, int change)
 {
 	struct net_device *real_dev = vlan_dev_info(dev)->real_dev;
 
+	/* NOTE: order of synchronization of IFF_PROMISC and IFF_ALLMULTI
+	   is important. Some (broken) drivers set IFF_PROMISC, when
+	   IFF_ALLMULTI is requested not asking us and not reporting.
+	 */
+	if (change & IFF_PROMISC)
+		dev_set_promiscuity(real_dev,
+				      dev->flags & IFF_PROMISC ? 1 : -1);
 	if (change & IFF_ALLMULTI)
 		dev_set_allmulti(real_dev, dev->flags & IFF_ALLMULTI ? 1 : -1);
-	if (change & IFF_PROMISC)
-		dev_set_promiscuity(real_dev, dev->flags & IFF_PROMISC ? 1 : -1);
 }
 
 static void vlan_dev_set_rx_mode(struct net_device *vlan_dev)
-- 
1.5.3.4

Re: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI

[Patrick McHardy]

Wang Chen wrote:
> IFF_PROMISC should be set before IFF_ALLMULTI.
> 
> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
> ---
>  net/8021q/vlan_dev.c |   25 +++++++++++++++++++------
>  1 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index 5d055c2..14742e3 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -547,10 +547,14 @@ static int vlan_dev_open(struct net_device *dev)
>  	}
>  	memcpy(vlan->real_dev_addr, real_dev->dev_addr, ETH_ALEN);
>  
> -	if (dev->flags & IFF_ALLMULTI)
> -		dev_set_allmulti(real_dev, 1);
> +	/* NOTE: order of synchronization of IFF_PROMISC and IFF_ALLMULTI
> +	   is important. Some (broken) drivers set IFF_PROMISC, when
> +	   IFF_ALLMULTI is requested not asking us and not reporting.
> +	 */
>  	if (dev->flags & IFF_PROMISC)
>  		dev_set_promiscuity(real_dev, 1);
> +	if (dev->flags & IFF_ALLMULTI)
> +		dev_set_allmulti(real_dev, 1);


What exactly is the problem here? The VLAN code is obviously not
one of the broken drivers, so why should it care what other drivers
do?

Re: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI

[Wang Chen]

Patrick McHardy said the following on 2008-6-16 17:27:
> Wang Chen wrote:
>> IFF_PROMISC should be set before IFF_ALLMULTI.
>>
>> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
>> ---
>>  net/8021q/vlan_dev.c |   25 +++++++++++++++++++------
>>  1 files changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
>> index 5d055c2..14742e3 100644
>> --- a/net/8021q/vlan_dev.c
>> +++ b/net/8021q/vlan_dev.c
>> @@ -547,10 +547,14 @@ static int vlan_dev_open(struct net_device *dev)
>>      }
>>      memcpy(vlan->real_dev_addr, real_dev->dev_addr, ETH_ALEN);
>>  
>> -    if (dev->flags & IFF_ALLMULTI)
>> -        dev_set_allmulti(real_dev, 1);
>> +    /* NOTE: order of synchronization of IFF_PROMISC and IFF_ALLMULTI
>> +       is important. Some (broken) drivers set IFF_PROMISC, when
>> +       IFF_ALLMULTI is requested not asking us and not reporting.
>> +     */
>>      if (dev->flags & IFF_PROMISC)
>>          dev_set_promiscuity(real_dev, 1);
>> +    if (dev->flags & IFF_ALLMULTI)
>> +        dev_set_allmulti(real_dev, 1);
> 
> 
> What exactly is the problem here? The VLAN code is obviously not
> one of the broken drivers, so why should it care what other drivers
> do?
> 

I think the problem is that allmulti is not valid if promis is not on.

And about the comment, I copy it from dev_change_flags() and think
it seems suit for here.
Did I misunderstand this comment?

Re: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI

[Patrick McHardy]

Wang Chen wrote:
> Patrick McHardy said the following on 2008-6-16 17:27:
>> Wang Chen wrote:
>>> -    if (dev->flags & IFF_ALLMULTI)
>>> -        dev_set_allmulti(real_dev, 1);
>>> +    /* NOTE: order of synchronization of IFF_PROMISC and IFF_ALLMULTI
>>> +       is important. Some (broken) drivers set IFF_PROMISC, when
>>> +       IFF_ALLMULTI is requested not asking us and not reporting.
>>> +     */
>>>      if (dev->flags & IFF_PROMISC)
>>>          dev_set_promiscuity(real_dev, 1);
>>> +    if (dev->flags & IFF_ALLMULTI)
>>> +        dev_set_allmulti(real_dev, 1);
>>
>> What exactly is the problem here? The VLAN code is obviously not
>> one of the broken drivers, so why should it care what other drivers
>> do?
>>
> 
> I think the problem is that allmulti is not valid if promis is not on.

No, PROMISC is a superset of ALLMULTI.

> And about the comment, I copy it from dev_change_flags() and think
> it seems suit for here.
> Did I misunderstand this comment?

I think it refers to broken behaviour by drivers that set
IFF_PROMISC themselves when asked to disable multicast
filtering by setting IFF_ALLMULTI. This would cause the
test for changed flags in dev_set_promiscuity to return zero
and not program the device for promiscous mode properly.

There are a few examples of this in the tree. But calling
dev_set_promiscuity() before dev_set_allmulti() only helps
in the dev_change_flags() case since its the only function
that might change both flags at once. In all other cases it
depends on the caller.

So for the dev_change_flags() case VLAN already uses the
"proper" ordering, the other cases might be broken with
or without your patch.

I'd suggest to fix the drivers instead, perhaps start by
adding a warning to dev_change_flags() that is triggered
by the driver changing the flags itself.

Re: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI

[Wang Chen]

Patrick McHardy said the following on 2008-6-16 18:03:
> Wang Chen wrote:
>> Patrick McHardy said the following on 2008-6-16 17:27:
>>> Wang Chen wrote:
>>>> -    if (dev->flags & IFF_ALLMULTI)
>>>> -        dev_set_allmulti(real_dev, 1);
>>>> +    /* NOTE: order of synchronization of IFF_PROMISC and IFF_ALLMULTI
>>>> +       is important. Some (broken) drivers set IFF_PROMISC, when
>>>> +       IFF_ALLMULTI is requested not asking us and not reporting.
>>>> +     */
>>>>      if (dev->flags & IFF_PROMISC)
>>>>          dev_set_promiscuity(real_dev, 1);
>>>> +    if (dev->flags & IFF_ALLMULTI)
>>>> +        dev_set_allmulti(real_dev, 1);
>>>
>>> What exactly is the problem here? The VLAN code is obviously not
>>> one of the broken drivers, so why should it care what other drivers
>>> do?
>>>
>>
>> I think the problem is that allmulti is not valid if promis is not on.
> 
> No, PROMISC is a superset of ALLMULTI.
> 
>> And about the comment, I copy it from dev_change_flags() and think
>> it seems suit for here.
>> Did I misunderstand this comment?
> 
> I think it refers to broken behaviour by drivers that set
> IFF_PROMISC themselves when asked to disable multicast
> filtering by setting IFF_ALLMULTI. This would cause the
> test for changed flags in dev_set_promiscuity to return zero
> and not program the device for promiscous mode properly.
> 

Do you mean things like that in do_mc32_set_multicast_list()?

> There are a few examples of this in the tree. But calling
> dev_set_promiscuity() before dev_set_allmulti() only helps
> in the dev_change_flags() case since its the only function
> that might change both flags at once. In all other cases it
> depends on the caller.
> 
> So for the dev_change_flags() case VLAN already uses the
> "proper" ordering, the other cases might be broken with
> or without your patch.
> 

Is there any other case might be broken?

> I'd suggest to fix the drivers instead, perhaps start by
> adding a warning to dev_change_flags() that is triggered
> by the driver changing the flags itself.
> 

In some driver's code of *_set_multicast_list(), IFF_PROMISC
will be set if IFF_ALLMULTI is set.
And there is comment about the necessity for setting IFF_PROMISC.
/*
 *	We must make the kernel realise we had to move
 *	into promisc mode or we start all out war on
 *	the cable. If it was a promisc request the
 *	flag is already set. If not we assert it.
 */
So, I doubt about fixing the drivers.

Re: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI

[Patrick McHardy]

Wang Chen wrote:
> Patrick McHardy said the following on 2008-6-16 18:03:
>> Wang Chen wrote:
>>> And about the comment, I copy it from dev_change_flags() and think
>>> it seems suit for here.
>>> Did I misunderstand this comment?
>> I think it refers to broken behaviour by drivers that set
>> IFF_PROMISC themselves when asked to disable multicast
>> filtering by setting IFF_ALLMULTI. This would cause the
>> test for changed flags in dev_set_promiscuity to return zero
>> and not program the device for promiscous mode properly.
>>
> 
> Do you mean things like that in do_mc32_set_multicast_list()?

Yes.

>> There are a few examples of this in the tree. But calling
>> dev_set_promiscuity() before dev_set_allmulti() only helps
>> in the dev_change_flags() case since its the only function
>> that might change both flags at once. In all other cases it
>> depends on the caller.
>>
>> So for the dev_change_flags() case VLAN already uses the
>> "proper" ordering, the other cases might be broken with
>> or without your patch.
>>
> 
> Is there any other case might be broken?

If that ordering is really required, yes:

- ip link set dev eth0 allmulticast on

<sets allmulticast *and* promisous with broken driver>

- ip link set dev eth0 promisc on

<no change>

So the only thing fixed by this workaround is of both are
enabled in a single command - something that doesn't even
make much sense since promisc will receive all multicast
frames anyway.

>> I'd suggest to fix the drivers instead, perhaps start by
>> adding a warning to dev_change_flags() that is triggered
>> by the driver changing the flags itself.
>>
> 
> In some driver's code of *_set_multicast_list(), IFF_PROMISC
> will be set if IFF_ALLMULTI is set.
> And there is comment about the necessity for setting IFF_PROMISC.
> /*
>  *	We must make the kernel realise we had to move
>  *	into promisc mode or we start all out war on
>  *	the cable. If it was a promisc request the
>  *	flag is already set. If not we assert it.
>  */
> So, I doubt about fixing the drivers.


I have no idea what this comment is trying to say. Drivers
shouldn't change dev->flags.

Re: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI

[Wang Chen]

Patrick McHardy said the following on 2008-6-17 21:06:
> Wang Chen wrote:
>> Patrick McHardy said the following on 2008-6-16 18:03:
>>> Wang Chen wrote:
>>>> And about the comment, I copy it from dev_change_flags() and think
>>>> it seems suit for here.
>>>> Did I misunderstand this comment?
>>> I think it refers to broken behaviour by drivers that set
>>> IFF_PROMISC themselves when asked to disable multicast
>>> filtering by setting IFF_ALLMULTI. This would cause the
>>> test for changed flags in dev_set_promiscuity to return zero
>>> and not program the device for promiscous mode properly.
>>>
>>
>> Do you mean things like that in do_mc32_set_multicast_list()?
> 
> Yes.
> 
>>> There are a few examples of this in the tree. But calling
>>> dev_set_promiscuity() before dev_set_allmulti() only helps
>>> in the dev_change_flags() case since its the only function
>>> that might change both flags at once. In all other cases it
>>> depends on the caller.
>>>
>>> So for the dev_change_flags() case VLAN already uses the
>>> "proper" ordering, the other cases might be broken with
>>> or without your patch.
>>>
>>
>> Is there any other case might be broken?
> 
> If that ordering is really required, yes:
> 
> - ip link set dev eth0 allmulticast on
> 
> <sets allmulticast *and* promisous with broken driver>
> 
> - ip link set dev eth0 promisc on
> 
> <no change>
> 
> So the only thing fixed by this workaround is of both are
> enabled in a single command - something that doesn't even
> make much sense since promisc will receive all multicast
> frames anyway.
> 
>>> I'd suggest to fix the drivers instead, perhaps start by
>>> adding a warning to dev_change_flags() that is triggered
>>> by the driver changing the flags itself.
>>>
>>
>> In some driver's code of *_set_multicast_list(), IFF_PROMISC
>> will be set if IFF_ALLMULTI is set.
>> And there is comment about the necessity for setting IFF_PROMISC.
>> /*
>>  *    We must make the kernel realise we had to move
>>  *    into promisc mode or we start all out war on
>>  *    the cable. If it was a promisc request the
>>  *    flag is already set. If not we assert it.
>>  */
>> So, I doubt about fixing the drivers.
> 
> 
> I have no idea what this comment is trying to say. Drivers
> shouldn't change dev->flags.
> 

Yes.
Maybe we should ask the authors about why they let driver to
change the dev->flags.
Fox example, about 3c527.
Alan,
Can you explain why do_mc32_set_multicast_list() change flag
to IFF_PROMISC when IFF_ALLMULTI is set?

Jeff,
Any suggestion about this?
netdevice uses promiscuity as refcnt, and if driver set IFF_PROMISC
but don't change promiscuity will break the refcnt.

Re: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI

[Jeff Garzik]

Wang Chen wrote:
> Patrick McHardy said the following on 2008-6-17 21:06:
>> Wang Chen wrote:
>>> Patrick McHardy said the following on 2008-6-16 18:03:
>>>> Wang Chen wrote:
>>>>> And about the comment, I copy it from dev_change_flags() and think
>>>>> it seems suit for here.
>>>>> Did I misunderstand this comment?
>>>> I think it refers to broken behaviour by drivers that set
>>>> IFF_PROMISC themselves when asked to disable multicast
>>>> filtering by setting IFF_ALLMULTI. This would cause the
>>>> test for changed flags in dev_set_promiscuity to return zero
>>>> and not program the device for promiscous mode properly.
>>>>
>>> Do you mean things like that in do_mc32_set_multicast_list()?
>> Yes.
>>
>>>> There are a few examples of this in the tree. But calling
>>>> dev_set_promiscuity() before dev_set_allmulti() only helps
>>>> in the dev_change_flags() case since its the only function
>>>> that might change both flags at once. In all other cases it
>>>> depends on the caller.
>>>>
>>>> So for the dev_change_flags() case VLAN already uses the
>>>> "proper" ordering, the other cases might be broken with
>>>> or without your patch.
>>>>
>>> Is there any other case might be broken?
>> If that ordering is really required, yes:
>>
>> - ip link set dev eth0 allmulticast on
>>
>> <sets allmulticast *and* promisous with broken driver>
>>
>> - ip link set dev eth0 promisc on
>>
>> <no change>
>>
>> So the only thing fixed by this workaround is of both are
>> enabled in a single command - something that doesn't even
>> make much sense since promisc will receive all multicast
>> frames anyway.
>>
>>>> I'd suggest to fix the drivers instead, perhaps start by
>>>> adding a warning to dev_change_flags() that is triggered
>>>> by the driver changing the flags itself.
>>>>
>>> In some driver's code of *_set_multicast_list(), IFF_PROMISC
>>> will be set if IFF_ALLMULTI is set.
>>> And there is comment about the necessity for setting IFF_PROMISC.
>>> /*
>>>  *    We must make the kernel realise we had to move
>>>  *    into promisc mode or we start all out war on
>>>  *    the cable. If it was a promisc request the
>>>  *    flag is already set. If not we assert it.
>>>  */
>>> So, I doubt about fixing the drivers.
>>
>> I have no idea what this comment is trying to say. Drivers
>> shouldn't change dev->flags.
>>
> 
> Yes.
> Maybe we should ask the authors about why they let driver to
> change the dev->flags.
> Fox example, about 3c527.
> Alan,
> Can you explain why do_mc32_set_multicast_list() change flag
> to IFF_PROMISC when IFF_ALLMULTI is set?
> 
> Jeff,
> Any suggestion about this?
> netdevice uses promiscuity as refcnt, and if driver set IFF_PROMISC
> but don't change promiscuity will break the refcnt.

Drivers should not be setting IFF_* flags in set_multicast_list().

The normal logic is that a driver interprets the request implied in 
set_multicast_list ("promisc, all-multi, or select multi?"), and then 
programs the hardware based on that.

On some hardware, IFF_ALLMULTI requires that the hardware receive all 
packets (promisc).  Even for that case, the driver should -not- be 
setting the IFF_PROMISC flag.  It should be aware of its own hardware 
programming state through some other method.

	Jeff

Re: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI

[Wang Chen]

Jeff Garzik said the following on 2008-6-18 10:52:
> Wang Chen wrote:
>> Jeff,
>> Any suggestion about this?
>> netdevice uses promiscuity as refcnt, and if driver set IFF_PROMISC
>> but don't change promiscuity will break the refcnt.
> 
> Drivers should not be setting IFF_* flags in set_multicast_list().
> 
> The normal logic is that a driver interprets the request implied in
> set_multicast_list ("promisc, all-multi, or select multi?"), and then
> programs the hardware based on that.
> 
> On some hardware, IFF_ALLMULTI requires that the hardware receive all
> packets (promisc).  Even for that case, the driver should -not- be
> setting the IFF_PROMISC flag.  It should be aware of its own hardware
> programming state through some other method.
> 

OK. Roger that.
If Alan does not oppose it, I will send patches to these drivers later.
Thank you :-)

[PATCH] net-driver: Drivers don't set IFF_* flag [Was: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI]

[Wang Chen]

Jeff Garzik said the following on 2008-6-18 10:52:
>>>> In some driver's code of *_set_multicast_list(), IFF_PROMISC
>>>> will be set if IFF_ALLMULTI is set.
>>>> And there is comment about the necessity for setting IFF_PROMISC.
>>>> /*
>>>>  *    We must make the kernel realise we had to move
>>>>  *    into promisc mode or we start all out war on
>>>>  *    the cable. If it was a promisc request the
>>>>  *    flag is already set. If not we assert it.
>>>>  */
>>>> So, I doubt about fixing the drivers.
>>>
>>> I have no idea what this comment is trying to say. Drivers
>>> shouldn't change dev->flags.
>>>
>>
>> Yes.
>> Maybe we should ask the authors about why they let driver to
>> change the dev->flags.
>> Fox example, about 3c527.
>> Alan,
>> Can you explain why do_mc32_set_multicast_list() change flag
>> to IFF_PROMISC when IFF_ALLMULTI is set?
>>
>> Jeff,
>> Any suggestion about this?
>> netdevice uses promiscuity as refcnt, and if driver set IFF_PROMISC
>> but don't change promiscuity will break the refcnt.
> 
> Drivers should not be setting IFF_* flags in set_multicast_list().
> 
> The normal logic is that a driver interprets the request implied in
> set_multicast_list ("promisc, all-multi, or select multi?"), and then
> programs the hardware based on that.
> 
> On some hardware, IFF_ALLMULTI requires that the hardware receive all
> packets (promisc).  Even for that case, the driver should -not- be
> setting the IFF_PROMISC flag.  It should be aware of its own hardware
> programming state through some other method.
> 

Subject: [PATCH] net-driver: Drivers don't set IFF_* flag

Some hardware set promisc when they are requested to set IFF_ALLMULTI flag.
It's ok, but if drivers set IFF_PROMISC flag when they set promisc,
it will broken upper layer handle for promisc and allmulti.
In addition, drivers can use their own hardware programming to make it.
So do not allow drivers to set IFF_* flags.

This is a general driver fix, so I didn't split it to pieces and send
to specific driver maintainers.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
 drivers/net/3c523.c               |    4 +---
 drivers/net/3c527.c               |    9 +++------
 drivers/net/atp.c                 |    9 ++-------
 drivers/net/de620.c               |    7 -------
 drivers/net/eepro.c               |    8 --------
 drivers/net/eth16i.c              |    1 -
 drivers/net/lp486e.c              |    2 --
 drivers/net/ni5010.c              |    1 -
 drivers/net/ni52.c                |    2 +-
 drivers/net/sun3_82586.c          |    7 ++-----
 drivers/net/tulip/de4x5.c         |    2 ++
 drivers/net/wireless/orinoco.c    |    7 -------
 drivers/net/wireless/wavelan.c    |    3 ---
 drivers/net/wireless/wavelan_cs.c |    6 ------
 14 files changed, 11 insertions(+), 57 deletions(-)

diff --git a/drivers/net/3c523.c b/drivers/net/3c523.c
index 239fc42..9e48660 100644
--- a/drivers/net/3c523.c
+++ b/drivers/net/3c523.c
@@ -641,10 +641,8 @@ static int init586(struct net_device *dev)
 	cfg_cmd->time_low = 0x00;
 	cfg_cmd->time_high = 0xf2;
 	cfg_cmd->promisc = 0;
-	if (dev->flags & (IFF_ALLMULTI | IFF_PROMISC)) {
+	if (dev->flags & (IFF_ALLMULTI | IFF_PROMISC))
 		cfg_cmd->promisc = 1;
-		dev->flags |= IFF_PROMISC;
-	}
 	cfg_cmd->carr_coll = 0x00;
 
 	p->scb->cbl_offset = make16(cfg_cmd);
diff --git a/drivers/net/3c527.c b/drivers/net/3c527.c
index fae295b..a0bea51 100644
--- a/drivers/net/3c527.c
+++ b/drivers/net/3c527.c
@@ -1524,14 +1524,11 @@ static void do_mc32_set_multicast_list(struct net_device *dev, int retry)
 	struct mc32_local *lp = netdev_priv(dev);
 	u16 filt = (1<<2); /* Save Bad Packets, for stats purposes */
 
-	if (dev->flags&IFF_PROMISC)
+	if ((dev->flags&IFF_PROMISC) ||
+	    (dev->flags&IFF_ALLMULTI) ||
+	    dev->mc_count > 10)
 		/* Enable promiscuous mode */
 		filt |= 1;
-	else if((dev->flags&IFF_ALLMULTI) || dev->mc_count > 10)
-	{
-		dev->flags|=IFF_PROMISC;
-		filt |= 1;
-	}
 	else if(dev->mc_count)
 	{
 		unsigned char block[62];
diff --git a/drivers/net/atp.c b/drivers/net/atp.c
index 3d44333..c10cd80 100644
--- a/drivers/net/atp.c
+++ b/drivers/net/atp.c
@@ -854,14 +854,9 @@ static void set_rx_mode_8002(struct net_device *dev)
 	struct net_local *lp = netdev_priv(dev);
 	long ioaddr = dev->base_addr;
 
-	if ( dev->mc_count > 0 || (dev->flags & (IFF_ALLMULTI|IFF_PROMISC))) {
-		/* We must make the kernel realise we had to move
-		 *	into promisc mode or we start all out war on
-		 *	the cable. - AC
-		 */
-		dev->flags|=IFF_PROMISC;
+	if (dev->mc_count > 0 || (dev->flags & (IFF_ALLMULTI|IFF_PROMISC)))
 		lp->addr_mode = CMR2h_PROMISC;
-	} else
+	else
 		lp->addr_mode = CMR2h_Normal;
 	write_reg_high(ioaddr, CMR2, lp->addr_mode);
 }
diff --git a/drivers/net/de620.c b/drivers/net/de620.c
index 3f5190c..d454e14 100644
--- a/drivers/net/de620.c
+++ b/drivers/net/de620.c
@@ -488,13 +488,6 @@ static void de620_set_multicast_list(struct net_device *dev)
 {
 	if (dev->mc_count || dev->flags&(IFF_ALLMULTI|IFF_PROMISC))
 	{ /* Enable promiscuous mode */
-		/*
-		 *	We must make the kernel realise we had to move
-		 *	into promisc mode or we start all out war on
-		 *	the cable. - AC
-		 */
-		dev->flags|=IFF_PROMISC;
-
 		de620_set_register(dev, W_TCR, (TCR_DEF & ~RXPBM) | RXALL);
 	}
 	else
diff --git a/drivers/net/eepro.c b/drivers/net/eepro.c
index 56f5049..1f11350 100644
--- a/drivers/net/eepro.c
+++ b/drivers/net/eepro.c
@@ -1283,14 +1283,6 @@ set_multicast_list(struct net_device *dev)
 
 	if (dev->flags&(IFF_ALLMULTI|IFF_PROMISC) || dev->mc_count > 63)
 	{
-		/*
-		 *	We must make the kernel realise we had to move
-		 *	into promisc mode or we start all out war on
-		 *	the cable. If it was a promisc request the
-		 *	flag is already set. If not we assert it.
-		 */
-		dev->flags|=IFF_PROMISC;
-
 		eepro_sw2bank2(ioaddr); /* be CAREFUL, BANK 2 now */
 		mode = inb(ioaddr + REG2);
 		outb(mode | PRMSC_Mode, ioaddr + REG2);
diff --git a/drivers/net/eth16i.c b/drivers/net/eth16i.c
index e3dd8b1..bee8b3f 100644
--- a/drivers/net/eth16i.c
+++ b/drivers/net/eth16i.c
@@ -1356,7 +1356,6 @@ static void eth16i_multicast(struct net_device *dev)
 
 	if(dev->mc_count || dev->flags&(IFF_ALLMULTI|IFF_PROMISC))
 	{
-		dev->flags|=IFF_PROMISC;	/* Must do this */
 		outb(3, ioaddr + RECEIVE_MODE_REG);
 	} else {
 		outb(2, ioaddr + RECEIVE_MODE_REG);
diff --git a/drivers/net/lp486e.c b/drivers/net/lp486e.c
index 591a7e4..83fa9d8 100644
--- a/drivers/net/lp486e.c
+++ b/drivers/net/lp486e.c
@@ -1272,8 +1272,6 @@ static void set_multicast_list(struct net_device *dev) {
 			return;
 		}
 		if (dev->mc_count == 0 && !(dev->flags & (IFF_PROMISC | IFF_ALLMULTI))) {
-			if (dev->flags & IFF_ALLMULTI)
-				dev->flags |= IFF_PROMISC;
 			lp->i596_config[8] &= ~0x01;
 		} else {
 			lp->i596_config[8] |= 0x01;
diff --git a/drivers/net/ni5010.c b/drivers/net/ni5010.c
index a20005c..8e0ca9f 100644
--- a/drivers/net/ni5010.c
+++ b/drivers/net/ni5010.c
@@ -648,7 +648,6 @@ static void ni5010_set_multicast_list(struct net_device *dev)
 	PRINTK2((KERN_DEBUG "%s: entering set_multicast_list\n", dev->name));
 
 	if (dev->flags&IFF_PROMISC || dev->flags&IFF_ALLMULTI || dev->mc_list) {
-		dev->flags |= IFF_PROMISC;
 		outb(RMD_PROMISC, EDLC_RMODE); /* Enable promiscuous mode */
 		PRINTK((KERN_DEBUG "%s: Entering promiscuous mode\n", dev->name));
 	} else {
diff --git a/drivers/net/ni52.c b/drivers/net/ni52.c
index a316dcc..b9a882d 100644
--- a/drivers/net/ni52.c
+++ b/drivers/net/ni52.c
@@ -621,7 +621,7 @@ static int init586(struct net_device *dev)
 		if (num_addrs > len) {
 			printk(KERN_ERR "%s: switching to promisc. mode\n",
 				dev->name);
-			dev->flags |= IFF_PROMISC;
+			writeb(0x01, &cfg_cmd->promisc);
 		}
 	}
 	if (dev->flags & IFF_PROMISC)
diff --git a/drivers/net/sun3_82586.c b/drivers/net/sun3_82586.c
index 9b2a7f7..e531302 100644
--- a/drivers/net/sun3_82586.c
+++ b/drivers/net/sun3_82586.c
@@ -425,14 +425,11 @@ static int init586(struct net_device *dev)
 		int len = ((char *) p->iscp - (char *) ptr - 8) / 6;
 		if(num_addrs > len)	{
 			printk("%s: switching to promisc. mode\n",dev->name);
-			dev->flags|=IFF_PROMISC;
+			cfg_cmd->promisc = 1;
 		}
 	}
 	if(dev->flags&IFF_PROMISC)
-	{
-			 cfg_cmd->promisc=1;
-			 dev->flags|=IFF_PROMISC;
-	}
+		cfg_cmd->promisc = 1;
 	cfg_cmd->carr_coll	= 0x00;
 
 	p->scb->cbl_offset	= make16(cfg_cmd);
diff --git a/drivers/net/tulip/de4x5.c b/drivers/net/tulip/de4x5.c
index bc30c6e..df22589 100644
--- a/drivers/net/tulip/de4x5.c
+++ b/drivers/net/tulip/de4x5.c
@@ -5520,6 +5520,7 @@ de4x5_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 	omr |= OMR_PR;
 	outl(omr, DE4X5_OMR);
 	dev->flags |= IFF_PROMISC;
+	dev->promiscuity++;
 	break;
 
     case DE4X5_CLR_PROM:             /* Clear Promiscuous Mode */
@@ -5528,6 +5529,7 @@ de4x5_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 	omr &= ~OMR_PR;
 	outl(omr, DE4X5_OMR);
 	dev->flags &= ~IFF_PROMISC;
+	dev->promiscuity = 0;
 	break;
 
     case DE4X5_SAY_BOO:              /* Say "Boo!" to the kernel log file */
diff --git a/drivers/net/wireless/orinoco.c b/drivers/net/wireless/orinoco.c
index 6d13a0d..00a0d79 100644
--- a/drivers/net/wireless/orinoco.c
+++ b/drivers/net/wireless/orinoco.c
@@ -1998,13 +1998,6 @@ __orinoco_set_multicast_list(struct net_device *dev)
 		else
 			priv->mc_count = mc_count;
 	}
-
-	/* Since we can set the promiscuous flag when it wasn't asked
-	   for, make sure the net_device knows about it. */
-	if (priv->promiscuous)
-		dev->flags |= IFF_PROMISC;
-	else
-		dev->flags &= ~IFF_PROMISC;
 }
 
 /* This must be called from user context, without locks held - use
diff --git a/drivers/net/wireless/wavelan.c b/drivers/net/wireless/wavelan.c
index 49ae970..136220b 100644
--- a/drivers/net/wireless/wavelan.c
+++ b/drivers/net/wireless/wavelan.c
@@ -1409,9 +1409,6 @@ static void wavelan_set_multicast_list(struct net_device * dev)
 			lp->mc_count = 0;
 
 			wv_82586_reconfig(dev);
-
-			/* Tell the kernel that we are doing a really bad job. */
-			dev->flags |= IFF_PROMISC;
 		}
 	} else
 		/* Are there multicast addresses to send? */
diff --git a/drivers/net/wireless/wavelan_cs.c b/drivers/net/wireless/wavelan_cs.c
index b584c0e..00a3559 100644
--- a/drivers/net/wireless/wavelan_cs.c
+++ b/drivers/net/wireless/wavelan_cs.c
@@ -1412,9 +1412,6 @@ wavelan_set_multicast_list(struct net_device *	dev)
 	  lp->mc_count = 0;
 
 	  wv_82593_reconfig(dev);
-
-	  /* Tell the kernel that we are doing a really bad job... */
-	  dev->flags |= IFF_PROMISC;
 	}
     }
   else
@@ -1433,9 +1430,6 @@ wavelan_set_multicast_list(struct net_device *	dev)
 	    lp->mc_count = 0;
 
 	    wv_82593_reconfig(dev);
-
-	    /* Tell the kernel that we are doing a really bad job... */
-	    dev->flags |= IFF_ALLMULTI;
 	  }
       }
     else
-- 
1.5.3.4

Re: [PATCH] net-driver: Drivers don't set IFF_* flag [Was: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI]

[Patrick McHardy]

Wang Chen wrote:
> Jeff Garzik said the following on 2008-6-18 10:52:
>> Drivers should not be setting IFF_* flags in set_multicast_list().
>>
>> The normal logic is that a driver interprets the request implied in
>> set_multicast_list ("promisc, all-multi, or select multi?"), and then
>> programs the hardware based on that.
>>
>> On some hardware, IFF_ALLMULTI requires that the hardware receive all
>> packets (promisc).  Even for that case, the driver should -not- be
>> setting the IFF_PROMISC flag.  It should be aware of its own hardware
>> programming state through some other method.
>>
> 
> Subject: [PATCH] net-driver: Drivers don't set IFF_* flag
> 
> Some hardware set promisc when they are requested to set IFF_ALLMULTI flag.
> It's ok, but if drivers set IFF_PROMISC flag when they set promisc,
> it will broken upper layer handle for promisc and allmulti.
> In addition, drivers can use their own hardware programming to make it.
> So do not allow drivers to set IFF_* flags.
> 
> This is a general driver fix, so I didn't split it to pieces and send
> to specific driver maintainers.

Did you check that these drivers don't use the PROMISC flag they
set themselves somewhere? As Jeff said, they might use it to be
aware of their hardware programming state.

> diff --git a/drivers/net/tulip/de4x5.c b/drivers/net/tulip/de4x5.c
> index bc30c6e..df22589 100644
> --- a/drivers/net/tulip/de4x5.c
> +++ b/drivers/net/tulip/de4x5.c
> @@ -5520,6 +5520,7 @@ de4x5_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>  	omr |= OMR_PR;
>  	outl(omr, DE4X5_OMR);
>  	dev->flags |= IFF_PROMISC;
> +	dev->promiscuity++;
>  	break;
>  
>      case DE4X5_CLR_PROM:             /* Clear Promiscuous Mode */
> @@ -5528,6 +5529,7 @@ de4x5_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>  	omr &= ~OMR_PR;
>  	outl(omr, DE4X5_OMR);
>  	dev->flags &= ~IFF_PROMISC;
> +	dev->promiscuity = 0;
>  	break;

Shouldn't this be using dev_set_promiscuity().

Re: [PATCH] net-driver: Drivers don't set IFF_* flag [Was: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI]

[Wang Chen]

Patrick McHardy said the following on 2008-6-23 19:04:
> Wang Chen wrote:
>> Jeff Garzik said the following on 2008-6-18 10:52:
>>> Drivers should not be setting IFF_* flags in set_multicast_list().
>>>
>>> The normal logic is that a driver interprets the request implied in
>>> set_multicast_list ("promisc, all-multi, or select multi?"), and then
>>> programs the hardware based on that.
>>>
>>> On some hardware, IFF_ALLMULTI requires that the hardware receive all
>>> packets (promisc).  Even for that case, the driver should -not- be
>>> setting the IFF_PROMISC flag.  It should be aware of its own hardware
>>> programming state through some other method.
>>>
>>
>> Subject: [PATCH] net-driver: Drivers don't set IFF_* flag
>>
>> Some hardware set promisc when they are requested to set IFF_ALLMULTI
>> flag.
>> It's ok, but if drivers set IFF_PROMISC flag when they set promisc,
>> it will broken upper layer handle for promisc and allmulti.
>> In addition, drivers can use their own hardware programming to make it.
>> So do not allow drivers to set IFF_* flags.
>>
>> This is a general driver fix, so I didn't split it to pieces and send
>> to specific driver maintainers.
> 
> Did you check that these drivers don't use the PROMISC flag they
> set themselves somewhere? As Jeff said, they might use it to be
> aware of their hardware programming state.
> 

Yes. I checked.
The flag is set but not be used anywhere else.
All of the drivers set their own state and at the same time
set IFF_PROMIDC flag.
I think that by setting IFF_PROMISC the drivers want to inform
upper layer that they set hardware to promisc although they are
requested to set ALLMULTI.
But the driver's redundant action is unneeded.
Because, if the hardwares have to set promisc mode when they
required to receive all multicast packets, it's ok, upper layer
don't need to be informed.
Only if allmulti and promiscuity all be zero, the promisc mode will be off.

>> diff --git a/drivers/net/tulip/de4x5.c b/drivers/net/tulip/de4x5.c
>> index bc30c6e..df22589 100644
>> --- a/drivers/net/tulip/de4x5.c
>> +++ b/drivers/net/tulip/de4x5.c
>> @@ -5520,6 +5520,7 @@ de4x5_ioctl(struct net_device *dev, struct ifreq
>> *rq, int cmd)
>>      omr |= OMR_PR;
>>      outl(omr, DE4X5_OMR);
>>      dev->flags |= IFF_PROMISC;
>> +    dev->promiscuity++;
>>      break;
>>  
>>      case DE4X5_CLR_PROM:             /* Clear Promiscuous Mode */
>> @@ -5528,6 +5529,7 @@ de4x5_ioctl(struct net_device *dev, struct ifreq
>> *rq, int cmd)
>>      omr &= ~OMR_PR;
>>      outl(omr, DE4X5_OMR);
>>      dev->flags &= ~IFF_PROMISC;
>> +    dev->promiscuity = 0;
>>      break;
> 
> Shouldn't this be using dev_set_promiscuity().
> 

No.
1. dev_set_promiscuity do
  a. set/unset IFF_PROMISC
  b. promiscuity++/--
  c. audit
  d. dev_set_rx_mode (upload unicast and multicast list to device)
  Here, in ioctl, a & b is enough.
2. dev->flags unset IFF_PROMISC and dev->promiscuity = 0 can not be
   replaced by dev_set_promiscuity(). Because, we don't decrease
   promiscuity here, but set promiscuity zero for unset IFF_PROMISC.

Re: [PATCH] net-driver: Drivers don't set IFF_* flag [Was: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI]

[Patrick McHardy]

Wang Chen wrote:
> Patrick McHardy said the following on 2008-6-23 19:04:
>> Did you check that these drivers don't use the PROMISC flag they
>> set themselves somewhere? As Jeff said, they might use it to be
>> aware of their hardware programming state.
>>
> 
> Yes. I checked.
> The flag is set but not be used anywhere else.
> All of the drivers set their own state and at the same time
> set IFF_PROMIDC flag.
> I think that by setting IFF_PROMISC the drivers want to inform
> upper layer that they set hardware to promisc although they are
> requested to set ALLMULTI.
> But the driver's redundant action is unneeded.
> Because, if the hardwares have to set promisc mode when they
> required to receive all multicast packets, it's ok, upper layer
> don't need to be informed.
> Only if allmulti and promiscuity all be zero, the promisc mode will be off.

OK, thanks for the explanation.

>>> @@ -5528,6 +5529,7 @@ de4x5_ioctl(struct net_device *dev, struct ifreq
>>> *rq, int cmd)
>>>      omr &= ~OMR_PR;
>>>      outl(omr, DE4X5_OMR);
>>>      dev->flags &= ~IFF_PROMISC;
>>> +    dev->promiscuity = 0;
>>>      break;
>> Shouldn't this be using dev_set_promiscuity().
>>

I actually meant dev_change_flags(), sorry.

> No.
> 1. dev_set_promiscuity do
>   a. set/unset IFF_PROMISC
>   b. promiscuity++/--
>   c. audit
>   d. dev_set_rx_mode (upload unicast and multicast list to device)
>   Here, in ioctl, a & b is enough.

Auditing should certainly be done if promiscous mode is set.
Calling dev_set_rx_mode doesn't hurt, even if it does the ioctl
handler could be changed not to care. Besides this is neither
taking the rtnl_mutex as required nor sending notifcations
to userspace.

> 2. dev->flags unset IFF_PROMISC and dev->promiscuity = 0 can not be
>    replaced by dev_set_promiscuity(). Because, we don't decrease
>    promiscuity here, but set promiscuity zero for unset IFF_PROMISC.

And that looks like a bug, the driver shouldn't disable
promiscuity if something still requires it.

Re: [PATCH] net-driver: Drivers don't set IFF_* flag [Was: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI]

[Wang Chen]

Patrick McHardy said the following on 2008-6-23 21:47:
>>>> @@ -5528,6 +5529,7 @@ de4x5_ioctl(struct net_device *dev, struct ifreq
>>>> *rq, int cmd)
>>>>      omr &= ~OMR_PR;
>>>>      outl(omr, DE4X5_OMR);
>>>>      dev->flags &= ~IFF_PROMISC;
>>>> +    dev->promiscuity = 0;
>>>>      break;
>>> Shouldn't this be using dev_set_promiscuity().
>>>
> 
> I actually meant dev_change_flags(), sorry.
> 

dev_change_flags() can not completely change flag IFF_PROMISC like
IFF_UP, etc.
So dev_change_flags() has no big difference to dev_set_promiscuity().
I think dev_set_promiscuity() is suitable here.

>> No.
>> 1. dev_set_promiscuity do
>>   a. set/unset IFF_PROMISC
>>   b. promiscuity++/--
>>   c. audit
>>   d. dev_set_rx_mode (upload unicast and multicast list to device)
>>   Here, in ioctl, a & b is enough.
> 
> Auditing should certainly be done if promiscous mode is set.
> Calling dev_set_rx_mode doesn't hurt, even if it does the ioctl
> handler could be changed not to care. Besides this is neither
> taking the rtnl_mutex as required nor sending notifcations
> to userspace.
> 

Agree.

>> 2. dev->flags unset IFF_PROMISC and dev->promiscuity = 0 can not be
>>    replaced by dev_set_promiscuity(). Because, we don't decrease
>>    promiscuity here, but set promiscuity zero for unset IFF_PROMISC.
> 
> And that looks like a bug, the driver shouldn't disable
> promiscuity if something still requires it.
> 

It's hard to say that.
In theory, user-space can require device to disable promisc by driver's
ioctl. 
But OTOH if something else still want device to be promisc, user and
driver have no method to let them decrease the refcnt promiscuity. Because
promiscuity decrement is initiative action from upper layer, drivers don't
know who need promiscuity.

Humm, tired, go to sleep and figure out how to do after refreshing. :)

Re: [PATCH] net-driver: Drivers don't set IFF_* flag [Was: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI]

[Patrick McHardy]

Wang Chen wrote:
> Patrick McHardy said the following on 2008-6-23 21:47:
>>>>> @@ -5528,6 +5529,7 @@ de4x5_ioctl(struct net_device *dev, struct ifreq
>>>>> *rq, int cmd)
>>>>>      omr &= ~OMR_PR;
>>>>>      outl(omr, DE4X5_OMR);
>>>>>      dev->flags &= ~IFF_PROMISC;
>>>>> +    dev->promiscuity = 0;
>>>>>      break;
>>>> Shouldn't this be using dev_set_promiscuity().
>>>>
>> I actually meant dev_change_flags(), sorry.
>>
> 
> dev_change_flags() can not completely change flag IFF_PROMISC like
> IFF_UP, etc.

It shouldn't (in the case of IFF_PROMISC). It can for IFF_UP.

> So dev_change_flags() has no big difference to dev_set_promiscuity().
> I think dev_set_promiscuity() is suitable here.

It doesn't send notifications and this should always be
done if changes are performed on behalf of userspace.

>>> No.
>>> 1. dev_set_promiscuity do
>>>   a. set/unset IFF_PROMISC
>>>   b. promiscuity++/--
>>>   c. audit
>>>   d. dev_set_rx_mode (upload unicast and multicast list to device)
>>>   Here, in ioctl, a & b is enough.
>> Auditing should certainly be done if promiscous mode is set.
>> Calling dev_set_rx_mode doesn't hurt, even if it does the ioctl
>> handler could be changed not to care. Besides this is neither
>> taking the rtnl_mutex as required nor sending notifcations
>> to userspace.
>>
> 
> Agree.
> 
>>> 2. dev->flags unset IFF_PROMISC and dev->promiscuity = 0 can not be
>>>    replaced by dev_set_promiscuity(). Because, we don't decrease
>>>    promiscuity here, but set promiscuity zero for unset IFF_PROMISC.
>> And that looks like a bug, the driver shouldn't disable
>> promiscuity if something still requires it.
>>
> 
> It's hard to say that.
> In theory, user-space can require device to disable promisc by driver's
> ioctl. 

It can't normally, only this driver can. And that doesn't
look right.

> But OTOH if something else still want device to be promisc, user and
> driver have no method to let them decrease the refcnt promiscuity. Because
> promiscuity decrement is initiative action from upper layer, drivers don't
> know who need promiscuity.
> 
> Humm, tired, go to sleep and figure out how to do after refreshing. :)

I'd suggest to make it user dev_change_flags() and
maybe even print a warning and add these ioctls to
feature-removal-schedule.

Re: [PATCH] net-driver: Drivers don't set IFF_* flag [Was: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI]

[Wang Chen]

Patrick McHardy said the following on 2008-6-23 22:52:
> Wang Chen wrote:
>> Patrick McHardy said the following on 2008-6-23 21:47:
>>>>>> @@ -5528,6 +5529,7 @@ de4x5_ioctl(struct net_device *dev, struct
>>>>>> ifreq
>>>>>> *rq, int cmd)
>>>>>>      omr &= ~OMR_PR;
>>>>>>      outl(omr, DE4X5_OMR);
>>>>>>      dev->flags &= ~IFF_PROMISC;
>>>>>> +    dev->promiscuity = 0;
>>>>>>      break;
>>>>> Shouldn't this be using dev_set_promiscuity().
>>>>>
>>> I actually meant dev_change_flags(), sorry.
>>>
>>
>> dev_change_flags() can not completely change flag IFF_PROMISC like
>> IFF_UP, etc.
> 
> It shouldn't (in the case of IFF_PROMISC). It can for IFF_UP.
> 
>> So dev_change_flags() has no big difference to dev_set_promiscuity().
>> I think dev_set_promiscuity() is suitable here.
> 
> It doesn't send notifications and this should always be
> done if changes are performed on behalf of userspace.
> 

Ooh, there is a call_netdevice_notifiers() in  dev_change_flags().
Yes. dev_change_flags() should be used.

>>>> No.
>>>> 1. dev_set_promiscuity do
>>>>   a. set/unset IFF_PROMISC
>>>>   b. promiscuity++/--
>>>>   c. audit
>>>>   d. dev_set_rx_mode (upload unicast and multicast list to device)
>>>>   Here, in ioctl, a & b is enough.
>>> Auditing should certainly be done if promiscous mode is set.
>>> Calling dev_set_rx_mode doesn't hurt, even if it does the ioctl
>>> handler could be changed not to care. Besides this is neither
>>> taking the rtnl_mutex as required nor sending notifcations
>>> to userspace.
>>>
>>
>> Agree.
>>
>>>> 2. dev->flags unset IFF_PROMISC and dev->promiscuity = 0 can not be
>>>>    replaced by dev_set_promiscuity(). Because, we don't decrease
>>>>    promiscuity here, but set promiscuity zero for unset IFF_PROMISC.
>>> And that looks like a bug, the driver shouldn't disable
>>> promiscuity if something still requires it.
>>>
>>
>> It's hard to say that.
>> In theory, user-space can require device to disable promisc by driver's
>> ioctl. 
> 
> It can't normally, only this driver can. And that doesn't
> look right.
> 
>> But OTOH if something else still want device to be promisc, user and
>> driver have no method to let them decrease the refcnt promiscuity.
>> Because
>> promiscuity decrement is initiative action from upper layer, drivers
>> don't
>> know who need promiscuity.
>>
>> Humm, tired, go to sleep and figure out how to do after refreshing. :)
> 
> I'd suggest to make it user dev_change_flags() and
> maybe even print a warning and add these ioctls to
> feature-removal-schedule.
> 

Yes. I also agree.
Except that if use dev_change_flags() for "case DE4X5_CLR_PROM",
we will break the logic of this code. Because it sets hardware to
non-promisc, but dev_change_flags() can only decrease the refcnt and
do not clear IFF_PROMISC. The hw side and software side will be
inconsistent.

So I want to remove "clear promisc" feature by ioctl of this driver
instead of adding it to feature-removal-schedule.
"set promisc" feature by ioctl can be fixed temporarily and should be
added to feature-removal-schedule. 
Fortunately most of the features of this driver's ioctl are for
developer testing.

This week I want to wait for tulip driver maintainer's confirmation.

Re: [PATCH] net-driver: Drivers don't set IFF_* flag [Was: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI]

[Grant Grundler]

On Tue, Jun 24, 2008 at 09:02:03AM +0800, Wang Chen wrote:
...
> So I want to remove "clear promisc" feature by ioctl of this driver
> instead of adding it to feature-removal-schedule.
> "set promisc" feature by ioctl can be fixed temporarily and should be
> added to feature-removal-schedule. 
> Fortunately most of the features of this driver's ioctl are for
> developer testing.
> 
> This week I want to wait for tulip driver maintainer's confirmation.

Confirmation of what?
Apologies for not being able to follow the whole conversation.

If you want to remove a developer debug feature, please just submit
a patch like usual to kyle and myself.

thanks,
grant

Re: [PATCH] net-driver: Drivers don't set IFF_* flag [Was: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI]

[Wang Chen]

Grant Grundler said the following on 2008-6-24 13:10:
> On Tue, Jun 24, 2008 at 09:02:03AM +0800, Wang Chen wrote:
> ...
>> So I want to remove "clear promisc" feature by ioctl of this driver
>> instead of adding it to feature-removal-schedule.
>> "set promisc" feature by ioctl can be fixed temporarily and should be
>> added to feature-removal-schedule. 
>> Fortunately most of the features of this driver's ioctl are for
>> developer testing.
>>
>> This week I want to wait for tulip driver maintainer's confirmation.
> 
> Confirmation of what?
> Apologies for not being able to follow the whole conversation.
> 

Sorry for that I cc-ed you in the mid of discussion.
Patrick and me were discussing about that driver should not
change device's promisc mode, because it's not a boolean state,
but a refcnt to decide whether IFF_PROMISC should be set or not.

In de4x5_ioctl(), user can simply set IFF_PROMISC flag and that
can cause upper layer protocol broken.

Patrick suggest to use dev_change_flags() to change IFF_PROMISC
flag and add the feature to feature-removal-schedule.

My suggestion is that we remove IFF_PROMISC changing feature from
de4x5_ioctl() now.

> If you want to remove a developer debug feature, please just submit
> a patch like usual to kyle and myself.
> 

Is there any user space binary use these features?
case DE4X5_SET_PROM:
case DE4X5_CLR_PROM: 
If not, I will send a patch to remove them.

v2 [PATCH 1/2] net-driver: Drivers don't set IFF_* flag

[Wang Chen]

Diff from v1 to v2: split de4x5's patch from previews one, because
it is a little bit different to others.

---
Some hardware set promisc when they are requested to set IFF_ALLMULTI flag.
It's ok, but if drivers set IFF_PROMISC flag when they set promisc,
it will broken upper layer handle for promisc and allmulti.
In addition, drivers can use their own hardware programming to make it.
So do not allow drivers to set IFF_* flags.

This is a general driver fix, so I didn't split it to pieces and send
to specific driver maintainers.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
diff --git a/drivers/net/3c523.c b/drivers/net/3c523.c
index 239fc42..9e48660 100644
--- a/drivers/net/3c523.c
+++ b/drivers/net/3c523.c
@@ -641,10 +641,8 @@ static int init586(struct net_device *dev)
 	cfg_cmd->time_low = 0x00;
 	cfg_cmd->time_high = 0xf2;
 	cfg_cmd->promisc = 0;
-	if (dev->flags & (IFF_ALLMULTI | IFF_PROMISC)) {
+	if (dev->flags & (IFF_ALLMULTI | IFF_PROMISC))
 		cfg_cmd->promisc = 1;
-		dev->flags |= IFF_PROMISC;
-	}
 	cfg_cmd->carr_coll = 0x00;
 
 	p->scb->cbl_offset = make16(cfg_cmd);
diff --git a/drivers/net/3c527.c b/drivers/net/3c527.c
index fae295b..a0bea51 100644
--- a/drivers/net/3c527.c
+++ b/drivers/net/3c527.c
@@ -1524,14 +1524,11 @@ static void do_mc32_set_multicast_list(struct net_device *dev, int retry)
 	struct mc32_local *lp = netdev_priv(dev);
 	u16 filt = (1<<2); /* Save Bad Packets, for stats purposes */
 
-	if (dev->flags&IFF_PROMISC)
+	if ((dev->flags&IFF_PROMISC) ||
+	    (dev->flags&IFF_ALLMULTI) ||
+	    dev->mc_count > 10)
 		/* Enable promiscuous mode */
 		filt |= 1;
-	else if((dev->flags&IFF_ALLMULTI) || dev->mc_count > 10)
-	{
-		dev->flags|=IFF_PROMISC;
-		filt |= 1;
-	}
 	else if(dev->mc_count)
 	{
 		unsigned char block[62];
diff --git a/drivers/net/atp.c b/drivers/net/atp.c
index 3d44333..c10cd80 100644
--- a/drivers/net/atp.c
+++ b/drivers/net/atp.c
@@ -854,14 +854,9 @@ static void set_rx_mode_8002(struct net_device *dev)
 	struct net_local *lp = netdev_priv(dev);
 	long ioaddr = dev->base_addr;
 
-	if ( dev->mc_count > 0 || (dev->flags & (IFF_ALLMULTI|IFF_PROMISC))) {
-		/* We must make the kernel realise we had to move
-		 *	into promisc mode or we start all out war on
-		 *	the cable. - AC
-		 */
-		dev->flags|=IFF_PROMISC;
+	if (dev->mc_count > 0 || (dev->flags & (IFF_ALLMULTI|IFF_PROMISC)))
 		lp->addr_mode = CMR2h_PROMISC;
-	} else
+	else
 		lp->addr_mode = CMR2h_Normal;
 	write_reg_high(ioaddr, CMR2, lp->addr_mode);
 }
diff --git a/drivers/net/de620.c b/drivers/net/de620.c
index 3f5190c..d454e14 100644
--- a/drivers/net/de620.c
+++ b/drivers/net/de620.c
@@ -488,13 +488,6 @@ static void de620_set_multicast_list(struct net_device *dev)
 {
 	if (dev->mc_count || dev->flags&(IFF_ALLMULTI|IFF_PROMISC))
 	{ /* Enable promiscuous mode */
-		/*
-		 *	We must make the kernel realise we had to move
-		 *	into promisc mode or we start all out war on
-		 *	the cable. - AC
-		 */
-		dev->flags|=IFF_PROMISC;
-
 		de620_set_register(dev, W_TCR, (TCR_DEF & ~RXPBM) | RXALL);
 	}
 	else
diff --git a/drivers/net/eepro.c b/drivers/net/eepro.c
index 56f5049..1f11350 100644
--- a/drivers/net/eepro.c
+++ b/drivers/net/eepro.c
@@ -1283,14 +1283,6 @@ set_multicast_list(struct net_device *dev)
 
 	if (dev->flags&(IFF_ALLMULTI|IFF_PROMISC) || dev->mc_count > 63)
 	{
-		/*
-		 *	We must make the kernel realise we had to move
-		 *	into promisc mode or we start all out war on
-		 *	the cable. If it was a promisc request the
-		 *	flag is already set. If not we assert it.
-		 */
-		dev->flags|=IFF_PROMISC;
-
 		eepro_sw2bank2(ioaddr); /* be CAREFUL, BANK 2 now */
 		mode = inb(ioaddr + REG2);
 		outb(mode | PRMSC_Mode, ioaddr + REG2);
diff --git a/drivers/net/eth16i.c b/drivers/net/eth16i.c
index e3dd8b1..bee8b3f 100644
--- a/drivers/net/eth16i.c
+++ b/drivers/net/eth16i.c
@@ -1356,7 +1356,6 @@ static void eth16i_multicast(struct net_device *dev)
 
 	if(dev->mc_count || dev->flags&(IFF_ALLMULTI|IFF_PROMISC))
 	{
-		dev->flags|=IFF_PROMISC;	/* Must do this */
 		outb(3, ioaddr + RECEIVE_MODE_REG);
 	} else {
 		outb(2, ioaddr + RECEIVE_MODE_REG);
diff --git a/drivers/net/lp486e.c b/drivers/net/lp486e.c
index 591a7e4..83fa9d8 100644
--- a/drivers/net/lp486e.c
+++ b/drivers/net/lp486e.c
@@ -1272,8 +1272,6 @@ static void set_multicast_list(struct net_device *dev) {
 			return;
 		}
 		if (dev->mc_count == 0 && !(dev->flags & (IFF_PROMISC | IFF_ALLMULTI))) {
-			if (dev->flags & IFF_ALLMULTI)
-				dev->flags |= IFF_PROMISC;
 			lp->i596_config[8] &= ~0x01;
 		} else {
 			lp->i596_config[8] |= 0x01;
diff --git a/drivers/net/ni5010.c b/drivers/net/ni5010.c
index a20005c..8e0ca9f 100644
--- a/drivers/net/ni5010.c
+++ b/drivers/net/ni5010.c
@@ -648,7 +648,6 @@ static void ni5010_set_multicast_list(struct net_device *dev)
 	PRINTK2((KERN_DEBUG "%s: entering set_multicast_list\n", dev->name));
 
 	if (dev->flags&IFF_PROMISC || dev->flags&IFF_ALLMULTI || dev->mc_list) {
-		dev->flags |= IFF_PROMISC;
 		outb(RMD_PROMISC, EDLC_RMODE); /* Enable promiscuous mode */
 		PRINTK((KERN_DEBUG "%s: Entering promiscuous mode\n", dev->name));
 	} else {
diff --git a/drivers/net/ni52.c b/drivers/net/ni52.c
index a316dcc..b9a882d 100644
--- a/drivers/net/ni52.c
+++ b/drivers/net/ni52.c
@@ -621,7 +621,7 @@ static int init586(struct net_device *dev)
 		if (num_addrs > len) {
 			printk(KERN_ERR "%s: switching to promisc. mode\n",
 				dev->name);
-			dev->flags |= IFF_PROMISC;
+			writeb(0x01, &cfg_cmd->promisc);
 		}
 	}
 	if (dev->flags & IFF_PROMISC)
diff --git a/drivers/net/sun3_82586.c b/drivers/net/sun3_82586.c
index 9b2a7f7..e531302 100644
--- a/drivers/net/sun3_82586.c
+++ b/drivers/net/sun3_82586.c
@@ -425,14 +425,11 @@ static int init586(struct net_device *dev)
 		int len = ((char *) p->iscp - (char *) ptr - 8) / 6;
 		if(num_addrs > len)	{
 			printk("%s: switching to promisc. mode\n",dev->name);
-			dev->flags|=IFF_PROMISC;
+			cfg_cmd->promisc = 1;
 		}
 	}
 	if(dev->flags&IFF_PROMISC)
-	{
-			 cfg_cmd->promisc=1;
-			 dev->flags|=IFF_PROMISC;
-	}
+		cfg_cmd->promisc = 1;
 	cfg_cmd->carr_coll	= 0x00;
 
 	p->scb->cbl_offset	= make16(cfg_cmd);
diff --git a/drivers/net/wireless/orinoco.c b/drivers/net/wireless/orinoco.c
index 6d13a0d..00a0d79 100644
--- a/drivers/net/wireless/orinoco.c
+++ b/drivers/net/wireless/orinoco.c
@@ -1998,13 +1998,6 @@ __orinoco_set_multicast_list(struct net_device *dev)
 		else
 			priv->mc_count = mc_count;
 	}
-
-	/* Since we can set the promiscuous flag when it wasn't asked
-	   for, make sure the net_device knows about it. */
-	if (priv->promiscuous)
-		dev->flags |= IFF_PROMISC;
-	else
-		dev->flags &= ~IFF_PROMISC;
 }
 
 /* This must be called from user context, without locks held - use
diff --git a/drivers/net/wireless/wavelan.c b/drivers/net/wireless/wavelan.c
index 49ae970..136220b 100644
--- a/drivers/net/wireless/wavelan.c
+++ b/drivers/net/wireless/wavelan.c
@@ -1409,9 +1409,6 @@ static void wavelan_set_multicast_list(struct net_device * dev)
 			lp->mc_count = 0;
 
 			wv_82586_reconfig(dev);
-
-			/* Tell the kernel that we are doing a really bad job. */
-			dev->flags |= IFF_PROMISC;
 		}
 	} else
 		/* Are there multicast addresses to send? */
diff --git a/drivers/net/wireless/wavelan_cs.c b/drivers/net/wireless/wavelan_cs.c
index b584c0e..00a3559 100644
--- a/drivers/net/wireless/wavelan_cs.c
+++ b/drivers/net/wireless/wavelan_cs.c
@@ -1412,9 +1412,6 @@ wavelan_set_multicast_list(struct net_device *	dev)
 	  lp->mc_count = 0;
 
 	  wv_82593_reconfig(dev);
-
-	  /* Tell the kernel that we are doing a really bad job... */
-	  dev->flags |= IFF_PROMISC;
 	}
     }
   else
@@ -1433,9 +1430,6 @@ wavelan_set_multicast_list(struct net_device *	dev)
 	    lp->mc_count = 0;
 
 	    wv_82593_reconfig(dev);
-
-	    /* Tell the kernel that we are doing a really bad job... */
-	    dev->flags |= IFF_ALLMULTI;
 	  }
       }
     else

v2 [PATCH 2/2] de4x5: Remove developer debug feature about set/clear promisc

[Wang Chen]

>> If you want to remove a developer debug feature, please just submit
>> a patch like usual to kyle and myself.
>>
> 
> Is there any user space binary use these features?
> case DE4X5_SET_PROM:
> case DE4X5_CLR_PROM: 
> If not, I will send a patch to remove them.
> 

IFF_PROMISC flag shouldn't be set or cleared by drivers, because
whether device be promisc mode is decided by how many upper layer
callers being referenced to it.
And the promisc changing feature of de4x5 ioctl is developer debug
feature, we can remove it now.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
diff --git a/drivers/net/tulip/de4x5.c b/drivers/net/tulip/de4x5.c
index bc30c6e..617ef41 100644
--- a/drivers/net/tulip/de4x5.c
+++ b/drivers/net/tulip/de4x5.c
@@ -5514,22 +5514,6 @@ de4x5_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 	netif_wake_queue(dev);                      /* Unlock the TX ring */
 	break;
 
-    case DE4X5_SET_PROM:             /* Set Promiscuous Mode */
-	if (!capable(CAP_NET_ADMIN)) return -EPERM;
-	omr = inl(DE4X5_OMR);
-	omr |= OMR_PR;
-	outl(omr, DE4X5_OMR);
-	dev->flags |= IFF_PROMISC;
-	break;
-
-    case DE4X5_CLR_PROM:             /* Clear Promiscuous Mode */
-	if (!capable(CAP_NET_ADMIN)) return -EPERM;
-	omr = inl(DE4X5_OMR);
-	omr &= ~OMR_PR;
-	outl(omr, DE4X5_OMR);
-	dev->flags &= ~IFF_PROMISC;
-	break;
-
     case DE4X5_SAY_BOO:              /* Say "Boo!" to the kernel log file */
 	if (!capable(CAP_NET_ADMIN)) return -EPERM;
 	printk("%s: Boo!\n", dev->name);
diff --git a/drivers/net/tulip/de4x5.h b/drivers/net/tulip/de4x5.h
index f5f33b3..a6b7638 100644
--- a/drivers/net/tulip/de4x5.h
+++ b/drivers/net/tulip/de4x5.h
@@ -1004,8 +1004,8 @@ struct de4x5_ioctl {
 */
 #define DE4X5_GET_HWADDR	0x01 /* Get the hardware address */
 #define DE4X5_SET_HWADDR	0x02 /* Set the hardware address */
-#define DE4X5_SET_PROM  	0x03 /* Set Promiscuous Mode */
-#define DE4X5_CLR_PROM  	0x04 /* Clear Promiscuous Mode */
+#define DE4X5_SET_PROM  	0x03 /* Obsoleted. Set Promiscuous Mode */
+#define DE4X5_CLR_PROM  	0x04 /* Obsoleted. Clear Promiscuous Mode */
 #define DE4X5_SAY_BOO	        0x05 /* Say "Boo!" to the kernel log file */
 #define DE4X5_GET_MCA   	0x06 /* Get a multicast address */
 #define DE4X5_SET_MCA   	0x07 /* Set a multicast address */

Re: v2 [PATCH 2/2] de4x5: Remove developer debug feature about set/clear promisc

[Grant Grundler]

On Fri, Jun 27, 2008 at 09:14:52AM +0800, Wang Chen wrote:
> >> If you want to remove a developer debug feature, please just submit
> >> a patch like usual to kyle and myself.
> >>
> > 
> > Is there any user space binary use these features?
> > case DE4X5_SET_PROM:
> > case DE4X5_CLR_PROM: 
> > If not, I will send a patch to remove them.

I spent about 20 minutes using both google and yahoo search trying
to find a use and did not. Given this driver has been visible
since about 1994, I'd expect such use to be visible.

> IFF_PROMISC flag shouldn't be set or cleared by drivers, because
> whether device be promisc mode is decided by how many upper layer
> callers being referenced to it.
> And the promisc changing feature of de4x5 ioctl is developer debug
> feature, we can remove it now.
> 
> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
> ---
> diff --git a/drivers/net/tulip/de4x5.c b/drivers/net/tulip/de4x5.c
> index bc30c6e..617ef41 100644
> --- a/drivers/net/tulip/de4x5.c
> +++ b/drivers/net/tulip/de4x5.c
> @@ -5514,22 +5514,6 @@ de4x5_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>  	netif_wake_queue(dev);                      /* Unlock the TX ring */
>  	break;
>  
> -    case DE4X5_SET_PROM:             /* Set Promiscuous Mode */
> -	if (!capable(CAP_NET_ADMIN)) return -EPERM;
> -	omr = inl(DE4X5_OMR);
> -	omr |= OMR_PR;
> -	outl(omr, DE4X5_OMR);
> -	dev->flags |= IFF_PROMISC;
> -	break;
> -
> -    case DE4X5_CLR_PROM:             /* Clear Promiscuous Mode */
> -	if (!capable(CAP_NET_ADMIN)) return -EPERM;
> -	omr = inl(DE4X5_OMR);
> -	omr &= ~OMR_PR;
> -	outl(omr, DE4X5_OMR);
> -	dev->flags &= ~IFF_PROMISC;
> -	break;
> -
>      case DE4X5_SAY_BOO:              /* Say "Boo!" to the kernel log file */
>  	if (!capable(CAP_NET_ADMIN)) return -EPERM;
>  	printk("%s: Boo!\n", dev->name);
> diff --git a/drivers/net/tulip/de4x5.h b/drivers/net/tulip/de4x5.h
> index f5f33b3..a6b7638 100644
> --- a/drivers/net/tulip/de4x5.h
> +++ b/drivers/net/tulip/de4x5.h
> @@ -1004,8 +1004,8 @@ struct de4x5_ioctl {
>  */
>  #define DE4X5_GET_HWADDR	0x01 /* Get the hardware address */
>  #define DE4X5_SET_HWADDR	0x02 /* Set the hardware address */
> -#define DE4X5_SET_PROM  	0x03 /* Set Promiscuous Mode */
> -#define DE4X5_CLR_PROM  	0x04 /* Clear Promiscuous Mode */
> +#define DE4X5_SET_PROM  	0x03 /* Obsoleted. Set Promiscuous Mode */
> +#define DE4X5_CLR_PROM  	0x04 /* Obsoleted. Clear Promiscuous Mode */

Please just remove them. If the kernel isn't going to provide the
functionality, I'd rather any apps that attempt to use it break
when they compile (vs later when they try to be run).


>  #define DE4X5_SAY_BOO	        0x05 /* Say "Boo!" to the kernel log file */
>  #define DE4X5_GET_MCA   	0x06 /* Get a multicast address */
>  #define DE4X5_SET_MCA   	0x07 /* Set a multicast address */

thanks,
grant

v3 [PATCH 2/2] de4x5: Remove developer debug feature about set/clear promisc

[Wang Chen]

Grant Grundler said the following on 2008-6-29 1:52:
>> @@ -1004,8 +1004,8 @@ struct de4x5_ioctl {
>>  */
>>  #define DE4X5_GET_HWADDR	0x01 /* Get the hardware address */
>>  #define DE4X5_SET_HWADDR	0x02 /* Set the hardware address */
>> -#define DE4X5_SET_PROM  	0x03 /* Set Promiscuous Mode */
>> -#define DE4X5_CLR_PROM  	0x04 /* Clear Promiscuous Mode */
>> +#define DE4X5_SET_PROM  	0x03 /* Obsoleted. Set Promiscuous Mode */
>> +#define DE4X5_CLR_PROM  	0x04 /* Obsoleted. Clear Promiscuous Mode */
> 
> Please just remove them. If the kernel isn't going to provide the
> functionality, I'd rather any apps that attempt to use it break
> when they compile (vs later when they try to be run).

OK. I removed the macros, but left comment to prevent people from reusing
the code 0x03 and 0x04.

---
IFF_PROMISC flag shouldn't be set or cleared by drivers, because
whether device be promisc mode is decided by how many upper layer
callers being referenced to it.
And the promisc changing feature of de4x5 ioctl is developer debug
feature, we can remove it now.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
diff --git a/drivers/net/tulip/de4x5.c b/drivers/net/tulip/de4x5.c
index bc30c6e..617ef41 100644
--- a/drivers/net/tulip/de4x5.c
+++ b/drivers/net/tulip/de4x5.c
@@ -5514,22 +5514,6 @@ de4x5_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 	netif_wake_queue(dev);                      /* Unlock the TX ring */
 	break;
 
-    case DE4X5_SET_PROM:             /* Set Promiscuous Mode */
-	if (!capable(CAP_NET_ADMIN)) return -EPERM;
-	omr = inl(DE4X5_OMR);
-	omr |= OMR_PR;
-	outl(omr, DE4X5_OMR);
-	dev->flags |= IFF_PROMISC;
-	break;
-
-    case DE4X5_CLR_PROM:             /* Clear Promiscuous Mode */
-	if (!capable(CAP_NET_ADMIN)) return -EPERM;
-	omr = inl(DE4X5_OMR);
-	omr &= ~OMR_PR;
-	outl(omr, DE4X5_OMR);
-	dev->flags &= ~IFF_PROMISC;
-	break;
-
     case DE4X5_SAY_BOO:              /* Say "Boo!" to the kernel log file */
 	if (!capable(CAP_NET_ADMIN)) return -EPERM;
 	printk("%s: Boo!\n", dev->name);
diff --git a/drivers/net/tulip/de4x5.h b/drivers/net/tulip/de4x5.h
index f5f33b3..9f28774 100644
--- a/drivers/net/tulip/de4x5.h
+++ b/drivers/net/tulip/de4x5.h
@@ -1004,8 +1004,7 @@ struct de4x5_ioctl {
 */
 #define DE4X5_GET_HWADDR	0x01 /* Get the hardware address */
 #define DE4X5_SET_HWADDR	0x02 /* Set the hardware address */
-#define DE4X5_SET_PROM  	0x03 /* Set Promiscuous Mode */
-#define DE4X5_CLR_PROM  	0x04 /* Clear Promiscuous Mode */
+/* 0x03 and 0x04 were used before and are obsoleted now. Don't use them. */
 #define DE4X5_SAY_BOO	        0x05 /* Say "Boo!" to the kernel log file */
 #define DE4X5_GET_MCA   	0x06 /* Get a multicast address */
 #define DE4X5_SET_MCA   	0x07 /* Set a multicast address */

Re: v3 [PATCH 2/2] de4x5: Remove developer debug feature about set/clear promisc

[Grant Grundler]

On Mon, Jun 30, 2008 at 11:24:33AM +0800, Wang Chen wrote:
> Grant Grundler said the following on 2008-6-29 1:52:
> >> @@ -1004,8 +1004,8 @@ struct de4x5_ioctl {
> >>  */
> >>  #define DE4X5_GET_HWADDR	0x01 /* Get the hardware address */
> >>  #define DE4X5_SET_HWADDR	0x02 /* Set the hardware address */
> >> -#define DE4X5_SET_PROM  	0x03 /* Set Promiscuous Mode */
> >> -#define DE4X5_CLR_PROM  	0x04 /* Clear Promiscuous Mode */
> >> +#define DE4X5_SET_PROM  	0x03 /* Obsoleted. Set Promiscuous Mode */
> >> +#define DE4X5_CLR_PROM  	0x04 /* Obsoleted. Clear Promiscuous Mode */
> > 
> > Please just remove them. If the kernel isn't going to provide the
> > functionality, I'd rather any apps that attempt to use it break
> > when they compile (vs later when they try to be run).
> 
> OK. I removed the macros, but left comment to prevent people from reusing
> the code 0x03 and 0x04.

LGTM.
Jeff, can you please apply this?

> IFF_PROMISC flag shouldn't be set or cleared by drivers, because
> whether device be promisc mode is decided by how many upper layer
> callers being referenced to it.
> And the promisc changing feature of de4x5 ioctl is developer debug
> feature, we can remove it now.
> 
> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>

Acked-by: Grant Grundler <grundler@parisc-linux.org>

> ---
> diff --git a/drivers/net/tulip/de4x5.c b/drivers/net/tulip/de4x5.c
> index bc30c6e..617ef41 100644
> --- a/drivers/net/tulip/de4x5.c
> +++ b/drivers/net/tulip/de4x5.c
> @@ -5514,22 +5514,6 @@ de4x5_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>  	netif_wake_queue(dev);                      /* Unlock the TX ring */
>  	break;
>  
> -    case DE4X5_SET_PROM:             /* Set Promiscuous Mode */
> -	if (!capable(CAP_NET_ADMIN)) return -EPERM;
> -	omr = inl(DE4X5_OMR);
> -	omr |= OMR_PR;
> -	outl(omr, DE4X5_OMR);
> -	dev->flags |= IFF_PROMISC;
> -	break;
> -
> -    case DE4X5_CLR_PROM:             /* Clear Promiscuous Mode */
> -	if (!capable(CAP_NET_ADMIN)) return -EPERM;
> -	omr = inl(DE4X5_OMR);
> -	omr &= ~OMR_PR;
> -	outl(omr, DE4X5_OMR);
> -	dev->flags &= ~IFF_PROMISC;
> -	break;
> -
>      case DE4X5_SAY_BOO:              /* Say "Boo!" to the kernel log file */
>  	if (!capable(CAP_NET_ADMIN)) return -EPERM;
>  	printk("%s: Boo!\n", dev->name);
> diff --git a/drivers/net/tulip/de4x5.h b/drivers/net/tulip/de4x5.h
> index f5f33b3..9f28774 100644
> --- a/drivers/net/tulip/de4x5.h
> +++ b/drivers/net/tulip/de4x5.h
> @@ -1004,8 +1004,7 @@ struct de4x5_ioctl {
>  */
>  #define DE4X5_GET_HWADDR	0x01 /* Get the hardware address */
>  #define DE4X5_SET_HWADDR	0x02 /* Set the hardware address */
> -#define DE4X5_SET_PROM  	0x03 /* Set Promiscuous Mode */
> -#define DE4X5_CLR_PROM  	0x04 /* Clear Promiscuous Mode */
> +/* 0x03 and 0x04 were used before and are obsoleted now. Don't use them. */
>  #define DE4X5_SAY_BOO	        0x05 /* Say "Boo!" to the kernel log file */
>  #define DE4X5_GET_MCA   	0x06 /* Get a multicast address */
>  #define DE4X5_SET_MCA   	0x07 /* Set a multicast address */
>