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

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

[Florian Fainelli]

Hi David,

This patch set 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 | 32 +++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

-- 
1.9.1

[PATCH net-next v4 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 v4:
- re-align 2nd and 3rd lines properly according to the network style

Changes in v3:
- attempted to re-align the CMD_HD_EN lines but failed miserably

Changes in v2:
- respin

 drivers/net/ethernet/broadcom/bcmsysport.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 4dc8d1e9829b..ac42a9e91f3a 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) |
+	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);
+		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

[PATCH net-next v4 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 transmits up to the specified size, just
make sure that we properly pad the end of the packet with zeroes using
skb_padto() and set a minimum packet length of 64 bytes.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v4:
- respin due to first patch

Changes in v3:
- reword the commit message and comment to explain the special hardware
  requirements
- print skb_len instead of skb->len in the DMA map failure error

Changes in v2:
- add missing skb_padto() to zero-out packets

 drivers/net/ethernet/broadcom/bcmsysport.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index ac42a9e91f3a..dbae674384ea 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,10 +849,22 @@ 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 transmits up to the
+	 * specified length.
+	 */
+	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);
+				skb->data, skb_len);
 		ret = NETDEV_TX_OK;
 		goto out;
 	}
@@ -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 v4 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 transmits up to the specified size, just
> make sure that we properly pad the end of the packet with zeroes using
> skb_padto() and set a minimum packet length of 64 bytes.

Surely the above should just be:
    "Pad transmit packets to the minimum length (64 bytes)."
All ethernet packets should be padded to 64 bytes (including the CRC).

...
> @@ -848,10 +849,22 @@ 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 transmits up to the
> +	 * specified length.
> +	 */
> +	if (skb_padto(skb, 64)) {

Shouldn't that be 60?
I presume there is an appropriate constant defined for it as well.

> +		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);
> +				skb->data, skb_len);
>  		ret = NETDEV_TX_OK;
>  		goto out;
>  	}
...

	david

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

[Florian Fainelli]

2014-05-12 5:45 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 transmits up to the specified size, just
>> make sure that we properly pad the end of the packet with zeroes using
>> skb_padto() and set a minimum packet length of 64 bytes.
>
> Surely the above should just be:
>     "Pad transmit packets to the minimum length (64 bytes)."
> All ethernet packets should be padded to 64 bytes (including the CRC).

Sure, I can definitively simplify the commit message.

>
> ...
>> @@ -848,10 +849,22 @@ 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 transmits up to the
>> +      * specified length.
>> +      */
>> +     if (skb_padto(skb, 64)) {
>
> Shouldn't that be 60?

It should, absolutely, and that is ETH_ZLEN. Thanks!

> I presume there is an appropriate constant defined for it as well.
>
>> +             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);
>> +                             skb->data, skb_len);
>>               ret = NETDEV_TX_OK;
>>               goto out;
>>       }
> ...
>
>         david
>
>



-- 
Florian

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

[Florian Fainelli]

2014-05-13 17:18 GMT-07:00 Florian Fainelli <f.fainelli@gmail.com>:
> 2014-05-12 5:45 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 transmits up to the specified size, just
>>> make sure that we properly pad the end of the packet with zeroes using
>>> skb_padto() and set a minimum packet length of 64 bytes.
>>
>> Surely the above should just be:
>>     "Pad transmit packets to the minimum length (64 bytes)."
>> All ethernet packets should be padded to 64 bytes (including the CRC).
>
> Sure, I can definitively simplify the commit message.
>
>>
>> ...
>>> @@ -848,10 +849,22 @@ 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 transmits up to the
>>> +      * specified length.
>>> +      */
>>> +     if (skb_padto(skb, 64)) {
>>
>> Shouldn't that be 60?
>
> It should, absolutely, and that is ETH_ZLEN. Thanks!

In fact, no, despite the Ethernet MAC appending the CRC, the minimum
packet length from the DMA perspective really needs to be 64, as the
hardware inserted CRC is not accounted for to generate the End of
Packet signal. That just translates into ETH_ZLEN + ETH_FCS_LEN
anyway.

>
>> I presume there is an appropriate constant defined for it as well.
>>
>>> +             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);
>>> +                             skb->data, skb_len);
>>>               ret = NETDEV_TX_OK;
>>>               goto out;
>>>       }
>> ...
>>
>>         david
>>
>>
>
>
>
> --
> Florian



-- 
Florian

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

[David Laight]

From: Florian Fainelli 
...
> >>> +     if (skb_padto(skb, 64)) {
> >>
> >> Shouldn't that be 60?
> >
> > It should, absolutely, and that is ETH_ZLEN. Thanks!
> 
> In fact, no, despite the Ethernet MAC appending the CRC, the minimum
> packet length from the DMA perspective really needs to be 64, as the
> hardware inserted CRC is not accounted for to generate the End of
> Packet signal. That just translates into ETH_ZLEN + ETH_FCS_LEN
> anyway.

Hmmm.... That really doesn't sound right.
The CRC can't be replacing the last 4 bytes of the skb - otherwise
longer packets would be corrupted.
So I'm sure that you are padding frames to 68 bytes.
While a typical IP implementation probably ignores pad bytes after
any length frame, other protocols will only expect padding on short
frames - and then only to the minimum frame length.

	David

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

[Florian Fainelli]

2014-05-14 1:51 GMT-07:00 David Laight <David.Laight@aculab.com>:
> From: Florian Fainelli
> ...
>> >>> +     if (skb_padto(skb, 64)) {
>> >>
>> >> Shouldn't that be 60?
>> >
>> > It should, absolutely, and that is ETH_ZLEN. Thanks!
>>
>> In fact, no, despite the Ethernet MAC appending the CRC, the minimum
>> packet length from the DMA perspective really needs to be 64, as the
>> hardware inserted CRC is not accounted for to generate the End of
>> Packet signal. That just translates into ETH_ZLEN + ETH_FCS_LEN
>> anyway.
>
> Hmmm.... That really doesn't sound right.
> The CRC can't be replacing the last 4 bytes of the skb - otherwise
> longer packets would be corrupted.
> So I'm sure that you are padding frames to 68 bytes.

Yes that's what I am doing. I was confused because I enabled the
switch tagging, which inserts a 4bytes tag between the Ethernet SA and
the Ethertype field. That specific switch tag length is not taken into
account by the switch to determine if a packet is runt, because the
tag is extracted before the packet enters the parsing logic. The
minimum frame length really needs to be 64 bytes with the FCS, but
without the switch tag inserted, so if we want to be safe, regardless
of the switch tag being inserted or not, the minimum frame length
needs to be 68 bytes with the FCS.

> While a typical IP implementation probably ignores pad bytes after
> any length frame, other protocols will only expect padding on short
> frames - and then only to the minimum frame length.
>
>         David
>



-- 
Florian

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

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Fri,  9 May 2014 13:16:37 -0700

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

Please address David Laight's feedback on patch #2 and resubmit.

Thank you.