IMHO even though netdevice is in the promiscuous mode, we should receive all of ingress packets.
This disable the vlan filtering feature when a vlan hw accel configured e1000 device goes into promiscuous mode.
This make packets visible to sniffers though it's not vlan id of itself.
Any check, comments will be appreciated.
Thanks.
Signed-off-by: Joonwoo Park <[email protected]>
---
drivers/net/e1000/e1000_main.c | 26 ++++++++++++++++++++------
1 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c index 72deff0..cdd5c84 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -2424,7 +2424,7 @@ e1000_set_multi(struct net_device *netdev)
struct e1000_adapter *adapter = netdev_priv(netdev);
struct e1000_hw *hw = &adapter->hw;
struct dev_mc_list *mc_ptr;
- uint32_t rctl;
+ uint32_t rctl, ctrl;
uint32_t hash_value;
int i, rar_entries = E1000_RAR_ENTRIES;
int mta_reg_count = (hw->mac_type == e1000_ich8lan) ?
@@ -2441,14 +2441,25 @@ e1000_set_multi(struct net_device *netdev)
/* Check for Promiscuous and All Multicast modes */
rctl = E1000_READ_REG(hw, RCTL);
+ ctrl = E1000_READ_REG(&adapter->hw, CTRL);
if (netdev->flags & IFF_PROMISC) {
rctl |= (E1000_RCTL_UPE | E1000_RCTL_MPE);
- } else if (netdev->flags & IFF_ALLMULTI) {
- rctl |= E1000_RCTL_MPE;
- rctl &= ~E1000_RCTL_UPE;
+ if (adapter->hw.mac_type != e1000_ich8lan) {
+ if (ctrl & E1000_CTRL_VME)
+ rctl &= ~E1000_RCTL_VFE;
+ }
} else {
- rctl &= ~(E1000_RCTL_UPE | E1000_RCTL_MPE);
+ if (adapter->hw.mac_type != e1000_ich8lan) {
+ if (ctrl & E1000_CTRL_VME)
+ rctl |= E1000_RCTL_VFE;
+ }
+ if (netdev->flags & IFF_ALLMULTI) {
+ rctl |= E1000_RCTL_MPE;
+ rctl &= ~E1000_RCTL_UPE;
+ } else {
+ rctl &= ~(E1000_RCTL_UPE | E1000_RCTL_MPE);
+ }
}
E1000_WRITE_REG(hw, RCTL, rctl);
@@ -4952,7 +4963,10 @@ e1000_vlan_rx_register(struct net_device *netdev, struct vlan_group *grp)
if (adapter->hw.mac_type != e1000_ich8lan) {
/* enable VLAN receive filtering */
rctl = E1000_READ_REG(&adapter->hw, RCTL);
- rctl |= E1000_RCTL_VFE;
+ if (netdev->flags & IFF_PROMISC)
+ rctl &= ~E1000_RCTL_VFE;
+ else
+ rctl |= E1000_RCTL_VFE;
rctl &= ~E1000_RCTL_CFIEN;
E1000_WRITE_REG(&adapter->hw, RCTL, rctl);
e1000_update_mng_vlan(adapter);
---
Joonwoo Park wrote:
> IMHO even though netdevice is in the promiscuous mode, we should receive all of ingress packets.
> This disable the vlan filtering feature when a vlan hw accel configured e1000 device goes into promiscuous mode.
> This make packets visible to sniffers though it's not vlan id of itself.
> Any check, comments will be appreciated.
Actually I think this patch removes a choice from the user.
Before this patch, the user can sniff all traffic by disabling vlans, or a
specific vlan only by leaving vlans on when going into promisc mode.
After this patch, the user has no choice but to sniff all vlans at all times.
I don't think that that is such a good improvement.
Auke
> Thanks.
>
> Signed-off-by: Joonwoo Park <[email protected]>
> ---
> drivers/net/e1000/e1000_main.c | 26 ++++++++++++++++++++------
> 1 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c index 72deff0..cdd5c84 100644
> --- a/drivers/net/e1000/e1000_main.c
> +++ b/drivers/net/e1000/e1000_main.c
> @@ -2424,7 +2424,7 @@ e1000_set_multi(struct net_device *netdev)
> struct e1000_adapter *adapter = netdev_priv(netdev);
> struct e1000_hw *hw = &adapter->hw;
> struct dev_mc_list *mc_ptr;
> - uint32_t rctl;
> + uint32_t rctl, ctrl;
> uint32_t hash_value;
> int i, rar_entries = E1000_RAR_ENTRIES;
> int mta_reg_count = (hw->mac_type == e1000_ich8lan) ?
> @@ -2441,14 +2441,25 @@ e1000_set_multi(struct net_device *netdev)
> /* Check for Promiscuous and All Multicast modes */
>
> rctl = E1000_READ_REG(hw, RCTL);
> + ctrl = E1000_READ_REG(&adapter->hw, CTRL);
>
> if (netdev->flags & IFF_PROMISC) {
> rctl |= (E1000_RCTL_UPE | E1000_RCTL_MPE);
> - } else if (netdev->flags & IFF_ALLMULTI) {
> - rctl |= E1000_RCTL_MPE;
> - rctl &= ~E1000_RCTL_UPE;
> + if (adapter->hw.mac_type != e1000_ich8lan) {
> + if (ctrl & E1000_CTRL_VME)
> + rctl &= ~E1000_RCTL_VFE;
> + }
> } else {
> - rctl &= ~(E1000_RCTL_UPE | E1000_RCTL_MPE);
> + if (adapter->hw.mac_type != e1000_ich8lan) {
> + if (ctrl & E1000_CTRL_VME)
> + rctl |= E1000_RCTL_VFE;
> + }
> + if (netdev->flags & IFF_ALLMULTI) {
> + rctl |= E1000_RCTL_MPE;
> + rctl &= ~E1000_RCTL_UPE;
> + } else {
> + rctl &= ~(E1000_RCTL_UPE | E1000_RCTL_MPE);
> + }
> }
>
> E1000_WRITE_REG(hw, RCTL, rctl);
> @@ -4952,7 +4963,10 @@ e1000_vlan_rx_register(struct net_device *netdev, struct vlan_group *grp)
> if (adapter->hw.mac_type != e1000_ich8lan) {
> /* enable VLAN receive filtering */
> rctl = E1000_READ_REG(&adapter->hw, RCTL);
> - rctl |= E1000_RCTL_VFE;
> + if (netdev->flags & IFF_PROMISC)
> + rctl &= ~E1000_RCTL_VFE;
> + else
> + rctl |= E1000_RCTL_VFE;
> rctl &= ~E1000_RCTL_CFIEN;
> E1000_WRITE_REG(&adapter->hw, RCTL, rctl);
> e1000_update_mng_vlan(adapter);
> ---
>
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Kok, Auke wrote:
> Joonwoo Park wrote:
>> IMHO even though netdevice is in the promiscuous mode, we should receive all of ingress packets.
>> This disable the vlan filtering feature when a vlan hw accel configured e1000 device goes into promiscuous mode.
>> This make packets visible to sniffers though it's not vlan id of itself.
>> Any check, comments will be appreciated.
>
> Actually I think this patch removes a choice from the user.
>
> Before this patch, the user can sniff all traffic by disabling vlans, or a
> specific vlan only by leaving vlans on when going into promisc mode.
>
> After this patch, the user has no choice but to sniff all vlans at all times.
>
> I don't think that that is such a good improvement.
Do you really consider that a realistic choice? Who is going to
remove interfaces that are in use just to see traffic for other
VLANs? Sniffing specific VLANs can always be done on the VLAN
device itself.
IMO its more a question of what we want promiscous mode to mean,
and I tend to agree with Joonwoo that it should receive all packets.
Patrick McHardy wrote:
> Kok, Auke wrote:
>> Joonwoo Park wrote:
>>> IMHO even though netdevice is in the promiscuous mode, we should receive all of ingress packets.
>>> This disable the vlan filtering feature when a vlan hw accel configured e1000 device goes into promiscuous mode.
>>> This make packets visible to sniffers though it's not vlan id of itself.
>>> Any check, comments will be appreciated.
>> Actually I think this patch removes a choice from the user.
>>
>> Before this patch, the user can sniff all traffic by disabling vlans, or a
>> specific vlan only by leaving vlans on when going into promisc mode.
>>
>> After this patch, the user has no choice but to sniff all vlans at all times.
>>
>> I don't think that that is such a good improvement.
>
>
> Do you really consider that a realistic choice? Who is going to
> remove interfaces that are in use just to see traffic for other
> VLANs? Sniffing specific VLANs can always be done on the VLAN
> device itself.
right, I had not thought of that.
> IMO its more a question of what we want promiscous mode to mean,
> and I tend to agree with Joonwoo that it should receive all packets.
OK, Joonwoo: can you submit a patch against e1000e as well?
Auke
From: "Kok, Auke" <[email protected]>
Date: Mon, 12 Nov 2007 09:12:40 -0800
> Actually I think this patch removes a choice from the user.
>
> Before this patch, the user can sniff all traffic by disabling vlans, or a
> specific vlan only by leaving vlans on when going into promisc mode.
>
> After this patch, the user has no choice but to sniff all vlans at all times.
>
> I don't think that that is such a good improvement.
I agree.
Look at the VLAN setting as being just like moving the
ethernet cable from one switch to another.
There is no logical difference.
From: Patrick McHardy <[email protected]>
Date: Mon, 12 Nov 2007 18:21:35 +0100
> Do you really consider that a realistic choice? Who is going to
> remove interfaces that are in use just to see traffic for other
> VLANs? Sniffing specific VLANs can always be done on the VLAN
> device itself.
Change the example to wanting to see traffic on another
physical switch.
How is this any different?
> IMO its more a question of what we want promiscous mode to mean,
> and I tend to agree with Joonwoo that it should receive all packets.
On what LAN?
When you select VLAN, you by definition are asking for non-VLAN
traffic to be elided. It is like plugging the ethernet cable
into one switch or another.
David Miller wrote:
> When you select VLAN, you by definition are asking for non-VLAN
> traffic to be elided. It is like plugging the ethernet cable
> into one switch or another.
For max functionality it seems like the raw eth device should show
everything on the wire in promiscuous mode.
If we want to sniff only the traffic for a specific vlan, we can sniff
the vlan device.
Chris
Chris Friesen wrote:
> David Miller wrote:
>
>> When you select VLAN, you by definition are asking for non-VLAN
>> traffic to be elided. It is like plugging the ethernet cable
>> into one switch or another.
>
> For max functionality it seems like the raw eth device should show
> everything on the wire in promiscuous mode.
>
> If we want to sniff only the traffic for a specific vlan, we can sniff
> the vlan device.
actually the impact can be quite negative, imagine doing a tcpdump on a 10gig
interface with vlan's enabled - all of a sudden you might accidentally flood the
system with a 100-fold increase in traffic and force the stack to dump all those
packets for you.
I'm still very reluctant about this patch, I think the current situation is OK for
everyone and offers everyone the possibility to do what they need, without hidden
consequences.
Auke
From: "Chris Friesen" <[email protected]>
Date: Mon, 12 Nov 2007 16:43:24 -0600
> David Miller wrote:
>
> > When you select VLAN, you by definition are asking for non-VLAN
> > traffic to be elided. It is like plugging the ethernet cable
> > into one switch or another.
>
> For max functionality it seems like the raw eth device should show
> everything on the wire in promiscuous mode.
>
> If we want to sniff only the traffic for a specific vlan, we can sniff
> the vlan device.
VLAN settings are a filter of sorts, much like plugging into
one switch or another filters traffic physically.
If you don't want that filter, turn the VLAN settings off.
On Mon, Nov 12, 2007 at 02:57:16PM -0800, David Miller wrote:
> From: "Chris Friesen" <[email protected]>
> Date: Mon, 12 Nov 2007 16:43:24 -0600
>
> > David Miller wrote:
> >
> > > When you select VLAN, you by definition are asking for non-VLAN
> > > traffic to be elided. It is like plugging the ethernet cable
> > > into one switch or another.
> >
> > For max functionality it seems like the raw eth device should show
> > everything on the wire in promiscuous mode.
> >
> > If we want to sniff only the traffic for a specific vlan, we can sniff
> > the vlan device.
>
> VLAN settings are a filter of sorts, much like plugging into
> one switch or another filters traffic physically.
>
> If you don't want that filter, turn the VLAN settings off.
I don't really agree with that view. Having spent a lot of time with
tcpdump on production systems, I can say that sometimes you'd like to
be aware that one of your VLANs is wrong and you'd simply like to
sniff the wire to guess the correct tag. And on production, you simply
cannot remove other VLANs, otherwise you disrupt the service.
Basically, what generally happens is that the guy responsible for the
switch tells you "it's OK now", but for you it isn't and you cannot
access the switch.
If the solution is to disable VLAN hardware acceleration, I agree that
it is very risky to do that without the user being aware of it. But at
least we should be able to do this by any means (eg: ethtool) without
disabling what's running.
And since you made the parallel with a switch, when you receive tagged
traffic on a switch port, you generally can mirror that port to another
one and catch all VLANs at once. A new feature that is starting to appear
is the ability to mirror tagged traffic to a VLAN on another port (which
means you get a double 802.1q tag). This is useful for inter-site links
between data-centers for instance.
Regards,
Willy
From: Willy Tarreau <[email protected]>
Date: Tue, 13 Nov 2007 00:15:16 +0100
> I can say that sometimes you'd like to be aware that one of your
> VLANs is wrong and you'd simply like to sniff the wire to guess the
> correct tag. And on production, you simply cannot remove other
> VLANs, otherwise you disrupt the service.
If you were plugged into the wrong physical switch, how might
you debug that problem in production? :-)
Really, it's the same issue, just virtualized, as in the name
for the feature, VLAN.
On Mon, Nov 12, 2007 at 03:19:23PM -0800, David Miller wrote:
> From: Willy Tarreau <[email protected]>
> Date: Tue, 13 Nov 2007 00:15:16 +0100
>
> > I can say that sometimes you'd like to be aware that one of your
> > VLANs is wrong and you'd simply like to sniff the wire to guess the
> > correct tag. And on production, you simply cannot remove other
> > VLANs, otherwise you disrupt the service.
>
> If you were plugged into the wrong physical switch, how might
> you debug that problem in production? :-)
tcpdump on an unfiltered RJ45 tells you a lot of things. Some switches
for instance send keepalive packets (ether proto 9000) with their own
MAC as source+dest, others send CDP packets, etc... I've very rarely
stayed plugged to the wrong switch for too long before discovering it.
But that requires the ability to sniff.
> Really, it's the same issue, just virtualized, as in the name
> for the feature, VLAN.
No, it's not the same, because as soon as you pass through a switch
which is not configured for VLANs, it does not make any distinction,
and the same DMAC is considered on a single port whatever the VLAN.
This is not true with multiple switches. Also, sometimes, being able
to see that your port gets flooded with traffic for a VLAN on which
you're not configured helps you understand why you cannot fill the
port.
I'd like that we can use the hardware correctly without having to
buy TAPs. It reminds me the discussions about TOE. You claim
yourself that TOE is bad in part because you have little if any
control on the bugs on the TCP stack on the chip. This is the same
here. If I know that the chip does not send me what it receives
when configured in promiscuous mode, I can I swear it never received
the missing packet ? There's always a doubt. Maybe sometimes the
filter is buggy, maybe it only passes tags when the remaining bits
are all zero, etc... Promiscuous mode generally means that you're
the closest possible to the wire. We already do not receive pause
frames and sometimes that's annoying. But having no control over
what we want to see is frustrating.
At least, being able to disable the feature at module load time
would be acceptable. Many people who often need to sniff on decent
machines would always keep it disabled.
Regards,
Willy
Willy Tarreau wrote:
> On Mon, Nov 12, 2007 at 03:19:23PM -0800, David Miller wrote:
>> From: Willy Tarreau <[email protected]>
>> Date: Tue, 13 Nov 2007 00:15:16 +0100
>>
>>> I can say that sometimes you'd like to be aware that one of your
>>> VLANs is wrong and you'd simply like to sniff the wire to guess the
>>> correct tag. And on production, you simply cannot remove other
>>> VLANs, otherwise you disrupt the service.
>> If you were plugged into the wrong physical switch, how might
>> you debug that problem in production? :-)
>
> tcpdump on an unfiltered RJ45 tells you a lot of things. Some switches
> for instance send keepalive packets (ether proto 9000) with their own
> MAC as source+dest, others send CDP packets, etc... I've very rarely
> stayed plugged to the wrong switch for too long before discovering it.
> But that requires the ability to sniff.
>
>> Really, it's the same issue, just virtualized, as in the name
>> for the feature, VLAN.
>
> No, it's not the same, because as soon as you pass through a switch
> which is not configured for VLANs, it does not make any distinction,
> and the same DMAC is considered on a single port whatever the VLAN.
> This is not true with multiple switches. Also, sometimes, being able
> to see that your port gets flooded with traffic for a VLAN on which
> you're not configured helps you understand why you cannot fill the
> port.
>
> I'd like that we can use the hardware correctly without having to
> buy TAPs. It reminds me the discussions about TOE. You claim
> yourself that TOE is bad in part because you have little if any
> control on the bugs on the TCP stack on the chip. This is the same
> here. If I know that the chip does not send me what it receives
> when configured in promiscuous mode, I can I swear it never received
> the missing packet ? There's always a doubt. Maybe sometimes the
> filter is buggy, maybe it only passes tags when the remaining bits
> are all zero, etc... Promiscuous mode generally means that you're
> the closest possible to the wire. We already do not receive pause
> frames and sometimes that's annoying. But having no control over
> what we want to see is frustrating.
>
> At least, being able to disable the feature at module load time
> would be acceptable. Many people who often need to sniff on decent
> machines would always keep it disabled.
I really do not want to add another module parameter to the driver for this. It
seems bogus as well: if we can agree on one sane choice it's much better, and if
we don't then we should use ethtool to provide a uniform way of implementing this
for all drivers sanely.
Auke
From: Willy Tarreau <[email protected]>
Date: Tue, 13 Nov 2007 00:32:57 +0100
> At least, being able to disable the feature at module load time
> would be acceptable. Many people who often need to sniff on decent
> machines would always keep it disabled.
I'm willing to accept the feature, in whatever form, as long
as the performance issue is dealt with properly.
2007/11/13, Willy Tarreau <[email protected]>:
> On Mon, Nov 12, 2007 at 02:57:16PM -0800, David Miller wrote:
> > From: "Chris Friesen" <[email protected]>
> > Date: Mon, 12 Nov 2007 16:43:24 -0600
> >
> > > David Miller wrote:
> > >
> > > > When you select VLAN, you by definition are asking for non-VLAN
> > > > traffic to be elided. It is like plugging the ethernet cable
> > > > into one switch or another.
> > >
> > > For max functionality it seems like the raw eth device should show
> > > everything on the wire in promiscuous mode.
> > >
> > > If we want to sniff only the traffic for a specific vlan, we can sniff
> > > the vlan device.
> >
> > VLAN settings are a filter of sorts, much like plugging into
> > one switch or another filters traffic physically.
> >
> > If you don't want that filter, turn the VLAN settings off.
>
> I don't really agree with that view. Having spent a lot of time with
> tcpdump on production systems, I can say that sometimes you'd like to
> be aware that one of your VLANs is wrong and you'd simply like to
> sniff the wire to guess the correct tag. And on production, you simply
> cannot remove other VLANs, otherwise you disrupt the service.
>
I agree.
If I had a mis-plugged cable, I can guess it with tcpdump.
Because I cannot see the packets. It means no such packets on the wire 100%.
But if I had a incorrect vlan configuration, it's hard to sure.
In a case both of mis-plugged and mis-configured situation, I cannot
see anything.
Moreover, if I am configuring a machine via vlan interface which is
mis-configured partially, I cannot disable vlan hw acceleration
feature.
Thanks.
Joonwoo
2007/11/13, David Miller <[email protected]>:
> From: Willy Tarreau <[email protected]>
> Date: Tue, 13 Nov 2007 00:32:57 +0100
>
> > At least, being able to disable the feature at module load time
> > would be acceptable. Many people who often need to sniff on decent
> > machines would always keep it disabled.
>
> I'm willing to accept the feature, in whatever form, as long
> as the performance issue is dealt with properly.
I agree with disabling the hw acceleration feature by manually would
be a non-negative solution.
IMO implementation in the ethtool seems better than module param.
As like Auke mentioned.
If you guys confirm it, I'll try it.
Thanks.
Joonwoo
Joonwoo Park wrote:
> 2007/11/13, David Miller <[email protected]>:
>
>> From: Willy Tarreau <[email protected]>
>> Date: Tue, 13 Nov 2007 00:32:57 +0100
>>
>>
>>> At least, being able to disable the feature at module load time
>>> would be acceptable. Many people who often need to sniff on decent
>>> machines would always keep it disabled.
>>>
>> I'm willing to accept the feature, in whatever form, as long
>> as the performance issue is dealt with properly.
>>
>
> I agree with disabling the hw acceleration feature by manually would
> be a non-negative solution.
> IMO implementation in the ethtool seems better than module param.
> As like Auke mentioned.
> If you guys confirm it, I'll try it.
>
I still think promiscous mode should disable all filters (which would
also provide a consistent view between accerlated and non-accerlated
devices), but an ethtool option is better than nothing :)
Patrick McHardy <[email protected]> wrote:
>
> I still think promiscous mode should disable all filters (which would
> also provide a consistent view between accerlated and non-accerlated
> devices), but an ethtool option is better than nothing :)
I agree. People doing a tcpdump don't have to turn on promiscuous
mode, that's what the -p option is for. In other words, having
promiscuous mode disable VLAN filtering does not take away the
user's options at all.
In fact, the very definition of promiscuous is to turn off hardware
filtering, albeit the filtering of MAC addresses rather than VLAN
tags. So it would seem logical to have it turn off VLAN filtering
too.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
From: Herbert Xu <[email protected]>
Date: Tue, 13 Nov 2007 19:09:23 +0800
> I agree. People doing a tcpdump don't have to turn on promiscuous
> mode, that's what the -p option is for. In other words, having
> promiscuous mode disable VLAN filtering does not take away the
> user's options at all.
>
> In fact, the very definition of promiscuous is to turn off hardware
> filtering, albeit the filtering of MAC addresses rather than VLAN
> tags. So it would seem logical to have it turn off VLAN filtering
> too.
Ok.
The performance implications can be pretty severe however.
I wish we could address this somehow.
On Tue, Nov 13, 2007 at 03:36:11AM -0800, David Miller wrote:
>
> The performance implications can be pretty severe however.
> I wish we could address this somehow.
Or perhaps we should just teach everyone to always run tcpdump
with -p, like me :)
Of course this would still have a negative impact on those who
have to be in promiscuous mode all the time (heh) due to multiple
unicast MAC addresses and such. However, we should able to
communicate that fact to the driver and the driver can then elect
to not disable VLAN acceleration unless we really want to be in
promiscuous mode.
In other words we can make it so that nobody is in promiscuous
mode and therefore have to disable VLAN acceleration *unless*
they really want to be in that state. In which case it would
imply that they wish to see everything and therefore we should
disable VLAN acceleration.
So that means I'd like to see our current core/driver interface
enhanced so that whether the user has requested us to be in
promiscuous mode can be differentiated from whether the network
stack wants us to be.
Once we have that then I would think that such a patch would be
less controversial.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
From: Herbert Xu <[email protected]>
Date: Tue, 13 Nov 2007 20:03:28 +0800
> On Tue, Nov 13, 2007 at 03:36:11AM -0800, David Miller wrote:
> >
> > The performance implications can be pretty severe however.
> > I wish we could address this somehow.
>
> Or perhaps we should just teach everyone to always run tcpdump
> with -p, like me :)
:-)
> Of course this would still have a negative impact on those who
> have to be in promiscuous mode all the time (heh) due to multiple
> unicast MAC addresses and such. However, we should able to
> communicate that fact to the driver and the driver can then elect
> to not disable VLAN acceleration unless we really want to be in
> promiscuous mode.
We already do with the code Patrick added a while ago so
that drivers can support multiple MAC addresses in hardware.
Now just to get the virtualization technologies and all the
drivers using it properly.
> In other words we can make it so that nobody is in promiscuous
> mode and therefore have to disable VLAN acceleration *unless*
> they really want to be in that state. In which case it would
> imply that they wish to see everything and therefore we should
> disable VLAN acceleration.
This is too complicated, we have multiple unicast MAC support
in the driver API already, let's simply use it.
On Tue, Nov 13, 2007 at 04:06:24AM -0800, David Miller wrote:
>
> > In other words we can make it so that nobody is in promiscuous
> > mode and therefore have to disable VLAN acceleration *unless*
> > they really want to be in that state. In which case it would
> > imply that they wish to see everything and therefore we should
> > disable VLAN acceleration.
>
> This is too complicated, we have multiple unicast MAC support
> in the driver API already, let's simply use it.
Yes I agree. People not using Patrick's new API deserves to
get poor performance so they can switch over sooner :)
What I was trying to say above is that e1000 currently uses
the old set_multicast_list interface (rather than dev_set_rx_mode)
so it's not immediately obvious why we're in promiscuous mode.
We could look at dev->promiscuity - !!uc_count but that feels a
bit fragile.
Perhaps those who want to push this patch should be encouraged
to convert e1000 to the new interface :)
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Herbert Xu wrote:
> On Tue, Nov 13, 2007 at 04:06:24AM -0800, David Miller wrote:
>>> In other words we can make it so that nobody is in promiscuous
>>> mode and therefore have to disable VLAN acceleration *unless*
>>> they really want to be in that state. In which case it would
>>> imply that they wish to see everything and therefore we should
>>> disable VLAN acceleration.
>> This is too complicated, we have multiple unicast MAC support
>> in the driver API already, let's simply use it.
>
> Yes I agree. People not using Patrick's new API deserves to
> get poor performance so they can switch over sooner :)
>
> What I was trying to say above is that e1000 currently uses
> the old set_multicast_list interface (rather than dev_set_rx_mode)
> so it's not immediately obvious why we're in promiscuous mode.
> We could look at dev->promiscuity - !!uc_count but that feels a
> bit fragile.
>
> Perhaps those who want to push this patch should be encouraged
> to convert e1000 to the new interface :)
I already posted a patch for this, not sure what happened to it.
Auke, any news on merging the secondary unicast address support?
From: Herbert Xu <[email protected]>
Date: Tue, 13 Nov 2007 20:16:47 +0800
> Perhaps those who want to push this patch should be encouraged
> to convert e1000 to the new interface :)
That is my feeling as well :-)
Patrick McHardy wrote:
> Herbert Xu wrote:
>> On Tue, Nov 13, 2007 at 04:06:24AM -0800, David Miller wrote:
>>>> In other words we can make it so that nobody is in promiscuous
>>>> mode and therefore have to disable VLAN acceleration *unless*
>>>> they really want to be in that state. In which case it would
>>>> imply that they wish to see everything and therefore we should
>>>> disable VLAN acceleration.
>>> This is too complicated, we have multiple unicast MAC support
>>> in the driver API already, let's simply use it.
>>
>> Yes I agree. People not using Patrick's new API deserves to
>> get poor performance so they can switch over sooner :)
>>
>> What I was trying to say above is that e1000 currently uses
>> the old set_multicast_list interface (rather than dev_set_rx_mode)
>> so it's not immediately obvious why we're in promiscuous mode.
>> We could look at dev->promiscuity - !!uc_count but that feels a
>> bit fragile.
>>
>> Perhaps those who want to push this patch should be encouraged
>> to convert e1000 to the new interface :)
>
>
> I already posted a patch for this, not sure what happened to it.
> Auke, any news on merging the secondary unicast address support?
I dropped the ball on that one. Care to resend it and send me one for e1000e as well?
Auke
[E1000]: Secondary unicast address support
Add support for configuring secondary unicast addresses. Unicast
addresses take precendece over multicast addresses when filling
the exact address filters to avoid going to promiscous mode.
When more unicast addresses are present than filter slots,
unicast filtering is disabled and all slots can be used for
multicast addresses.
Signed-off-by: Patrick McHardy <[email protected]>
---
commit 5d2e80a9c326ca529d278da823c8e4a4da91f612
tree 97a8ac20070b101c250e79912636124167a6dd07
parent 325d22df7b19e0116aff3391d3a03f73d0634ded
author Patrick McHardy <[email protected]> Tue, 13 Nov 2007 18:23:34 +0100
committer Patrick McHardy <[email protected]> Tue, 13 Nov 2007 18:23:34 +0100
drivers/net/e1000/e1000_main.c | 47 ++++++++++++++++++++++++++--------------
1 files changed, 31 insertions(+), 16 deletions(-)
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 72deff0..5fd5f51 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -153,7 +153,7 @@ static void e1000_clean_tx_ring(struct e1000_adapter *adapter,
struct e1000_tx_ring *tx_ring);
static void e1000_clean_rx_ring(struct e1000_adapter *adapter,
struct e1000_rx_ring *rx_ring);
-static void e1000_set_multi(struct net_device *netdev);
+static void e1000_set_rx_mode(struct net_device *netdev);
static void e1000_update_phy_info(unsigned long data);
static void e1000_watchdog(unsigned long data);
static void e1000_82547_tx_fifo_stall(unsigned long data);
@@ -514,7 +514,7 @@ static void e1000_configure(struct e1000_adapter *adapter)
struct net_device *netdev = adapter->netdev;
int i;
- e1000_set_multi(netdev);
+ e1000_set_rx_mode(netdev);
e1000_restore_vlan(adapter);
e1000_init_manageability(adapter);
@@ -926,7 +926,7 @@ e1000_probe(struct pci_dev *pdev,
netdev->stop = &e1000_close;
netdev->hard_start_xmit = &e1000_xmit_frame;
netdev->get_stats = &e1000_get_stats;
- netdev->set_multicast_list = &e1000_set_multi;
+ netdev->set_rx_mode = &e1000_set_rx_mode;
netdev->set_mac_address = &e1000_set_mac;
netdev->change_mtu = &e1000_change_mtu;
netdev->do_ioctl = &e1000_ioctl;
@@ -2409,21 +2409,22 @@ e1000_set_mac(struct net_device *netdev, void *p)
}
/**
- * e1000_set_multi - Multicast and Promiscuous mode set
+ * e1000_set_rx_mode - Secondary Unicast, Multicast and Promiscuous mode set
* @netdev: network interface device structure
*
- * The set_multi entry point is called whenever the multicast address
- * list or the network interface flags are updated. This routine is
- * responsible for configuring the hardware for proper multicast,
+ * The set_rx_mode entry point is called whenever the unicast or multicast
+ * address lists or the network interface flags are updated. This routine is
+ * responsible for configuring the hardware for proper unicast, multicast,
* promiscuous mode, and all-multi behavior.
**/
static void
-e1000_set_multi(struct net_device *netdev)
+e1000_set_rx_mode(struct net_device *netdev)
{
struct e1000_adapter *adapter = netdev_priv(netdev);
struct e1000_hw *hw = &adapter->hw;
- struct dev_mc_list *mc_ptr;
+ struct dev_addr_list *uc_ptr;
+ struct dev_addr_list *mc_ptr;
uint32_t rctl;
uint32_t hash_value;
int i, rar_entries = E1000_RAR_ENTRIES;
@@ -2446,9 +2447,16 @@ e1000_set_multi(struct net_device *netdev)
rctl |= (E1000_RCTL_UPE | E1000_RCTL_MPE);
} else if (netdev->flags & IFF_ALLMULTI) {
rctl |= E1000_RCTL_MPE;
- rctl &= ~E1000_RCTL_UPE;
} else {
- rctl &= ~(E1000_RCTL_UPE | E1000_RCTL_MPE);
+ rctl &= ~E1000_RCTL_MPE;
+ }
+
+ uc_ptr = NULL;
+ if (netdev->uc_count > rar_entries - 1) {
+ rctl |= E1000_RCTL_UPE;
+ } else if (!(netdev->flags & IFF_PROMISC)) {
+ rctl &= ~E1000_RCTL_UPE;
+ uc_ptr = netdev->uc_list;
}
E1000_WRITE_REG(hw, RCTL, rctl);
@@ -2458,7 +2466,10 @@ e1000_set_multi(struct net_device *netdev)
if (hw->mac_type == e1000_82542_rev2_0)
e1000_enter_82542_rst(adapter);
- /* load the first 14 multicast address into the exact filters 1-14
+ /* load the first 14 addresses into the exact filters 1-14. Unicast
+ * addresses take precedence to avoid disabling unicast filtering
+ * when possible.
+ *
* RAR 0 is used for the station MAC adddress
* if there are not 14 addresses, go ahead and clear the filters
* -- with 82571 controllers only 0-13 entries are filled here
@@ -2466,8 +2477,11 @@ e1000_set_multi(struct net_device *netdev)
mc_ptr = netdev->mc_list;
for (i = 1; i < rar_entries; i++) {
- if (mc_ptr) {
- e1000_rar_set(hw, mc_ptr->dmi_addr, i);
+ if (uc_ptr) {
+ e1000_rar_set(hw, uc_ptr->da_addr, i);
+ uc_ptr = uc_ptr->next;
+ } else if (mc_ptr) {
+ e1000_rar_set(hw, mc_ptr->da_addr, i);
mc_ptr = mc_ptr->next;
} else {
E1000_WRITE_REG_ARRAY(hw, RA, i << 1, 0);
@@ -2476,6 +2490,7 @@ e1000_set_multi(struct net_device *netdev)
E1000_WRITE_FLUSH(hw);
}
}
+ WARN_ON(uc_ptr != NULL);
/* clear the old settings from the multicast hash table */
@@ -2487,7 +2502,7 @@ e1000_set_multi(struct net_device *netdev)
/* load any remaining addresses into the hash table */
for (; mc_ptr; mc_ptr = mc_ptr->next) {
- hash_value = e1000_hash_mc_addr(hw, mc_ptr->dmi_addr);
+ hash_value = e1000_hash_mc_addr(hw, mc_ptr->da_addr);
e1000_mta_set(hw, hash_value);
}
@@ -5104,7 +5119,7 @@ e1000_suspend(struct pci_dev *pdev, pm_message_t state)
if (wufc) {
e1000_setup_rctl(adapter);
- e1000_set_multi(netdev);
+ e1000_set_rx_mode(netdev);
/* turn on all-multi mode if wake on multicast is enabled */
if (wufc & E1000_WUFC_MC) {
Patrick McHardy wrote:
> Kok, Auke wrote:
>> Patrick McHardy wrote:
>>
>>> I already posted a patch for this, not sure what happened to it.
>>> Auke, any news on merging the secondary unicast address support?
>>
>> I dropped the ball on that one. Care to resend it and send me one for
>> e1000e as well?
>
> Patch for e1000 attached.
>
> Does e1000e also work with PCI cards if I add the proper IDs?
> Otherwise I could only send an untested patch.
>
no, e1000e will only work with pci-e adapters. The code however is largely the
same, so if you can dish me out (off-list) some test cases I can have our labs do
the testing and add this to our test repertoire.
Cheers,
Auke
Patrick McHardy wrote:
> Kok, Auke wrote:
>> Patrick McHardy wrote:
>>
>>> I already posted a patch for this, not sure what happened to it.
>>> Auke, any news on merging the secondary unicast address support?
>>
>> I dropped the ball on that one. Care to resend it and send me one for
>> e1000e as well?
>
> Patch for e1000 attached.
>
> Does e1000e also work with PCI cards if I add the proper IDs?
> Otherwise I could only send an untested patch.
Johnwoo,
your patch unfortunately does not apply after patrick's unicast patch,
also, ich8lan support is removed from e1000 in the e1000 version in
jgarzik/netdev-2.6 #upstream as planned (moved over to e1000e!).
Can you resend your patch so that it applies to jgarzik/netdev-2.6 #upstream with
Patrick's patch applied? That would help a lot. And possibly do the e1000e patch
as well :)
Thanks,
Auke
---
[E1000]: Secondary unicast address support
Add support for configuring secondary unicast addresses. Unicast
addresses take precendece over multicast addresses when filling
the exact address filters to avoid going to promiscous mode.
When more unicast addresses are present than filter slots,
unicast filtering is disabled and all slots can be used for
multicast addresses.
Signed-off-by: Patrick McHardy <[email protected]>
---
commit 5d2e80a9c326ca529d278da823c8e4a4da91f612
tree 97a8ac20070b101c250e79912636124167a6dd07
parent 325d22df7b19e0116aff3391d3a03f73d0634ded
author Patrick McHardy <[email protected]> Tue, 13 Nov 2007 18:23:34 +0100
committer Patrick McHardy <[email protected]> Tue, 13 Nov 2007 18:23:34 +0100
drivers/net/e1000/e1000_main.c | 47 ++++++++++++++++++++++++++--------------
1 files changed, 31 insertions(+), 16 deletions(-)
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 72deff0..5fd5f51 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -153,7 +153,7 @@ static void e1000_clean_tx_ring(struct e1000_adapter *adapter,
struct e1000_tx_ring *tx_ring);
static void e1000_clean_rx_ring(struct e1000_adapter *adapter,
struct e1000_rx_ring *rx_ring);
-static void e1000_set_multi(struct net_device *netdev);
+static void e1000_set_rx_mode(struct net_device *netdev);
static void e1000_update_phy_info(unsigned long data);
static void e1000_watchdog(unsigned long data);
static void e1000_82547_tx_fifo_stall(unsigned long data);
@@ -514,7 +514,7 @@ static void e1000_configure(struct e1000_adapter *adapter)
struct net_device *netdev = adapter->netdev;
int i;
- e1000_set_multi(netdev);
+ e1000_set_rx_mode(netdev);
e1000_restore_vlan(adapter);
e1000_init_manageability(adapter);
@@ -926,7 +926,7 @@ e1000_probe(struct pci_dev *pdev,
netdev->stop = &e1000_close;
netdev->hard_start_xmit = &e1000_xmit_frame;
netdev->get_stats = &e1000_get_stats;
- netdev->set_multicast_list = &e1000_set_multi;
+ netdev->set_rx_mode = &e1000_set_rx_mode;
netdev->set_mac_address = &e1000_set_mac;
netdev->change_mtu = &e1000_change_mtu;
netdev->do_ioctl = &e1000_ioctl;
@@ -2409,21 +2409,22 @@ e1000_set_mac(struct net_device *netdev, void *p)
}
/**
- * e1000_set_multi - Multicast and Promiscuous mode set
+ * e1000_set_rx_mode - Secondary Unicast, Multicast and Promiscuous mode set
* @netdev: network interface device structure
*
- * The set_multi entry point is called whenever the multicast address
- * list or the network interface flags are updated. This routine is
- * responsible for configuring the hardware for proper multicast,
+ * The set_rx_mode entry point is called whenever the unicast or multicast
+ * address lists or the network interface flags are updated. This routine is
+ * responsible for configuring the hardware for proper unicast, multicast,
* promiscuous mode, and all-multi behavior.
**/
static void
-e1000_set_multi(struct net_device *netdev)
+e1000_set_rx_mode(struct net_device *netdev)
{
struct e1000_adapter *adapter = netdev_priv(netdev);
struct e1000_hw *hw = &adapter->hw;
- struct dev_mc_list *mc_ptr;
+ struct dev_addr_list *uc_ptr;
+ struct dev_addr_list *mc_ptr;
uint32_t rctl;
uint32_t hash_value;
int i, rar_entries = E1000_RAR_ENTRIES;
@@ -2446,9 +2447,16 @@ e1000_set_multi(struct net_device *netdev)
rctl |= (E1000_RCTL_UPE | E1000_RCTL_MPE);
} else if (netdev->flags & IFF_ALLMULTI) {
rctl |= E1000_RCTL_MPE;
- rctl &= ~E1000_RCTL_UPE;
} else {
- rctl &= ~(E1000_RCTL_UPE | E1000_RCTL_MPE);
+ rctl &= ~E1000_RCTL_MPE;
+ }
+
+ uc_ptr = NULL;
+ if (netdev->uc_count > rar_entries - 1) {
+ rctl |= E1000_RCTL_UPE;
+ } else if (!(netdev->flags & IFF_PROMISC)) {
+ rctl &= ~E1000_RCTL_UPE;
+ uc_ptr = netdev->uc_list;
}
E1000_WRITE_REG(hw, RCTL, rctl);
@@ -2458,7 +2466,10 @@ e1000_set_multi(struct net_device *netdev)
if (hw->mac_type == e1000_82542_rev2_0)
e1000_enter_82542_rst(adapter);
- /* load the first 14 multicast address into the exact filters 1-14
+ /* load the first 14 addresses into the exact filters 1-14. Unicast
+ * addresses take precedence to avoid disabling unicast filtering
+ * when possible.
+ *
* RAR 0 is used for the station MAC adddress
* if there are not 14 addresses, go ahead and clear the filters
* -- with 82571 controllers only 0-13 entries are filled here
@@ -2466,8 +2477,11 @@ e1000_set_multi(struct net_device *netdev)
mc_ptr = netdev->mc_list;
for (i = 1; i < rar_entries; i++) {
- if (mc_ptr) {
- e1000_rar_set(hw, mc_ptr->dmi_addr, i);
+ if (uc_ptr) {
+ e1000_rar_set(hw, uc_ptr->da_addr, i);
+ uc_ptr = uc_ptr->next;
+ } else if (mc_ptr) {
+ e1000_rar_set(hw, mc_ptr->da_addr, i);
mc_ptr = mc_ptr->next;
} else {
E1000_WRITE_REG_ARRAY(hw, RA, i << 1, 0);
@@ -2476,6 +2490,7 @@ e1000_set_multi(struct net_device *netdev)
E1000_WRITE_FLUSH(hw);
}
}
+ WARN_ON(uc_ptr != NULL);
/* clear the old settings from the multicast hash table */
@@ -2487,7 +2502,7 @@ e1000_set_multi(struct net_device *netdev)
/* load any remaining addresses into the hash table */
for (; mc_ptr; mc_ptr = mc_ptr->next) {
- hash_value = e1000_hash_mc_addr(hw, mc_ptr->dmi_addr);
+ hash_value = e1000_hash_mc_addr(hw, mc_ptr->da_addr);
e1000_mta_set(hw, hash_value);
}
@@ -5104,7 +5119,7 @@ e1000_suspend(struct pci_dev *pdev, pm_message_t state)
if (wufc) {
e1000_setup_rctl(adapter);
- e1000_set_multi(netdev);
+ e1000_set_rx_mode(netdev);
/* turn on all-multi mode if wake on multicast is enabled */
if (wufc & E1000_WUFC_MC) {
On Sun, 11 Nov 2007 09:51:20 +0900
"Joonwoo Park" <[email protected]> wrote:
> IMHO even though netdevice is in the promiscuous mode, we should receive all of ingress packets.
> This disable the vlan filtering feature when a vlan hw accel configured e1000 device goes into promiscuous mode.
> This make packets visible to sniffers though it's not vlan id of itself.
> Any check, comments will be appreciated.
> Thanks.
>
> Signed-off-by: Joonwoo Park <[email protected]>
Promiscuous has other uses such as bridging. Would this patch break any
existing users startup scripts?
2007/11/14, Kok, Auke <[email protected]>:
> Patrick McHardy wrote:
> > Kok, Auke wrote:
> >> Patrick McHardy wrote:
> >>
> >>> I already posted a patch for this, not sure what happened to it.
> >>> Auke, any news on merging the secondary unicast address support?
> >>
> >> I dropped the ball on that one. Care to resend it and send me one for
> >> e1000e as well?
> >
> > Patch for e1000 attached.
> >
> > Does e1000e also work with PCI cards if I add the proper IDs?
> > Otherwise I could only send an untested patch.
>
>
> Johnwoo,
>
> your patch unfortunately does not apply after patrick's unicast patch,
>
> also, ich8lan support is removed from e1000 in the e1000 version in
> jgarzik/netdev-2.6 #upstream as planned (moved over to e1000e!).
>
> Can you resend your patch so that it applies to jgarzik/netdev-2.6 #upstream with
> Patrick's patch applied? That would help a lot. And possibly do the e1000e patch
> as well :)
>
This is a patch for the jgarzik/netdev-2.6 #upstream with Patrick's one was applied.
But the ich8lan stuff was not removed at this patch.
I'll work e1000e too :-)
Thanks.
Joonwoo
[E1000]: Disable vlan hw accel when promiscuous mode
Even though netdevice is in the promiscuous mode, we should receive all of ingress packets.
This disable the vlan filtering feature when a vlan hw accel configured e1000 device goes into promiscuous mode.
This make packets visible to sniffers though it's not vlan id of itself.
Signed-off-by: Joonwoo Park <[email protected]>
---
drivers/net/e1000/e1000_main.c | 23 ++++++++++++++++++-----
1 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 5fd5f51..edf2ced 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -2425,7 +2425,7 @@ e1000_set_rx_mode(struct net_device *netdev)
struct e1000_hw *hw = &adapter->hw;
struct dev_addr_list *uc_ptr;
struct dev_addr_list *mc_ptr;
- uint32_t rctl;
+ uint32_t rctl, ctrl;
uint32_t hash_value;
int i, rar_entries = E1000_RAR_ENTRIES;
int mta_reg_count = (hw->mac_type == e1000_ich8lan) ?
@@ -2442,13 +2442,23 @@ e1000_set_rx_mode(struct net_device *netdev)
/* Check for Promiscuous and All Multicast modes */
rctl = E1000_READ_REG(hw, RCTL);
+ ctrl = E1000_READ_REG(hw, CTRL);
if (netdev->flags & IFF_PROMISC) {
rctl |= (E1000_RCTL_UPE | E1000_RCTL_MPE);
- } else if (netdev->flags & IFF_ALLMULTI) {
- rctl |= E1000_RCTL_MPE;
+ if (adapter->hw.mac_type != e1000_ich8lan) {
+ if (ctrl & E1000_CTRL_VME)
+ rctl &= ~E1000_RCTL_VFE;
+ }
} else {
- rctl &= ~E1000_RCTL_MPE;
+ if (adapter->hw.mac_type != e1000_ich8lan) {
+ if (ctrl & E1000_CTRL_VME)
+ rctl |= E1000_RCTL_VFE;
+ } else if (netdev->flags & IFF_ALLMULTI) {
+ rctl |= E1000_RCTL_MPE;
+ } else {
+ rctl &= ~E1000_RCTL_MPE;
+ }
}
uc_ptr = NULL;
@@ -4967,7 +4977,10 @@ e1000_vlan_rx_register(struct net_device *netdev, struct vlan_group *grp)
if (adapter->hw.mac_type != e1000_ich8lan) {
/* enable VLAN receive filtering */
rctl = E1000_READ_REG(&adapter->hw, RCTL);
- rctl |= E1000_RCTL_VFE;
+ if (netdev->flags & IFF_PROMISC)
+ rctl &= ~E1000_RCTL_VFE;
+ else
+ rctl |= E1000_RCTL_VFE;
rctl &= ~E1000_RCTL_CFIEN;
E1000_WRITE_REG(&adapter->hw, RCTL, rctl);
e1000_update_mng_vlan(adapter);
---
Joonwoo Park wrote:
> 2007/11/14, Kok, Auke <[email protected]>:
>> Patrick McHardy wrote:
>>> Kok, Auke wrote:
>>>> Patrick McHardy wrote:
>>>>
>>>>> I already posted a patch for this, not sure what happened to it.
>>>>> Auke, any news on merging the secondary unicast address support?
>>>> I dropped the ball on that one. Care to resend it and send me one for
>>>> e1000e as well?
>>> Patch for e1000 attached.
>>>
>>> Does e1000e also work with PCI cards if I add the proper IDs?
>>> Otherwise I could only send an untested patch.
>>
>> Johnwoo,
>>
>> your patch unfortunately does not apply after patrick's unicast patch,
>>
>> also, ich8lan support is removed from e1000 in the e1000 version in
>> jgarzik/netdev-2.6 #upstream as planned (moved over to e1000e!).
>>
>> Can you resend your patch so that it applies to jgarzik/netdev-2.6 #upstream with
>> Patrick's patch applied? That would help a lot. And possibly do the e1000e patch
>> as well :)
>>
>
> This is a patch for the jgarzik/netdev-2.6 #upstream with Patrick's one was applied.
> But the ich8lan stuff was not removed at this patch.
no, my TODO list is insane at the moment :)
thanks for the patch. I'll apply and rip the ich8 workarounds out myself later
when appropriate.
> I'll work e1000e too :-)
awesome, looking forward to that.
Auke
>
> Thanks.
> Joonwoo
>
> [E1000]: Disable vlan hw accel when promiscuous mode
>
> Even though netdevice is in the promiscuous mode, we should receive all of ingress packets.
> This disable the vlan filtering feature when a vlan hw accel configured e1000 device goes into promiscuous mode.
> This make packets visible to sniffers though it's not vlan id of itself.
>
> Signed-off-by: Joonwoo Park <[email protected]>
>
> ---
> drivers/net/e1000/e1000_main.c | 23 ++++++++++++++++++-----
> 1 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
> index 5fd5f51..edf2ced 100644
> --- a/drivers/net/e1000/e1000_main.c
> +++ b/drivers/net/e1000/e1000_main.c
> @@ -2425,7 +2425,7 @@ e1000_set_rx_mode(struct net_device *netdev)
> struct e1000_hw *hw = &adapter->hw;
> struct dev_addr_list *uc_ptr;
> struct dev_addr_list *mc_ptr;
> - uint32_t rctl;
> + uint32_t rctl, ctrl;
> uint32_t hash_value;
> int i, rar_entries = E1000_RAR_ENTRIES;
> int mta_reg_count = (hw->mac_type == e1000_ich8lan) ?
> @@ -2442,13 +2442,23 @@ e1000_set_rx_mode(struct net_device *netdev)
> /* Check for Promiscuous and All Multicast modes */
>
> rctl = E1000_READ_REG(hw, RCTL);
> + ctrl = E1000_READ_REG(hw, CTRL);
>
> if (netdev->flags & IFF_PROMISC) {
> rctl |= (E1000_RCTL_UPE | E1000_RCTL_MPE);
> - } else if (netdev->flags & IFF_ALLMULTI) {
> - rctl |= E1000_RCTL_MPE;
> + if (adapter->hw.mac_type != e1000_ich8lan) {
> + if (ctrl & E1000_CTRL_VME)
> + rctl &= ~E1000_RCTL_VFE;
> + }
> } else {
> - rctl &= ~E1000_RCTL_MPE;
> + if (adapter->hw.mac_type != e1000_ich8lan) {
> + if (ctrl & E1000_CTRL_VME)
> + rctl |= E1000_RCTL_VFE;
> + } else if (netdev->flags & IFF_ALLMULTI) {
> + rctl |= E1000_RCTL_MPE;
> + } else {
> + rctl &= ~E1000_RCTL_MPE;
> + }
> }
>
> uc_ptr = NULL;
> @@ -4967,7 +4977,10 @@ e1000_vlan_rx_register(struct net_device *netdev, struct vlan_group *grp)
> if (adapter->hw.mac_type != e1000_ich8lan) {
> /* enable VLAN receive filtering */
> rctl = E1000_READ_REG(&adapter->hw, RCTL);
> - rctl |= E1000_RCTL_VFE;
> + if (netdev->flags & IFF_PROMISC)
> + rctl &= ~E1000_RCTL_VFE;
> + else
> + rctl |= E1000_RCTL_VFE;
> rctl &= ~E1000_RCTL_CFIEN;
> E1000_WRITE_REG(&adapter->hw, RCTL, rctl);
> e1000_update_mng_vlan(adapter);
> ---
>
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> > I'll work e1000e too :-)
>
> awesome, looking forward to that.
>
BTW, It seems to need Patrick's unicast patch for e1000e first.
I'll looking forward to that too.
Thanks
Joonwoo
Kok, Auke wrote:
> Patrick McHardy wrote:
>> Kok, Auke wrote:
>>> Patrick McHardy wrote:
>>>
>>>> I already posted a patch for this, not sure what happened to it.
>>>> Auke, any news on merging the secondary unicast address support?
>>> I dropped the ball on that one. Care to resend it and send me one for
>>> e1000e as well?
>> Patch for e1000 attached.
>>
>> Does e1000e also work with PCI cards if I add the proper IDs?
>> Otherwise I could only send an untested patch.
>>
>
> no, e1000e will only work with pci-e adapters. The code however is largely the
> same, so if you can dish me out (off-list) some test cases I can have our labs do
> the testing and add this to our test repertoire.
Actually that part of the code is quite different. I'm not
sure I understand the entire intent behind the differences,
why does it use a packed array of addresses instead of simply
passing the netdev pointer to the callback? That would be
necessary for secondary unicast support since we have to
disable unicast filtering if either the address count exceeds
the number of filter slots or the device is in promiscous mode.
>>>>> "AK" == Kok, Auke <[email protected]> writes:
AK> actually the impact can be quite negative, imagine doing a tcpdump
AK> on a 10gig interface with vlan's enabled - all of a sudden you
AK> might accidentally flood the system with a 100-fold increase in
AK> traffic and force the stack to dump all those packets for you.
Why is the switch sending you all that traffic for VLAN's you don't
care about? I have a hard time imagining such a scenario. Sure, you
could have forgotten to limit the VLAN range sent to a particular
host, or even have decided that it's administratively easier to just
allow everything, but the switch still won't send unicast traffic out
that port unless the destination MAC matches. If broadcast or
non-solicited multicast takes up most of your bandwidth, you have
other problems.
/Benny
Patrick McHardy wrote:
> Kok, Auke wrote:
>> Patrick McHardy wrote:
>>> Kok, Auke wrote:
>>>> Patrick McHardy wrote:
>>>>
>>>>> I already posted a patch for this, not sure what happened to it.
>>>>> Auke, any news on merging the secondary unicast address support?
>>>> I dropped the ball on that one. Care to resend it and send me one for
>>>> e1000e as well?
>>> Patch for e1000 attached.
>>>
>>> Does e1000e also work with PCI cards if I add the proper IDs?
>>> Otherwise I could only send an untested patch.
>>>
>>
>> no, e1000e will only work with pci-e adapters. The code however is
>> largely the
>> same, so if you can dish me out (off-list) some test cases I can have
>> our labs do
>> the testing and add this to our test repertoire.
>
>
> Actually that part of the code is quite different. I'm not
> sure I understand the entire intent behind the differences,
> why does it use a packed array of addresses instead of simply
> passing the netdev pointer to the callback? That would be
> necessary for secondary unicast support since we have to
> disable unicast filtering if either the address count exceeds
> the number of filter slots or the device is in promiscous mode.
the underlying code was not specifically developed with linux only in mind and is
therefore not aware of the net_device struct, hence the specific netdev agnostic
implementation. if this implementation is posing issues then we need to look at
how to improve it or just code it specifically for linux in a better way.
Auke