[PATCH net-next v2 0/2] net: systemport: couple fixes

[PATCH net-next v2 0/2] net: systemport: couple fixes

[Florian Fainelli]

This patch fixes a few minor issues found in the SYSTEMPORT driver while
doing some more testing with its switching hardware.

Florian Fainelli (2):
  net: systemport: only update UMAC_CMD if something changed
  net: systemport: pad packets to a minimum of 64 bytes

 drivers/net/ethernet/broadcom/bcmsysport.c | 34 +++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH net-next v2 1/2] net: systemport: only update UMAC_CMD if something changed

[Florian Fainelli]

The link adjustment callback can be called as frequently as desired by
the PHY library, as such, let's avoid doing a Read/Modify/Write sequence
if nothing changed, which is more than likely since we are interfaced
with a switch device.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v2:
- rebased against latest net-next/master

 drivers/net/ethernet/broadcom/bcmsysport.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 4dc8d1e9829b..e118e7411ca4 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -959,15 +959,16 @@ static void bcm_sysport_adj_link(struct net_device *dev)
 	if (!phydev->pause)
 		cmd_bits |= CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE;
 
-	reg = umac_readl(priv, UMAC_CMD);
-	reg &= ~((CMD_SPEED_MASK << CMD_SPEED_SHIFT) |
-			CMD_HD_EN | CMD_RX_PAUSE_IGNORE |
-			CMD_TX_PAUSE_IGNORE);
-	reg |= cmd_bits;
-	umac_writel(priv, reg, UMAC_CMD);
+	if (changed) {
+		reg = umac_readl(priv, UMAC_CMD);
+		reg &= ~((CMD_SPEED_MASK << CMD_SPEED_SHIFT) |
+				CMD_HD_EN | CMD_RX_PAUSE_IGNORE |
+				CMD_TX_PAUSE_IGNORE);
+		reg |= cmd_bits;
+		umac_writel(priv, reg, UMAC_CMD);
 
-	if (changed)
 		phy_print_status(priv->phydev);
+	}
 }
 
 static int bcm_sysport_init_tx_ring(struct bcm_sysport_priv *priv,
-- 
1.9.1

Re: [PATCH net-next v2 1/2] net: systemport: only update UMAC_CMD if something changed

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon,  5 May 2014 12:20:51 -0700

> +		reg &= ~((CMD_SPEED_MASK << CMD_SPEED_SHIFT) |
> +				CMD_HD_EN | CMD_RX_PAUSE_IGNORE |
> +				CMD_TX_PAUSE_IGNORE);

While you're here use indentation more consistent with the rest of the
networking for this kind of expression.

On the second and third line, indent to the first column after the openning
parenthesis on the first line.

Thanks.

Re: [PATCH net-next v2 1/2] net: systemport: only update UMAC_CMD if something changed

[Florian Fainelli]

2014-05-07 12:59 GMT-07:00 David Miller <davem@davemloft.net>:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Mon,  5 May 2014 12:20:51 -0700
>
>> +             reg &= ~((CMD_SPEED_MASK << CMD_SPEED_SHIFT) |
>> +                             CMD_HD_EN | CMD_RX_PAUSE_IGNORE |
>> +                             CMD_TX_PAUSE_IGNORE);
>
> While you're here use indentation more consistent with the rest of the
> networking for this kind of expression.
>
> On the second and third line, indent to the first column after the openning
> parenthesis on the first line.

My email client does not render this properly apparently, so I do see
the same problem as you hinted, but the lines do really follow your
recommendation if you take a look at the raw patch in e.g: patchwork.
-- 
Florian

[PATCH net-next v2 2/2] net: systemport: pad packets to a minimum of 64 bytes

[Florian Fainelli]

The switch fabric which is used behind the Broadcom SYSTEMPORT Ethernet
controller will discard any incoming packet that is not 64 bytes or
more. Since the UniMAC hardware automatically pads up to the specified
length, we can simply ensure we instruct it to transmit >= 64 bytes
packets.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v2:
- use skb_padto() to avoid leaking packet contents

 drivers/net/ethernet/broadcom/bcmsysport.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index e118e7411ca4..1e228614b4dd 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -821,6 +821,7 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
 	struct bcm_sysport_cb *cb;
 	struct netdev_queue *txq;
 	struct dma_desc *desc;
+	unsigned int skb_len;
 	dma_addr_t mapping;
 	u32 len_status;
 	u16 queue;
@@ -848,7 +849,19 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
 		}
 	}
 
-	mapping = dma_map_single(kdev, skb->data, skb->len, DMA_TO_DEVICE);
+	/* The Ethernet switch we are interfaced with needs packets to be at
+	 * least 64 bytes otherwise they will be discarded when they enter
+	 * the switch port logic. The UniMAC hardware automatically pads if
+	 * instructed to do so.
+	 */
+	if (skb_padto(skb, 64)) {
+		ret = NETDEV_TX_OK;
+		goto out;
+	}
+
+	skb_len = skb->len < 64 ? 64 : skb->len;
+
+	mapping = dma_map_single(kdev, skb->data, skb_len, DMA_TO_DEVICE);
 	if (dma_mapping_error(kdev, mapping)) {
 		netif_err(priv, tx_err, dev, "DMA map failed at %p (len=%d)\n",
 				skb->data, skb->len);
@@ -860,14 +873,14 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
 	cb = &ring->cbs[ring->curr_desc];
 	cb->skb = skb;
 	dma_unmap_addr_set(cb, dma_addr, mapping);
-	dma_unmap_len_set(cb, dma_len, skb->len);
+	dma_unmap_len_set(cb, dma_len, skb_len);
 
 	/* Fetch a descriptor entry from our pool */
 	desc = ring->desc_cpu;
 
 	desc->addr_lo = lower_32_bits(mapping);
 	len_status = upper_32_bits(mapping) & DESC_ADDR_HI_MASK;
-	len_status |= (skb->len << DESC_LEN_SHIFT);
+	len_status |= (skb_len << DESC_LEN_SHIFT);
 	len_status |= (DESC_SOP | DESC_EOP | TX_STATUS_APP_CRC) <<
 			DESC_STATUS_SHIFT;
 	if (skb->ip_summed == CHECKSUM_PARTIAL)
-- 
1.9.1

RE: [PATCH net-next v2 2/2] net: systemport: pad packets to a minimum of 64 bytes

[David Laight]

From: Florian Fainelli
> The switch fabric which is used behind the Broadcom SYSTEMPORT Ethernet
> controller will discard any incoming packet that is not 64 bytes or
> more. Since the UniMAC hardware automatically pads up to the specified
> length, we can simply ensure we instruct it to transmit >= 64 bytes
> packets.

That is a strange description...
All ethernet packets are padded out to 64 bytes, but IIRC that includes the CRC.
If the UniMAC hardware automatically pads, then why do you need to do anything
in the driver?
If the actual problem (hinted at) is that the hardware is padding with the
'random' bytes after the end of the frame, then that should be in the comment.

...
> @@ -848,7 +849,19 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
>  		}
>  	}
> 
> -	mapping = dma_map_single(kdev, skb->data, skb->len, DMA_TO_DEVICE);
> +	/* The Ethernet switch we are interfaced with needs packets to be at
> +	 * least 64 bytes otherwise they will be discarded when they enter
> +	 * the switch port logic. The UniMAC hardware automatically pads if
> +	 * instructed to do so.
> +	 */
> +	if (skb_padto(skb, 64)) {
> +		ret = NETDEV_TX_OK;

Isn't that a silent tx discard, and possibly a skb/memory leak?

> +		goto out;
> +	}
> +
> +	skb_len = skb->len < 64 ? 64 : skb->len;
> +
> +	mapping = dma_map_single(kdev, skb->data, skb_len, DMA_TO_DEVICE);
>  	if (dma_mapping_error(kdev, mapping)) {
>  		netif_err(priv, tx_err, dev, "DMA map failed at %p (len=%d)\n",
>  				skb->data, skb->len);
> @@ -860,14 +873,14 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
>  	cb = &ring->cbs[ring->curr_desc];
>  	cb->skb = skb;
>  	dma_unmap_addr_set(cb, dma_addr, mapping);
> -	dma_unmap_len_set(cb, dma_len, skb->len);
> +	dma_unmap_len_set(cb, dma_len, skb_len);
> 
>  	/* Fetch a descriptor entry from our pool */
>  	desc = ring->desc_cpu;
> 
>  	desc->addr_lo = lower_32_bits(mapping);
>  	len_status = upper_32_bits(mapping) & DESC_ADDR_HI_MASK;
> -	len_status |= (skb->len << DESC_LEN_SHIFT);
> +	len_status |= (skb_len << DESC_LEN_SHIFT);
>  	len_status |= (DESC_SOP | DESC_EOP | TX_STATUS_APP_CRC) <<
>  			DESC_STATUS_SHIFT;
>  	if (skb->ip_summed == CHECKSUM_PARTIAL)
> --

	David

Re: [PATCH net-next v2 2/2] net: systemport: pad packets to a minimum of 64 bytes

[Florian Fainelli]

2014-05-06 2:31 GMT-07:00 David Laight <David.Laight@aculab.com>:
> From: Florian Fainelli
>> The switch fabric which is used behind the Broadcom SYSTEMPORT Ethernet
>> controller will discard any incoming packet that is not 64 bytes or
>> more. Since the UniMAC hardware automatically pads up to the specified
>> length, we can simply ensure we instruct it to transmit >= 64 bytes
>> packets.
>
> That is a strange description...
> All ethernet packets are padded out to 64 bytes, but IIRC that includes the CRC.
> If the UniMAC hardware automatically pads, then why do you need to do anything
> in the driver?

I think this explanation is not very good, I should have written something like:

"UniMAC will transmit up to the specified number of bytes" or something similar.

> If the actual problem (hinted at) is that the hardware is padding with the
> 'random' bytes after the end of the frame, then that should be in the comment.
>
> ...
>> @@ -848,7 +849,19 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
>>               }
>>       }
>>
>> -     mapping = dma_map_single(kdev, skb->data, skb->len, DMA_TO_DEVICE);
>> +     /* The Ethernet switch we are interfaced with needs packets to be at
>> +      * least 64 bytes otherwise they will be discarded when they enter
>> +      * the switch port logic. The UniMAC hardware automatically pads if
>> +      * instructed to do so.
>> +      */
>> +     if (skb_padto(skb, 64)) {
>> +             ret = NETDEV_TX_OK;
>
> Isn't that a silent tx discard, and possibly a skb/memory leak?

skb_padto() returns !=0 if it failed to pad and also frees the SKB
(see the comment in include/linux/skbuff.h).

>
>> +             goto out;
>> +     }
>> +
>> +     skb_len = skb->len < 64 ? 64 : skb->len;
>> +
>> +     mapping = dma_map_single(kdev, skb->data, skb_len, DMA_TO_DEVICE);
>>       if (dma_mapping_error(kdev, mapping)) {
>>               netif_err(priv, tx_err, dev, "DMA map failed at %p (len=%d)\n",
>>                               skb->data, skb->len);
>> @@ -860,14 +873,14 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
>>       cb = &ring->cbs[ring->curr_desc];
>>       cb->skb = skb;
>>       dma_unmap_addr_set(cb, dma_addr, mapping);
>> -     dma_unmap_len_set(cb, dma_len, skb->len);
>> +     dma_unmap_len_set(cb, dma_len, skb_len);
>>
>>       /* Fetch a descriptor entry from our pool */
>>       desc = ring->desc_cpu;
>>
>>       desc->addr_lo = lower_32_bits(mapping);
>>       len_status = upper_32_bits(mapping) & DESC_ADDR_HI_MASK;
>> -     len_status |= (skb->len << DESC_LEN_SHIFT);
>> +     len_status |= (skb_len << DESC_LEN_SHIFT);
>>       len_status |= (DESC_SOP | DESC_EOP | TX_STATUS_APP_CRC) <<
>>                       DESC_STATUS_SHIFT;
>>       if (skb->ip_summed == CHECKSUM_PARTIAL)
>> --
>
>         David
>
>
>



-- 
Florian

Re: [PATCH net-next v2 2/2] net: systemport: pad packets to a minimum of 64 bytes

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon,  5 May 2014 12:20:52 -0700

> The switch fabric which is used behind the Broadcom SYSTEMPORT Ethernet
> controller will discard any incoming packet that is not 64 bytes or
> more. Since the UniMAC hardware automatically pads up to the specified
> length, we can simply ensure we instruct it to transmit >= 64 bytes
> packets.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Please resubmit with an adjusted commit log as discussed in this
thread, thanks.