Patrick McHardy said the following on 2008-6-23 22:52:

Wang Chen wrote:

quoted

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

quoted

quoted

quoted

quoted

@@ -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.

quoted

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

quoted

quoted

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.

quoted

quoted

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.

quoted

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.