[PATCH net-next 0/4] net: systemport: misc fixes

[PATCH net-next 0/4] net: systemport: misc fixes

[Florian Fainelli]

Hi David,

This patch series contains some misc fixes for the SYSTEMPORT driver.

Thanks!

Florian Fainelli (4):
  net: systemport: fix transmit locking scheme
  net: systemport: correctly check for RX_STATUS_OVFLOW flag
  net: systemport: fix comment about HW prepending 2bytes
  net: systemport: start with carrier off

 drivers/net/ethernet/broadcom/bcmsysport.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

-- 
1.9.1

[PATCH net-next 1/4] net: systemport: fix transmit locking scheme

[Florian Fainelli]

Our transmit locking scheme did not account for the TX ring full
interrupt. If a TX ring full interrupt fires while we are attempting to
transmit, we will cause a deadlock to occur. Fix this by making sure
that we properly disable interrupts while acquiring the spinlock.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 25982b02f0ea..4dfc93fe9744 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -637,10 +637,11 @@ static unsigned int bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
 					   struct bcm_sysport_tx_ring *ring)
 {
 	unsigned int released;
+	unsigned long flags;
 
-	spin_lock(&ring->lock);
+	spin_lock_irqsave(&ring->lock, flags);
 	released = __bcm_sysport_tx_reclaim(priv, ring);
-	spin_unlock(&ring->lock);
+	spin_unlock_irqrestore(&ring->lock, flags);
 
 	return released;
 }
@@ -822,6 +823,7 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
 	struct netdev_queue *txq;
 	struct dma_desc *desc;
 	unsigned int skb_len;
+	unsigned long flags;
 	dma_addr_t mapping;
 	u32 len_status;
 	u16 queue;
@@ -831,8 +833,8 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
 	txq = netdev_get_tx_queue(dev, queue);
 	ring = &priv->tx_rings[queue];
 
-	/* lock against tx reclaim in BH context */
-	spin_lock(&ring->lock);
+	/* lock against tx reclaim in BH context and TX ring full interrupt */
+	spin_lock_irqsave(&ring->lock, flags);
 	if (unlikely(ring->desc_count == 0)) {
 		netif_tx_stop_queue(txq);
 		netdev_err(dev, "queue %d awake and ring full!\n", queue);
@@ -914,7 +916,7 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
 
 	ret = NETDEV_TX_OK;
 out:
-	spin_unlock(&ring->lock);
+	spin_unlock_irqrestore(&ring->lock, flags);
 	return ret;
 }
 
-- 
1.9.1

[PATCH net-next 2/4] net: systemport: correctly check for RX_STATUS_OVFLOW flag

[Florian Fainelli]

We were missing an and comparison with status to check whether
RX_STATUS_OVFLOW is asserted or not in the per-packet status word, fix
that.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 4dfc93fe9744..fc84aba0cde0 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -514,7 +514,7 @@ static unsigned int bcm_sysport_desc_rx(struct bcm_sysport_priv *priv,
 
 		if (unlikely(status & (RX_STATUS_ERR | RX_STATUS_OVFLOW))) {
 			netif_err(priv, rx_err, ndev, "error packet\n");
-			if (RX_STATUS_OVFLOW)
+			if (status & RX_STATUS_OVFLOW)
 				ndev->stats.rx_over_errors++;
 			ndev->stats.rx_dropped++;
 			ndev->stats.rx_errors++;
-- 
1.9.1

[PATCH net-next 3/4] net: systemport: fix comment about HW prepending 2bytes

[Florian Fainelli]

The comment about how the hardware prepends 2bytes to align the IP
header on a 4-byte boundary was not correct, fix that.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index fc84aba0cde0..54b51db0caf3 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -528,9 +528,9 @@ static unsigned int bcm_sysport_desc_rx(struct bcm_sysport_priv *priv,
 		if (likely(status & DESC_L4_CSUM))
 			skb->ip_summed = CHECKSUM_UNNECESSARY;
 
-		/* Hardware pre-pends packets with 2bytes between Ethernet
-		 * and IP header plus we have the Receive Status Block, strip
-		 * off all of this from the SKB.
+		/* Hardware pre-pends packets with 2bytes before Ethernet
+		 * header plus we have the Receive Status Block, strip off all
+		 * of this from the SKB.
 		 */
 		skb_pull(skb, sizeof(*rsb) + 2);
 		len -= (sizeof(*rsb) + 2);
-- 
1.9.1

[PATCH net-next 4/4] net: systemport: start with carrier off

[Florian Fainelli]

The SYSTEMPORT driver uses libphy to determine the carrier state, so
make sure we start with a carrier off until libphy has completed the
link training process.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 54b51db0caf3..141160ef249a 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -1595,6 +1595,9 @@ static int bcm_sysport_probe(struct platform_device *pdev)
 	 */
 	dev->flags &= ~IFF_MULTICAST;
 
+	/* libphy will adjust the link state accordingly */
+	netif_carrier_off(dev);
+
 	ret = register_netdev(dev);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register net_device\n");
-- 
1.9.1

Re: [PATCH net-next 0/4] net: systemport: misc fixes

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Thu,  5 Jun 2014 10:22:14 -0700

> This patch series contains some misc fixes for the SYSTEMPORT driver.

Series applied, thanks Florian.

RE: [PATCH net-next 1/4] net: systemport: fix transmit locking scheme

[David Laight]

From: Florian Fainelli
> Our transmit locking scheme did not account for the TX ring full
> interrupt. If a TX ring full interrupt fires while we are attempting to
> transmit, we will cause a deadlock to occur. Fix this by making sure
> that we properly disable interrupts while acquiring the spinlock.

Presumably you mean a 'TX complete' interrupt?

It ought to be possible to arrange the logic so that you don't
need to disable interrupts for the entirety of the tx reclaim 
and tx setup code.

If you have an absolutely accurate count of the number of
tx descriptors required, even the tx setup code probably only needs
the ring's lock while it advances the write pointer.
It can fill in the ring entries after releasing the lock.

	David

> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 25982b02f0ea..4dfc93fe9744 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -637,10 +637,11 @@ static unsigned int bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
>  					   struct bcm_sysport_tx_ring *ring)
>  {
>  	unsigned int released;
> +	unsigned long flags;
> 
> -	spin_lock(&ring->lock);
> +	spin_lock_irqsave(&ring->lock, flags);
>  	released = __bcm_sysport_tx_reclaim(priv, ring);
> -	spin_unlock(&ring->lock);
> +	spin_unlock_irqrestore(&ring->lock, flags);
> 
>  	return released;
>  }
> @@ -822,6 +823,7 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
>  	struct netdev_queue *txq;
>  	struct dma_desc *desc;
>  	unsigned int skb_len;
> +	unsigned long flags;
>  	dma_addr_t mapping;
>  	u32 len_status;
>  	u16 queue;
> @@ -831,8 +833,8 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
>  	txq = netdev_get_tx_queue(dev, queue);
>  	ring = &priv->tx_rings[queue];
> 
> -	/* lock against tx reclaim in BH context */
> -	spin_lock(&ring->lock);
> +	/* lock against tx reclaim in BH context and TX ring full interrupt */
> +	spin_lock_irqsave(&ring->lock, flags);
>  	if (unlikely(ring->desc_count == 0)) {
>  		netif_tx_stop_queue(txq);
>  		netdev_err(dev, "queue %d awake and ring full!\n", queue);
> @@ -914,7 +916,7 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
> 
>  	ret = NETDEV_TX_OK;
>  out:
> -	spin_unlock(&ring->lock);
> +	spin_unlock_irqrestore(&ring->lock, flags);
>  	return ret;
>  }
> 
> --
> 1.9.1
> 
> --
> 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

Re: [PATCH net-next 1/4] net: systemport: fix transmit locking scheme

[Florian Fainelli]

2014-06-06 2:29 GMT-07:00 David Laight <David.Laight@aculab.com>:
> From: Florian Fainelli
>> Our transmit locking scheme did not account for the TX ring full
>> interrupt. If a TX ring full interrupt fires while we are attempting to
>> transmit, we will cause a deadlock to occur. Fix this by making sure
>> that we properly disable interrupts while acquiring the spinlock.
>
> Presumably you mean a 'TX complete' interrupt?

There is an additional interrupt that gets fired when one of the TX
ring gets full, but it is in bcm_sysport_rx_isr() since it hooked to
the first interrupt line we have.

>
> It ought to be possible to arrange the logic so that you don't
> need to disable interrupts for the entirety of the tx reclaim
> and tx setup code.
>
> If you have an absolutely accurate count of the number of
> tx descriptors required, even the tx setup code probably only needs
> the ring's lock while it advances the write pointer.
> It can fill in the ring entries after releasing the lock.

Right, I am currently experimenting with that and plan to remove the
spinlock entirely, or grab it for as little time as possible.

>
>         David
>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  drivers/net/ethernet/broadcom/bcmsysport.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
>> index 25982b02f0ea..4dfc93fe9744 100644
>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>> @@ -637,10 +637,11 @@ static unsigned int bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
>>                                          struct bcm_sysport_tx_ring *ring)
>>  {
>>       unsigned int released;
>> +     unsigned long flags;
>>
>> -     spin_lock(&ring->lock);
>> +     spin_lock_irqsave(&ring->lock, flags);
>>       released = __bcm_sysport_tx_reclaim(priv, ring);
>> -     spin_unlock(&ring->lock);
>> +     spin_unlock_irqrestore(&ring->lock, flags);
>>
>>       return released;
>>  }
>> @@ -822,6 +823,7 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
>>       struct netdev_queue *txq;
>>       struct dma_desc *desc;
>>       unsigned int skb_len;
>> +     unsigned long flags;
>>       dma_addr_t mapping;
>>       u32 len_status;
>>       u16 queue;
>> @@ -831,8 +833,8 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
>>       txq = netdev_get_tx_queue(dev, queue);
>>       ring = &priv->tx_rings[queue];
>>
>> -     /* lock against tx reclaim in BH context */
>> -     spin_lock(&ring->lock);
>> +     /* lock against tx reclaim in BH context and TX ring full interrupt */
>> +     spin_lock_irqsave(&ring->lock, flags);
>>       if (unlikely(ring->desc_count == 0)) {
>>               netif_tx_stop_queue(txq);
>>               netdev_err(dev, "queue %d awake and ring full!\n", queue);
>> @@ -914,7 +916,7 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
>>
>>       ret = NETDEV_TX_OK;
>>  out:
>> -     spin_unlock(&ring->lock);
>> +     spin_unlock_irqrestore(&ring->lock, flags);
>>       return ret;
>>  }
>>
>> --
>> 1.9.1
>>
>> --
>> 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
>
>



-- 
Florian