Patchwork net: remove LLTX in atl2 driver

login
register
mail settings
Submitter Kevin Hao
Date Sept. 25, 2008, 12:35 a.m.
Message ID <1222302922-13200-1-git-send-email-kexin.hao@windriver.com>
Download mbox | patch
Permalink /patch/1408/
State Superseded
Delegated to: Jeff Garzik
Headers show

Comments

Kevin Hao - Sept. 25, 2008, 12:35 a.m.
When NETIF_F_LLTX is set, the atlx driver will use a private lock.
But in recent kernels this implementation seems redundant and
can cause problems where AF_PACKET sees things twice. Since
NETIF_F_LLTX is marked as deprecated and shouldn't be used in
new driver, this patch removes NETIF_F_LLTX and adds a mmiowb
before sending packet.

Signed-off-by: Kevin Hao <kexin.hao@windriver.com>
---
 drivers/net/atlx/atl2.c |   24 +-----------------------
 drivers/net/atlx/atl2.h |    1 -
 2 files changed, 1 insertions(+), 24 deletions(-)
Chris Snook - Sept. 25, 2008, 8:30 p.m.
Kevin Hao wrote:
> When NETIF_F_LLTX is set, the atlx driver will use a private lock.
> But in recent kernels this implementation seems redundant and
> can cause problems where AF_PACKET sees things twice. Since
> NETIF_F_LLTX is marked as deprecated and shouldn't be used in
> new driver, this patch removes NETIF_F_LLTX and adds a mmiowb
> before sending packet.
> 
> Signed-off-by: Kevin Hao <kexin.hao@windriver.com>

Can you explain a bit more concretely the problem this solves, and your testing? 
  Ultimately we'll want to merge this code with the atl1 code, so we need to be 
confident that we can and should make the same change there.

-- Chris

> ---
>  drivers/net/atlx/atl2.c |   24 +-----------------------
>  drivers/net/atlx/atl2.h |    1 -
>  2 files changed, 1 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/atlx/atl2.c b/drivers/net/atlx/atl2.c
> index b2995ac..3df4ee1 100644
> --- a/drivers/net/atlx/atl2.c
> +++ b/drivers/net/atlx/atl2.c
> @@ -116,7 +116,6 @@ static int __devinit atl2_sw_init(struct atl2_adapter *adapter)
>  	hw->max_frame_size = adapter->netdev->mtu;
>  
>  	spin_lock_init(&adapter->stats_lock);
> -	spin_lock_init(&adapter->tx_lock);
>  
>  	set_bit(__ATL2_DOWN, &adapter->flags);
>  
> @@ -749,11 +748,7 @@ static void atl2_down(struct atl2_adapter *adapter)
>  	 * reschedule our watchdog timer */
>  	set_bit(__ATL2_DOWN, &adapter->flags);
>  
> -#ifdef NETIF_F_LLTX
> -	netif_stop_queue(netdev);
> -#else
>  	netif_tx_disable(netdev);
> -#endif
>  
>  	/* reset MAC to disable all RX/TX */
>  	atl2_reset_hw(&adapter->hw);
> @@ -829,7 +824,6 @@ static inline int TxdFreeBytes(struct atl2_adapter *adapter)
>  static int atl2_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
>  {
>  	struct atl2_adapter *adapter = netdev_priv(netdev);
> -	unsigned long flags;
>  	struct tx_pkt_header *txph;
>  	u32 offset, copy_len;
>  	int txs_unused;
> @@ -845,16 +839,6 @@ static int atl2_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
>  		return NETDEV_TX_OK;
>  	}
>  
> -#ifdef NETIF_F_LLTX
> -	local_irq_save(flags);
> -	if (!spin_trylock(&adapter->tx_lock)) {
> -		/* Collision - tell upper layer to requeue */
> -		local_irq_restore(flags);
> -		return NETDEV_TX_LOCKED;
> -	}
> -#else
> -	spin_lock_irqsave(&adapter->tx_lock, flags);
> -#endif
>  	txs_unused = TxsFreeUnit(adapter);
>  	txbuf_unused = TxdFreeBytes(adapter);
>  
> @@ -862,7 +846,6 @@ static int atl2_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
>  		txs_unused < 1) {
>  		/* not enough resources */
>  		netif_stop_queue(netdev);
> -		spin_unlock_irqrestore(&adapter->tx_lock, flags);
>  		return NETDEV_TX_BUSY;
>  	}
>  
> @@ -908,8 +891,7 @@ static int atl2_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
>  	ATL2_WRITE_REGW(&adapter->hw, REG_MB_TXD_WR_IDX,
>  		(adapter->txd_write_ptr >> 2));
>  
> -	spin_unlock_irqrestore(&adapter->tx_lock, flags);
> -
> +	mmiowb();
>  	netdev->trans_start = jiffies;
>  	dev_kfree_skb_any(skb);
>  	return NETDEV_TX_OK;
> @@ -1447,10 +1429,6 @@ static int __devinit atl2_probe(struct pci_dev *pdev,
>  	netdev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
>  #endif
>  
> -#ifdef NETIF_F_LLTX
> -	netdev->features |= NETIF_F_LLTX;
> -#endif
> -
>  	/* Init PHY as early as possible due to power saving issue  */
>  	atl2_phy_init(&adapter->hw);
>  
> diff --git a/drivers/net/atlx/atl2.h b/drivers/net/atlx/atl2.h
> index 6e1f28f..09974df 100644
> --- a/drivers/net/atlx/atl2.h
> +++ b/drivers/net/atlx/atl2.h
> @@ -462,7 +462,6 @@ struct atl2_adapter {
>  	u16 link_duplex;
>  
>  	spinlock_t stats_lock;
> -	spinlock_t tx_lock;
>  
>  	struct work_struct reset_task;
>  	struct work_struct link_chg_task;

--
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
Jeff Garzik - Sept. 25, 2008, 8:55 p.m.
Chris Snook wrote:
> Kevin Hao wrote:
>> When NETIF_F_LLTX is set, the atlx driver will use a private lock.
>> But in recent kernels this implementation seems redundant and
>> can cause problems where AF_PACKET sees things twice. Since
>> NETIF_F_LLTX is marked as deprecated and shouldn't be used in
>> new driver, this patch removes NETIF_F_LLTX and adds a mmiowb
>> before sending packet.
>>
>> Signed-off-by: Kevin Hao <kexin.hao@windriver.com>
> 
> Can you explain a bit more concretely the problem this solves, and your 
> testing?  Ultimately we'll want to merge this code with the atl1 code, 
> so we need to be confident that we can and should make the same change 
> there.
> 
> -- Chris
> 
>> ---
>>  drivers/net/atlx/atl2.c |   24 +-----------------------
>>  drivers/net/atlx/atl2.h |    1 -
>>  2 files changed, 1 insertions(+), 24 deletions(-)

Not directly addressing your question, but LLTX is indeed deprecated and 
in general we want to move away from it.

	Jeff



--
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
Chris Snook - Sept. 25, 2008, 10:34 p.m.
Jeff Garzik wrote:
> Chris Snook wrote:
>> Kevin Hao wrote:
>>> When NETIF_F_LLTX is set, the atlx driver will use a private lock.
>>> But in recent kernels this implementation seems redundant and
>>> can cause problems where AF_PACKET sees things twice. Since
>>> NETIF_F_LLTX is marked as deprecated and shouldn't be used in
>>> new driver, this patch removes NETIF_F_LLTX and adds a mmiowb
>>> before sending packet.
>>>
>>> Signed-off-by: Kevin Hao <kexin.hao@windriver.com>
>>
>> Can you explain a bit more concretely the problem this solves, and 
>> your testing?  Ultimately we'll want to merge this code with the atl1 
>> code, so we need to be confident that we can and should make the same 
>> change there.
>>
>> -- Chris
>>
>>> ---
>>>  drivers/net/atlx/atl2.c |   24 +-----------------------
>>>  drivers/net/atlx/atl2.h |    1 -
>>>  2 files changed, 1 insertions(+), 24 deletions(-)
> 
> Not directly addressing your question, but LLTX is indeed deprecated and 
> in general we want to move away from it.
> 
>     Jeff

I'm all in favor of removing legacy cruft.  I want to know a little more about 
it so we can remove it from the atl1 code as well.  Ultimately most of this will 
be shared code.

-- Chris
--
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
Jay Cliburn - Sept. 25, 2008, 11:58 p.m.
On Thu, 25 Sep 2008 18:34:29 -0400
Chris Snook <csnook@redhat.com> wrote:

> 
> I'm all in favor of removing legacy cruft.  I want to know a little
> more about it so we can remove it from the atl1 code as well.
> Ultimately most of this will be shared code.

Yes, LLTX can (and should) be removed from the atl1 code.  I had an
exchange with Herbert Xu and David Miller on LKML a few weeks ago, and
they gave me a few tips on how to do it, but I haven't yet had the
time.  What takes an experienced netdev warrior about a half hour to
implement unfortunately takes me days of after-dayjob time as I track
through all the calls that that are affected and test the modified
driver.

Now back to atl2...  I'd like to hear from Kevin also whether he tested
the LLTX removal.  I'm building the driver with his patch as we speak,
and I'll test it myself this weekend, but Kevin I'd still like to know
if you tested the patch.

Thanks,
Jay
--
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
Kevin Hao - Sept. 26, 2008, 1:20 a.m.
On Thu, 2008-09-25 at 18:58 -0500, Jay Cliburn wrote:
> On Thu, 25 Sep 2008 18:34:29 -0400
> Chris Snook <csnook@redhat.com> wrote:
> 
> > 
> > I'm all in favor of removing legacy cruft.  I want to know a little
> > more about it so we can remove it from the atl1 code as well.
> > Ultimately most of this will be shared code.
> 
> Yes, LLTX can (and should) be removed from the atl1 code.  I had an
> exchange with Herbert Xu and David Miller on LKML a few weeks ago, and
> they gave me a few tips on how to do it, but I haven't yet had the
> time.  What takes an experienced netdev warrior about a half hour to
> implement unfortunately takes me days of after-dayjob time as I track
> through all the calls that that are affected and test the modified
> driver.
> 
> Now back to atl2...  I'd like to hear from Kevin also whether he tested
> the LLTX removal.  I'm building the driver with his patch as we speak,
> and I'll test it myself this weekend, but Kevin I'd still like to know
> if you tested the patch.

Yes, I have tested this driver on a Eee PC. It works well.

Thanks,
Kevin

> 
> Thanks,
> Jay
--
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
Chris Snook - Sept. 26, 2008, 1:24 a.m.
Kevin Hao wrote:
> On Thu, 2008-09-25 at 18:58 -0500, Jay Cliburn wrote:
>> On Thu, 25 Sep 2008 18:34:29 -0400
>> Chris Snook <csnook@redhat.com> wrote:
>>
>>> I'm all in favor of removing legacy cruft.  I want to know a little
>>> more about it so we can remove it from the atl1 code as well.
>>> Ultimately most of this will be shared code.
>> Yes, LLTX can (and should) be removed from the atl1 code.  I had an
>> exchange with Herbert Xu and David Miller on LKML a few weeks ago, and
>> they gave me a few tips on how to do it, but I haven't yet had the
>> time.  What takes an experienced netdev warrior about a half hour to
>> implement unfortunately takes me days of after-dayjob time as I track
>> through all the calls that that are affected and test the modified
>> driver.
>>
>> Now back to atl2...  I'd like to hear from Kevin also whether he tested
>> the LLTX removal.  I'm building the driver with his patch as we speak,
>> and I'll test it myself this weekend, but Kevin I'd still like to know
>> if you tested the patch.
> 
> Yes, I have tested this driver on a Eee PC. It works well.

Thanks, please mention this with your patch submissions.

-- Chris
--
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
Jay Cliburn - Sept. 26, 2008, 1:24 a.m.
On Thu, 25 Sep 2008 08:35:22 +0800
Kevin Hao <kexin.hao@windriver.com> wrote:

> When NETIF_F_LLTX is set, the atlx driver will use a private lock.
> But in recent kernels this implementation seems redundant and
> can cause problems where AF_PACKET sees things twice. Since
> NETIF_F_LLTX is marked as deprecated and shouldn't be used in
> new driver, this patch removes NETIF_F_LLTX and adds a mmiowb
> before sending packet.
> 
> Signed-off-by: Kevin Hao <kexin.hao@windriver.com>

Seems to work fine with large file (3.5 GiB) two-way scp and two-way
iperf runs.

[root@osprey ~]# ifconfig eth1
eth1      Link encap:Ethernet  HWaddr 00:13:74:00:5C:38  
          inet addr:192.168.1.33  Bcast:192.168.1.255  Mask:255.255.255.0
          inet6 addr: fe80::213:74ff:fe00:5c38/64 Scope:Link
          UP BROADCAST RUNNING MULTICAST  MTU:1492  Metric:1
          RX packets:10054352 errors:0 dropped:0 overruns:0 frame:1
          TX packets:12364268 errors:0 dropped:0 overruns:0 carrier:6
          collisions:0 txqueuelen:1000 
          RX bytes:11990462653 (11.1 GiB)  TX bytes:12351847745 (11.5 GiB)
          Memory:dbfc0000-dc000000 

Acked-by: Jay Cliburn <jacliburn@bellsouth.net>

> ---
>  drivers/net/atlx/atl2.c |   24 +-----------------------
>  drivers/net/atlx/atl2.h |    1 -
>  2 files changed, 1 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/atlx/atl2.c b/drivers/net/atlx/atl2.c
> index b2995ac..3df4ee1 100644
> --- a/drivers/net/atlx/atl2.c
> +++ b/drivers/net/atlx/atl2.c
> @@ -116,7 +116,6 @@ static int __devinit atl2_sw_init(struct
> atl2_adapter *adapter) hw->max_frame_size = adapter->netdev->mtu;
>  
>  	spin_lock_init(&adapter->stats_lock);
> -	spin_lock_init(&adapter->tx_lock);
>  
>  	set_bit(__ATL2_DOWN, &adapter->flags);
>  
> @@ -749,11 +748,7 @@ static void atl2_down(struct atl2_adapter
> *adapter)
>  	 * reschedule our watchdog timer */
>  	set_bit(__ATL2_DOWN, &adapter->flags);
>  
> -#ifdef NETIF_F_LLTX
> -	netif_stop_queue(netdev);
> -#else
>  	netif_tx_disable(netdev);
> -#endif
>  
>  	/* reset MAC to disable all RX/TX */
>  	atl2_reset_hw(&adapter->hw);
> @@ -829,7 +824,6 @@ static inline int TxdFreeBytes(struct
> atl2_adapter *adapter) static int atl2_xmit_frame(struct sk_buff
> *skb, struct net_device *netdev) {
>  	struct atl2_adapter *adapter = netdev_priv(netdev);
> -	unsigned long flags;
>  	struct tx_pkt_header *txph;
>  	u32 offset, copy_len;
>  	int txs_unused;
> @@ -845,16 +839,6 @@ static int atl2_xmit_frame(struct sk_buff *skb,
> struct net_device *netdev) return NETDEV_TX_OK;
>  	}
>  
> -#ifdef NETIF_F_LLTX
> -	local_irq_save(flags);
> -	if (!spin_trylock(&adapter->tx_lock)) {
> -		/* Collision - tell upper layer to requeue */
> -		local_irq_restore(flags);
> -		return NETDEV_TX_LOCKED;
> -	}
> -#else
> -	spin_lock_irqsave(&adapter->tx_lock, flags);
> -#endif
>  	txs_unused = TxsFreeUnit(adapter);
>  	txbuf_unused = TxdFreeBytes(adapter);
>  
> @@ -862,7 +846,6 @@ static int atl2_xmit_frame(struct sk_buff *skb,
> struct net_device *netdev) txs_unused < 1) {
>  		/* not enough resources */
>  		netif_stop_queue(netdev);
> -		spin_unlock_irqrestore(&adapter->tx_lock, flags);
>  		return NETDEV_TX_BUSY;
>  	}
>  
> @@ -908,8 +891,7 @@ static int atl2_xmit_frame(struct sk_buff *skb,
> struct net_device *netdev) ATL2_WRITE_REGW(&adapter->hw,
> REG_MB_TXD_WR_IDX, (adapter->txd_write_ptr >> 2));
>  
> -	spin_unlock_irqrestore(&adapter->tx_lock, flags);
> -
> +	mmiowb();
>  	netdev->trans_start = jiffies;
>  	dev_kfree_skb_any(skb);
>  	return NETDEV_TX_OK;
> @@ -1447,10 +1429,6 @@ static int __devinit atl2_probe(struct pci_dev
> *pdev, netdev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
>  #endif
>  
> -#ifdef NETIF_F_LLTX
> -	netdev->features |= NETIF_F_LLTX;
> -#endif
> -
>  	/* Init PHY as early as possible due to power saving issue
> */ atl2_phy_init(&adapter->hw);
>  
> diff --git a/drivers/net/atlx/atl2.h b/drivers/net/atlx/atl2.h
> index 6e1f28f..09974df 100644
> --- a/drivers/net/atlx/atl2.h
> +++ b/drivers/net/atlx/atl2.h
> @@ -462,7 +462,6 @@ struct atl2_adapter {
>  	u16 link_duplex;
>  
>  	spinlock_t stats_lock;
> -	spinlock_t tx_lock;
>  
>  	struct work_struct reset_task;
>  	struct work_struct link_chg_task;
--
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

Patch

diff --git a/drivers/net/atlx/atl2.c b/drivers/net/atlx/atl2.c
index b2995ac..3df4ee1 100644
--- a/drivers/net/atlx/atl2.c
+++ b/drivers/net/atlx/atl2.c
@@ -116,7 +116,6 @@  static int __devinit atl2_sw_init(struct atl2_adapter *adapter)
 	hw->max_frame_size = adapter->netdev->mtu;
 
 	spin_lock_init(&adapter->stats_lock);
-	spin_lock_init(&adapter->tx_lock);
 
 	set_bit(__ATL2_DOWN, &adapter->flags);
 
@@ -749,11 +748,7 @@  static void atl2_down(struct atl2_adapter *adapter)
 	 * reschedule our watchdog timer */
 	set_bit(__ATL2_DOWN, &adapter->flags);
 
-#ifdef NETIF_F_LLTX
-	netif_stop_queue(netdev);
-#else
 	netif_tx_disable(netdev);
-#endif
 
 	/* reset MAC to disable all RX/TX */
 	atl2_reset_hw(&adapter->hw);
@@ -829,7 +824,6 @@  static inline int TxdFreeBytes(struct atl2_adapter *adapter)
 static int atl2_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 {
 	struct atl2_adapter *adapter = netdev_priv(netdev);
-	unsigned long flags;
 	struct tx_pkt_header *txph;
 	u32 offset, copy_len;
 	int txs_unused;
@@ -845,16 +839,6 @@  static int atl2_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 		return NETDEV_TX_OK;
 	}
 
-#ifdef NETIF_F_LLTX
-	local_irq_save(flags);
-	if (!spin_trylock(&adapter->tx_lock)) {
-		/* Collision - tell upper layer to requeue */
-		local_irq_restore(flags);
-		return NETDEV_TX_LOCKED;
-	}
-#else
-	spin_lock_irqsave(&adapter->tx_lock, flags);
-#endif
 	txs_unused = TxsFreeUnit(adapter);
 	txbuf_unused = TxdFreeBytes(adapter);
 
@@ -862,7 +846,6 @@  static int atl2_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 		txs_unused < 1) {
 		/* not enough resources */
 		netif_stop_queue(netdev);
-		spin_unlock_irqrestore(&adapter->tx_lock, flags);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -908,8 +891,7 @@  static int atl2_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 	ATL2_WRITE_REGW(&adapter->hw, REG_MB_TXD_WR_IDX,
 		(adapter->txd_write_ptr >> 2));
 
-	spin_unlock_irqrestore(&adapter->tx_lock, flags);
-
+	mmiowb();
 	netdev->trans_start = jiffies;
 	dev_kfree_skb_any(skb);
 	return NETDEV_TX_OK;
@@ -1447,10 +1429,6 @@  static int __devinit atl2_probe(struct pci_dev *pdev,
 	netdev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
 #endif
 
-#ifdef NETIF_F_LLTX
-	netdev->features |= NETIF_F_LLTX;
-#endif
-
 	/* Init PHY as early as possible due to power saving issue  */
 	atl2_phy_init(&adapter->hw);
 
diff --git a/drivers/net/atlx/atl2.h b/drivers/net/atlx/atl2.h
index 6e1f28f..09974df 100644
--- a/drivers/net/atlx/atl2.h
+++ b/drivers/net/atlx/atl2.h
@@ -462,7 +462,6 @@  struct atl2_adapter {
 	u16 link_duplex;
 
 	spinlock_t stats_lock;
-	spinlock_t tx_lock;
 
 	struct work_struct reset_task;
 	struct work_struct link_chg_task;