Skip to contentSkip to conversation

netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI

PATCH Applied Wang Chen 4 participants 23 messages Jun 16, 2008 – Jul 2, 2008

Subsystems (3)
Networking Network drivers Networking drivers (wireless)
lore
Share & tools
lore

Wang Chen

author
#0 lore raw

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

IFF_PROMISC should be set before IFF_ALLMULTI.

Signed-off-by Wang Chen <[email protected]>
---
 net/8021q/vlan_dev.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)
net/8021q/vlan_dev.c +19 -6
index 5d055c2..14742e3 100644
@@ -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)
Signature · 1 line
1.5.3.4
#1 lore raw
Quoted 15 lines · Wang Chen

IFF_PROMISC should be set before IFF_ALLMULTI.

Signed-off-by Wang Chen <[email protected]>
---
 net/8021q/vlan_dev.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)
net/8021q/vlan_dev.c +0 -0
index 5d055c2..14742e3 100644
@@ -547,10 +547,14 @@ static int vlan_dev_open(struct net_device *dev)
 	}
 	memcpy(vlan->real_dev_addr, real_dev->dev_addr, ETH_ALEN);

net/8021q/vlan_dev.c static int vlan_dev_open(struct net_device *dev) · line 557

Quoted 10 lines
net/8021q/vlan_dev.c +6 -2
-	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?

author
#2 lore raw

Patrick McHardy said the following on 2008-6-16 17:27:

Quoted 16 lines
Quoted 15 lines · Wang Chen

IFF_PROMISC should be set before IFF_ALLMULTI.

Signed-off-by Wang Chen <[email protected]>
---
 net/8021q/vlan_dev.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)
net/8021q/vlan_dev.c +0 -0
index 5d055c2..14742e3 100644
@@ -547,10 +547,14 @@ static int vlan_dev_open(struct net_device *dev)
     }
     memcpy(vlan->real_dev_addr, real_dev->dev_addr, ETH_ALEN);
Quoted 16 lines

net/8021q/vlan_dev.c static int vlan_dev_open(struct net_device *dev) · line 557

Quoted 10 lines
net/8021q/vlan_dev.c +6 -2
-    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?

#3 lore raw
Quoted 13 lines · Wang Chen

Patrick McHardy said the following on 2008-6-16 17:27:

Quoted 12 lines
Quoted 10 lines · Wang Chen
net/8021q/vlan_dev.c +6 -2
-    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);
Quoted 6 lines
Quoted 4 lines

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.

Quoted 3 lines

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.

author
#4 lore raw

Patrick McHardy said the following on 2008-6-16 18:03:

Quoted 27 lines
Quoted 13 lines · Wang Chen

Patrick McHardy said the following on 2008-6-16 17:27:

Quoted 12 lines
Quoted 10 lines · Wang Chen
net/8021q/vlan_dev.c +6 -2
-    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);
Quoted 6 lines
Quoted 4 lines

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.

Quoted 3 lines

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

Quoted 6 lines

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()?

Quoted 10 lines

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?

Quoted 4 lines

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.

#5 lore raw
Quoted 5 lines · Wang Chen

Patrick McHardy said the following on 2008-6-16 18:03:

Quoted 4 lines
Quoted 3 lines · Wang Chen

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

Quoted 8 lines
Quoted 6 lines

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.

Quoted 6 lines
Quoted 6 lines

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.

Quoted 6 lines
Quoted 4 lines

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.

Quoted 5 lines
Quoted 4 lines

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.

Quoted 10 lines

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.

author
#6 lore raw

Patrick McHardy said the following on 2008-6-17 21:06:

Quoted 62 lines
Quoted 5 lines · Wang Chen

Patrick McHardy said the following on 2008-6-16 18:03:

Quoted 4 lines
Quoted 3 lines · Wang Chen

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

Quoted 8 lines
Quoted 6 lines

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.

Quoted 6 lines
Quoted 6 lines

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.

Quoted 6 lines
Quoted 4 lines

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.

Quoted 9 lines
Quoted 4 lines

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. /*

Quoted 6 lines
 *    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.

Quoted 3 lines

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.

#7 lore raw
Quoted 69 lines · Wang Chen

Patrick McHardy said the following on 2008-6-17 21:06:

Quoted 56 lines
Quoted 5 lines · Wang Chen

Patrick McHardy said the following on 2008-6-16 18:03:

Quoted 4 lines
Quoted 3 lines · Wang Chen

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

Quoted 7 lines
Quoted 6 lines

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.

Quoted 11 lines
Quoted 10 lines

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.

Quoted 8 lines
Quoted 4 lines

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. /*

Quoted 6 lines
 *    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.

Quoted 3 lines

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?

Quoted 4 lines

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

author
#8 lore raw

Jeff Garzik said the following on 2008-6-18 10:52:

Quoted 8 lines
Quoted 4 lines · Wang Chen

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().

Quoted 9 lines

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 :-)

author
#9 lore raw

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

In reply to Jeff Garzik

Jeff Garzik said the following on 2008-6-18 10:52:

Quoted 30 lines
Quoted 23 lines
Quoted 11 lines
Quoted 10 lines

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.

Quoted 3 lines

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?

Quoted 4 lines

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().

Quoted 9 lines

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 <[email protected]>
---
 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(-)
drivers/net/3c523.c +1 -3
index 239fc42..9e48660 100644
@@ -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);
drivers/net/3c527.c +3 -6
index fae295b..a0bea51 100644
@@ -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];
drivers/net/atp.c +2 -7
index 3d44333..c10cd80 100644
@@ -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);
 }
drivers/net/de620.c +0 -7
index 3f5190c..d454e14 100644
@@ -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
drivers/net/eepro.c +0 -8
index 56f5049..1f11350 100644
@@ -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);
drivers/net/eth16i.c +0 -1
index e3dd8b1..bee8b3f 100644
@@ -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);
drivers/net/lp486e.c +0 -2
index 591a7e4..83fa9d8 100644
@@ -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;
drivers/net/ni5010.c +0 -1
index a20005c..8e0ca9f 100644
@@ -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 {
drivers/net/ni52.c +1 -1
index a316dcc..b9a882d 100644
@@ -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)
drivers/net/sun3_82586.c +2 -5
index 9b2a7f7..e531302 100644
@@ -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);
drivers/net/tulip/de4x5.c +2 -0
index bc30c6e..df22589 100644
@@ -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 */
drivers/net/wireless/orinoco.c +0 -7
index 6d13a0d..00a0d79 100644
@@ -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
drivers/net/wireless/wavelan.c +0 -3
index 49ae970..136220b 100644
@@ -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? */
drivers/net/wireless/wavelan_cs.c +0 -6
index b584c0e..00a3559 100644
@@ -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
Signature · 1 line
1.5.3.4
#10 lore raw

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

Quoted 15 lines · Wang Chen

Jeff Garzik said the following on 2008-6-18 10:52:

Quoted 11 lines

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

Quoted 8 lines

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.

Quoted 13 lines
drivers/net/tulip/de4x5.c +1 -0
index bc30c6e..df22589 100644
@@ -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 */

drivers/net/tulip/de4x5.c de4x5_ioctl(struct net_device *dev, struct ifreq *rq, int cmd) · line 5533

Quoted 5 lines
drivers/net/tulip/de4x5.c +1 -0
@@ -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().

author
#11 lore raw

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 said the following on 2008-6-23 19:04:

Quoted 23 lines
Quoted 22 lines · Wang Chen

Jeff Garzik said the following on 2008-6-18 10:52:

Quoted 11 lines

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.

Quoted 7 lines
Quoted 2 lines

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.

Quoted 12 lines
Quoted 12 lines
drivers/net/tulip/de4x5.c +1 -0
index bc30c6e..df22589 100644
@@ -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;
Quoted 11 lines

drivers/net/tulip/de4x5.c de4x5_ioctl(struct net_device *dev, struct ifreq · line 5534

Quoted 8 lines

case DE4X5_CLR_PROM: /* Clear Promiscuous Mode */

drivers/net/tulip/de4x5.c +1 -0
@@ -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.

#12 lore raw

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

Quoted 6 lines · Wang Chen

Patrick McHardy said the following on 2008-6-23 19:04:

Quoted 4 lines

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.

Quoted 12 lines

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.

Quoted 9 lines
Quoted 9 lines

drivers/net/tulip/de4x5.c de4x5_ioctl(struct net_device *dev, struct ifreq · line 5534

Quoted 7 lines
drivers/net/tulip/de4x5.c +1 -0
@@ -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.

Quoted 7 lines

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.

Quoted 3 lines

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.

author
#13 lore raw

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 said the following on 2008-6-23 21:47:

Quoted 10 lines
Quoted 9 lines
Quoted 9 lines

drivers/net/tulip/de4x5.c de4x5_ioctl(struct net_device *dev, struct ifreq · line 5534

Quoted 7 lines
drivers/net/tulip/de4x5.c +1 -0
@@ -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().

Quoted 2 lines

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.

Quoted 8 lines
Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 7 lines
Quoted 3 lines

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. :)

#14 lore raw

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

Quoted 10 lines · Wang Chen

Patrick McHardy said the following on 2008-6-23 21:47:

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

drivers/net/tulip/de4x5.c de4x5_ioctl(struct net_device *dev, struct ifreq · line 5534

Quoted 7 lines
drivers/net/tulip/de4x5.c +1 -0
@@ -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().

Quoted 5 lines
Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 19 lines
Quoted 7 lines
Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 3 lines
Quoted 3 lines

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.

Quoted 7 lines
Quoted 3 lines

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.

Quoted 6 lines

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.

author
#15 lore raw

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 said the following on 2008-6-23 22:52:

Quoted 18 lines
Quoted 11 lines · Wang Chen

Patrick McHardy said the following on 2008-6-23 21:47:

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 8 lines
(quoted diff) +0 -0
@@ -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().

Quoted 5 lines
Quoted 2 lines

I actually meant dev_change_flags(), sorry.

dev_change_flags() can not completely change flag IFF_PROMISC like IFF_UP, etc.

Quoted 8 lines

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

Quoted 2 lines

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.

Quoted 37 lines
Quoted 19 lines
Quoted 7 lines
Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 3 lines
Quoted 3 lines

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.

Quoted 7 lines
Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 6 lines
Quoted 1 line

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.

#16 lore raw

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

On Tue, Jun 24, 2008 at 09:02:03AM +0800, Wang Chen wrote:
...

Quoted 8 lines

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

author
#17 lore raw

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 said the following on 2008-6-24 13:10:

Quoted 9 lines · Wang Chen

...

Quoted 7 lines

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.

Quoted 5 lines
Quoted 1 line

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.

Quoted 3 lines

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.

author
#18 lore raw

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

Quoted 9 lines
Quoted 3 lines

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 <[email protected]>

---

drivers/net/tulip/de4x5.c +0 -16
index bc30c6e..617ef41 100644
@@ -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);
drivers/net/tulip/de4x5.h +2 -2
index f5f33b3..a6b7638 100644
@@ -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 */
#19 lore raw

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

Quoted 8 lines · Wang Chen
Quoted 8 lines
Quoted 3 lines

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.

Quoted 40 lines

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 <[email protected]>

---

drivers/net/tulip/de4x5.c +0 -16
index bc30c6e..617ef41 100644
@@ -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);
drivers/net/tulip/de4x5.h +0 -0
index f5f33b3..a6b7638 100644

drivers/net/tulip/de4x5.h struct de4x5_ioctl { · line 1008

Quoted 7 lines
drivers/net/tulip/de4x5.h +2 -2
@@ -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).

drivers/net/tulip/de4x5.h struct de4x5_ioctl { · line 1011

Quoted 3 lines
drivers/net/tulip/de4x5.h +0 -0
 #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

author
#20 lore raw

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

Grant Grundler said the following on 2008-6-29 1:52:

Quoted 4 lines
Quoted 4 lines
(quoted diff) +0 -0
@@ -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 */
Quoted 8 lines

Review struct de4x5_ioctl { · line 1008

Quoted 4 lines
(quoted diff) +2 -2
-#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 <[email protected]>

---

drivers/net/tulip/de4x5.c +0 -16
index bc30c6e..617ef41 100644
@@ -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);
drivers/net/tulip/de4x5.h +1 -2
index f5f33b3..9f28774 100644
@@ -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 */
Acked-by
#21 lore raw

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

Quoted 14 lines · Wang Chen

Grant Grundler said the following on 2008-6-29 1:52:

Quoted 4 lines
Quoted 4 lines
drivers/net/tulip/de4x5.h +0 -0
@@ -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 */
Quoted 8 lines

drivers/net/tulip/de4x5.h struct de4x5_ioctl {

Quoted 4 lines
drivers/net/tulip/de4x5.h +2 -2
-#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).

Quoted 2 lines

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?

Quoted 7 lines

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 <[email protected]>
Acked-by Grant Grundler <[email protected]>
Quoted 43 lines

---

drivers/net/tulip/de4x5.c +0 -16
index bc30c6e..617ef41 100644
@@ -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);
drivers/net/tulip/de4x5.h +1 -2
index f5f33b3..9f28774 100644
@@ -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 */
author
#22 lore raw

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

In reply to 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 <[email protected]>

---

drivers/net/3c523.c +1 -3
index 239fc42..9e48660 100644
@@ -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);
drivers/net/3c527.c +3 -6
index fae295b..a0bea51 100644
@@ -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];
drivers/net/atp.c +2 -7
index 3d44333..c10cd80 100644
@@ -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);
 }
drivers/net/de620.c +0 -7
index 3f5190c..d454e14 100644
@@ -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
drivers/net/eepro.c +0 -8
index 56f5049..1f11350 100644
@@ -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);
drivers/net/eth16i.c +0 -1
index e3dd8b1..bee8b3f 100644
@@ -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);
drivers/net/lp486e.c +0 -2
index 591a7e4..83fa9d8 100644
@@ -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;
drivers/net/ni5010.c +0 -1
index a20005c..8e0ca9f 100644
@@ -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 {
drivers/net/ni52.c +1 -1
index a316dcc..b9a882d 100644
@@ -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)
drivers/net/sun3_82586.c +2 -5
index 9b2a7f7..e531302 100644
@@ -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);
drivers/net/wireless/orinoco.c +0 -7
index 6d13a0d..00a0d79 100644
@@ -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
drivers/net/wireless/wavelan.c +0 -3
index 49ae970..136220b 100644
@@ -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? */
drivers/net/wireless/wavelan_cs.c +0 -6
index b584c0e..00a3559 100644
@@ -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