Re: [PATCH 001/001] forcedeth: Don't enable hardware vlan support on hardware that doesn't support it

3 messages, 3 authors, 2011-06-16 · open the first message on its own page

Re: [PATCH 001/001] forcedeth: Don't enable hardware vlan support on hardware that doesn't support it

From: Stephen Hemminger <hidden>
Date: 2011-06-15 20:53:04

quoted hunk
In the forcedeth driver hardware vlan support is used even on hardware
that doesn't support it leading to incorrect tagging of some packets
when using vlan.

Signed-off-by: Antoine Reversat <redacted>
---
--- linux-2.6.39/drivers/net/forcedeth.c 2011-05-19 00:06:34.000000000
-0400 +++ linux-2.6.39-fixed/drivers/net/forcedeth.c 2011-06-15
15:57:45.331158001 -0400
@@ -4915,6 +4915,10 @@ static void nv_vlan_rx_register(struct n
{ struct fe_priv *np = get_nvpriv(dev);

+ /* Don't do anything if device doesn't support VLAN */
+ if (!(np->driver_data & DEV_HAS_VLAN))
+ return;
+ spin_lock_irq(&np->lock);

/* save vlan group */
This shouldn't be necessary. rx_register should not be called
unless NETIF_F_HW_VLAN_RX is set; and device should not be setting
NETIF_F_HW_VLAN_RX unless DEV_HAS_VLAN is set.

The real problem is vlan_dev.c, and applies to all devices.

Re: [PATCH 001/001] forcedeth: Don't enable hardware vlan support on hardware that doesn't support it

From: Antoine Reversat <hidden>
Date: 2011-06-15 21:08:19

On Wed, Jun 15, 2011 at 4:53 PM, Stephen Hemminger
[off-list ref] wrote:
This shouldn't be necessary. rx_register should not be called
unless NETIF_F_HW_VLAN_RX is set; and device should not be setting
NETIF_F_HW_VLAN_RX unless DEV_HAS_VLAN is set.
I can confirm that rx_register gets called on hardware that doesn't
have vlan support (namely MCP79).
From what I can see in vlan.c (in register_vlan_dev) there is no check
on the features of the device before calling the register function :

    if (ngrp) {
        if (ops->ndo_vlan_rx_register)
            ops->ndo_vlan_rx_register(real_dev, ngrp);
        rcu_assign_pointer(real_dev->vlgrp, ngrp);
    }

If the function exists, it's called. Should I send a patch to call the
function only if the hardware supports it ?
The real problem is vlan_dev.c, and applies to all devices.

Re: [PATCH 001/001] forcedeth: Don't enable hardware vlan support on hardware that doesn't support it

From: Stephen Hemminger <hidden>
Date: 2011-06-16 15:42:50

On Wed, 15 Jun 2011 17:08:18 -0400
Antoine Reversat [off-list ref] wrote:
On Wed, Jun 15, 2011 at 4:53 PM, Stephen Hemminger
[off-list ref] wrote:
quoted
This shouldn't be necessary. rx_register should not be called
unless NETIF_F_HW_VLAN_RX is set; and device should not be setting
NETIF_F_HW_VLAN_RX unless DEV_HAS_VLAN is set.
I can confirm that rx_register gets called on hardware that doesn't
have vlan support (namely MCP79).

From what I can see in vlan.c (in register_vlan_dev) there is no check
on the features of the device before calling the register function :

    if (ngrp) {
        if (ops->ndo_vlan_rx_register)
            ops->ndo_vlan_rx_register(real_dev, ngrp);
        rcu_assign_pointer(real_dev->vlgrp, ngrp);
    }

If the function exists, it's called. Should I send a patch to call the
function only if the hardware supports it ?
quoted
The real problem is vlan_dev.c, and applies to all devices.
That is was suggesting because other drivers may have the same issue
where they need to define rx_register for some hardware and control
usage of vlan by the feature bits.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help