Date:	Tue, 24 Jun 2008 09:02:03 +0800
From:	Wang Chen <wangchen@...fujitsu.com>
To:	Patrick McHardy <kaber@...sh.net>
CC:	Jeff Garzik <jgarzik@...ox.com>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	NETDEV <netdev@...r.kernel.org>, davies@...iac.ultranet.com,
	grundler@...isc-linux.org, kyle@...isc-linux.org
Subject: 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:
> 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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists