| Message ID | 1268630132-10410-2-git-send-email-cooldavid@cooldavid.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
what does the spinlock protect ? On Sun, Mar 14, 2010 at 10:15 PM, <cooldavid@cooldavid.org> wrote: > From: Guo-Fu Tseng <cooldavid@cooldavid.org> > > Adding a lock to prevent modifying the vlgrp structure while receiving > VLAN packet. > > Signed-off-by: Guo-Fu Tseng <cooldavid@cooldavid.org> > --- > drivers/net/jme.c | 6 ++++++ > drivers/net/jme.h | 1 + > 2 files changed, 7 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/jme.c b/drivers/net/jme.c > index 3da390a..f10d9db 100644 > --- a/drivers/net/jme.c > +++ b/drivers/net/jme.c > @@ -942,11 +942,14 @@ jme_alloc_and_feed_skb(struct jme_adapter *jme, int idx) > skb->ip_summed = CHECKSUM_NONE; > > if (rxdesc->descwb.flags & cpu_to_le16(RXWBFLAG_TAGON)) { > + spin_lock(&jme->vlgrp_lock); > if (jme->vlgrp) { > jme->jme_vlan_rx(skb, jme->vlgrp, > le16_to_cpu(rxdesc->descwb.vlan)); > + spin_unlock(&jme->vlgrp_lock); > NET_STAT(jme).rx_bytes += 4; > } else { > + spin_unlock(&jme->vlgrp_lock); > dev_kfree_skb(skb); > } > } else { > @@ -2092,7 +2095,9 @@ jme_vlan_rx_register(struct net_device *netdev, struct vlan_group *grp) > { > struct jme_adapter *jme = netdev_priv(netdev); > > + spin_lock_bh(&jme->vlgrp_lock); > jme->vlgrp = grp; > + spin_unlock_bh(&jme->vlgrp_lock); > } > > static void > @@ -2759,6 +2764,7 @@ jme_init_one(struct pci_dev *pdev, > spin_lock_init(&jme->phy_lock); > spin_lock_init(&jme->macaddr_lock); > spin_lock_init(&jme->rxmcs_lock); > + spin_lock_init(&jme->vlgrp_lock); > > atomic_set(&jme->link_changing, 1); > atomic_set(&jme->rx_cleaning, 1); > diff --git a/drivers/net/jme.h b/drivers/net/jme.h > index 251abed..fbde5c5 100644 > --- a/drivers/net/jme.h > +++ b/drivers/net/jme.h > @@ -420,6 +420,7 @@ struct jme_adapter { > spinlock_t phy_lock; > spinlock_t macaddr_lock; > spinlock_t rxmcs_lock; > + spinlock_t vlgrp_lock; > struct tasklet_struct rxempty_task; > struct tasklet_struct rxclean_task; > struct tasklet_struct txclean_task; > -- > 1.6.4.4 > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Laurent: The vlan_rx_register is called through ioctl. And the packet feeding is called in the tasklet. I see no lock in register_vlan_dev(), register_vlan_device(), and vlan_ioctl_handler() which is related to the vlan_hwaccel_receive_skb(), vlan_hwaccel_rx(). It prevents the vlgrp pointer be modified while trying to feed the packet. Ex: > if (jme->vlgrp) { True. vlan_rx_register called, vlgrp set to NULL. > jme->jme_vlan_rx(skb, jme->vlgrp, > le16_to_cpu(rxdesc->descwb.vlan)); passing vlgrp with the value "NULL" to jme_vlan_rx. Or it might even be modified during the vlan_hwaccel_{receive_skb|rx} call. Please correct me if I'm wrong. On Mon, 15 Mar 2010 11:22:48 -0700, Laurent Chavey wrote > what does the spinlock protect ? > -- Guo-Fu Tseng -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: "Guo-Fu Tseng" <cooldavid@cooldavid.org> Date: Tue, 16 Mar 2010 03:13:33 +0800 > The vlan_rx_register is called through ioctl. > And the packet feeding is called in the tasklet. > I see no lock in register_vlan_dev(), register_vlan_device(), and vlan_ioctl_handler() > which is related to the vlan_hwaccel_receive_skb(), vlan_hwaccel_rx(). > > It prevents the vlgrp pointer be modified while trying to feed the packet. This is not how you fix this. Adding a new lock to your hot code path of packet receiving is the last thing you should be doing. Instead, do what other drivers do, take down the device and bring it back up again when changing the ->vlgrp pointer. See drivers/net/tg3.c:tg3_vlan_rx_register() for an example. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 15 Mar 2010 15:53:50 -0700 (PDT), David Miller wrote > From: "Guo-Fu Tseng" <cooldavid@cooldavid.org> > Date: Tue, 16 Mar 2010 03:13:33 +0800 > > > The vlan_rx_register is called through ioctl. > > And the packet feeding is called in the tasklet. > > I see no lock in register_vlan_dev(), register_vlan_device(), and vlan_ioctl_handler() > > which is related to the vlan_hwaccel_receive_skb(), vlan_hwaccel_rx(). > > > > It prevents the vlgrp pointer be modified while trying to feed the packet. > > This is not how you fix this. Adding a new lock to your hot code > path of packet receiving is the last thing you should be doing. > > Instead, do what other drivers do, take down the device and bring it > back up again when changing the ->vlgrp pointer. > > See drivers/net/tg3.c:tg3_vlan_rx_register() for an example. I see. I'll work on it. -- Guo-Fu Tseng -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/jme.c b/drivers/net/jme.c index 3da390a..f10d9db 100644 --- a/drivers/net/jme.c +++ b/drivers/net/jme.c @@ -942,11 +942,14 @@ jme_alloc_and_feed_skb(struct jme_adapter *jme, int idx) skb->ip_summed = CHECKSUM_NONE; if (rxdesc->descwb.flags & cpu_to_le16(RXWBFLAG_TAGON)) { + spin_lock(&jme->vlgrp_lock); if (jme->vlgrp) { jme->jme_vlan_rx(skb, jme->vlgrp, le16_to_cpu(rxdesc->descwb.vlan)); + spin_unlock(&jme->vlgrp_lock); NET_STAT(jme).rx_bytes += 4; } else { + spin_unlock(&jme->vlgrp_lock); dev_kfree_skb(skb); } } else { @@ -2092,7 +2095,9 @@ jme_vlan_rx_register(struct net_device *netdev, struct vlan_group *grp) { struct jme_adapter *jme = netdev_priv(netdev); + spin_lock_bh(&jme->vlgrp_lock); jme->vlgrp = grp; + spin_unlock_bh(&jme->vlgrp_lock); } static void @@ -2759,6 +2764,7 @@ jme_init_one(struct pci_dev *pdev, spin_lock_init(&jme->phy_lock); spin_lock_init(&jme->macaddr_lock); spin_lock_init(&jme->rxmcs_lock); + spin_lock_init(&jme->vlgrp_lock); atomic_set(&jme->link_changing, 1); atomic_set(&jme->rx_cleaning, 1); diff --git a/drivers/net/jme.h b/drivers/net/jme.h index 251abed..fbde5c5 100644 --- a/drivers/net/jme.h +++ b/drivers/net/jme.h @@ -420,6 +420,7 @@ struct jme_adapter { spinlock_t phy_lock; spinlock_t macaddr_lock; spinlock_t rxmcs_lock; + spinlock_t vlgrp_lock; struct tasklet_struct rxempty_task; struct tasklet_struct rxclean_task; struct tasklet_struct txclean_task;