Wang Chen wrote:

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

quoted

Wang Chen wrote:

quoted

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.

quoted

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

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.