[PATCH net-next 0/2] r6040: misc fixes

[PATCH net-next 0/2] r6040: misc fixes

[Florian Fainelli]

Hi David,

Here are two small fixes, patch 1 could potentially be backported to stable
trees since it affects MDIO operations.

Thanks!

Florian Fainelli (2):
  r6040: add delays in MDIO read/write polling loops
  r6040: use ETH_ZLEN instead of MISR for SKB length checking

 drivers/net/ethernet/rdc/r6040.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
1.8.3.2

[PATCH net-next 1/2] r6040: add delays in MDIO read/write polling loops

[Florian Fainelli]

On newer and faster machines (Vortex X86DX) using the r6040 driver, it
was noticed that the driver was returning an error during probing traced
down to being the MDIO bus probing and the inability to complete a MDIO
read operation in time. It turns out that the MDIO operations on these
faster machines usually complete after ~2140 iterations which is bigger
than 2048 (MAC_DEF_TIMEOUT) and results in spurious timeouts depending
on the system load.

Update r6040_phy_read() and r6040_phy_write() to include a 1
micro second delay in each busy-looping iteration of the loop which is a
much safer operation than incrementing MAC_DEF_TIMEOUT.

Reported-by: Nils Koehler <nils.koehler@ibt-interfaces.de>
Reported-by: Daniel Goertzen <daniel.goertzen@gmail.com>
Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
 drivers/net/ethernet/rdc/r6040.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/rdc/r6040.c b/drivers/net/ethernet/rdc/r6040.c
index 1e49ec5..ff4683a 100644
--- a/drivers/net/ethernet/rdc/r6040.c
+++ b/drivers/net/ethernet/rdc/r6040.c
@@ -222,6 +222,7 @@ static int r6040_phy_read(void __iomem *ioaddr, int phy_addr, int reg)
 		cmd = ioread16(ioaddr + MMDIO);
 		if (!(cmd & MDIO_READ))
 			break;
+		udelay(1);
 	}
 
 	if (limit < 0)
@@ -245,6 +246,7 @@ static int r6040_phy_write(void __iomem *ioaddr,
 		cmd = ioread16(ioaddr + MMDIO);
 		if (!(cmd & MDIO_WRITE))
 			break;
+		udelay(1);
 	}
 
 	return (limit < 0) ? -ETIMEDOUT : 0;
-- 
1.8.3.2

[PATCH net-next 2/2] r6040: use ETH_ZLEN instead of MISR for SKB length checking

[Florian Fainelli]

Ever since this driver was merged the following code was included:

if (skb->len < MISR)
	skb->len = MISR;

MISR is defined to 0x3C which is also equivalent to ETH_ZLEN, but use
ETH_ZLEN directly which is exactly what we want to be checking for.

Reported-by: Marc Volovic <marcv@ezchip.com>
Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
 drivers/net/ethernet/rdc/r6040.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/rdc/r6040.c b/drivers/net/ethernet/rdc/r6040.c
index ff4683a..eb15ebf 100644
--- a/drivers/net/ethernet/rdc/r6040.c
+++ b/drivers/net/ethernet/rdc/r6040.c
@@ -836,8 +836,8 @@ static netdev_tx_t r6040_start_xmit(struct sk_buff *skb,
 	/* Set TX descriptor & Transmit it */
 	lp->tx_free_desc--;
 	descptr = lp->tx_insert_ptr;
-	if (skb->len < MISR)
-		descptr->len = MISR;
+	if (skb->len < ETH_ZLEN)
+		descptr->len = ETH_ZLEN;
 	else
 		descptr->len = skb->len;
 
-- 
1.8.3.2

Re: [PATCH net-next 2/2] r6040: use ETH_ZLEN instead of MISR for SKB length checking

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 3766 bytes --]

On Wed, 2014-01-15 at 13:04 -0800, Florian Fainelli wrote:
> Ever since this driver was merged the following code was included:
> 
> if (skb->len < MISR)
> 	skb->len = MISR;

The second 'skb->len' is currently 'descptr->len'.

> MISR is defined to 0x3C which is also equivalent to ETH_ZLEN, but use
> ETH_ZLEN directly which is exactly what we want to be checking for.
> 
> Reported-by: Marc Volovic <marcv@ezchip.com>
> Signed-off-by: Florian Fainelli <florian@openwrt.org>
> ---
>  drivers/net/ethernet/rdc/r6040.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/rdc/r6040.c b/drivers/net/ethernet/rdc/r6040.c
> index ff4683a..eb15ebf 100644
> --- a/drivers/net/ethernet/rdc/r6040.c
> +++ b/drivers/net/ethernet/rdc/r6040.c
> @@ -836,8 +836,8 @@ static netdev_tx_t r6040_start_xmit(struct sk_buff *skb,
>  	/* Set TX descriptor & Transmit it */
>  	lp->tx_free_desc--;
>  	descptr = lp->tx_insert_ptr;
> -	if (skb->len < MISR)
> -		descptr->len = MISR;
> +	if (skb->len < ETH_ZLEN)
> +		descptr->len = ETH_ZLEN;

It looks like this is just increasing the TX descriptor length so the
packet is tail-padded with whatever happens to come next in the skb.
This is absolutely incorrect behaviour and may leak sensitive
information.

Presumably it is necessary to pad the frame because the MAC is too lame
to do it, and the correct way to do that is using skb_padto(skb,
ETH_ZLEN).  But this may fail as it might have to allocate memory

Additionally, the driver DMA-maps a length of skb->len but needs to map
a length of descptr->len.

Finally, it doesn't check for failure of DMA mapping.

Tidying up naming should be way down the list of priorities for
fixing this code.

I think this will fix those problems, though I need to split it up into
multiple patches:

--- a/drivers/net/ethernet/rdc/r6040.c
+++ b/drivers/net/ethernet/rdc/r6040.c
@@ -816,6 +816,7 @@ static netdev_tx_t r6040_start_xmit(struct sk_buff *skb,
 	struct r6040_descriptor *descptr;
 	void __iomem *ioaddr = lp->base;
 	unsigned long flags;
+	dma_addr_t dma_addr;
 
 	/* Critical Section */
 	spin_lock_irqsave(&lp->lock, flags);
@@ -828,24 +829,35 @@ static netdev_tx_t r6040_start_xmit(struct sk_buff *skb,
 		return NETDEV_TX_BUSY;
 	}
 
-	/* Statistic Counter */
-	dev->stats.tx_packets++;
-	dev->stats.tx_bytes += skb->len;
 	/* Set TX descriptor & Transmit it */
-	lp->tx_free_desc--;
 	descptr = lp->tx_insert_ptr;
-	if (skb->len < MISR)
-		descptr->len = MISR;
-	else
-		descptr->len = skb->len;
+
+	/* MAC doesn't pad frames so we have to */
+	if (skb_padto(skb, ETH_ZLEN)) {
+		kfree_skb(skb);
+		goto out;
+	}
+	descptr->len = max_t(int, skb->len, ETH_ZLEN);
+
+	dma_addr = pci_map_single(lp->pdev, skb->data, descptr->len,
+				  PCI_DMA_TODEVICE);
+	if (pci_dma_mapping_error(lp->pdev, dma_addr)) {
+		kfree_skb(skb);
+		goto out;
+	}
+
+	lp->tx_free_desc--;
 
 	descptr->skb_ptr = skb;
-	descptr->buf = cpu_to_le32(pci_map_single(lp->pdev,
-		skb->data, skb->len, PCI_DMA_TODEVICE));
+	descptr->buf = cpu_to_le32(dma_addr);
 	descptr->status = DSC_OWNER_MAC;
 
 	skb_tx_timestamp(skb);
 
+	/* Statistic Counter */
+	dev->stats.tx_packets++;
+	dev->stats.tx_bytes += skb->len;
+
 	/* Trigger the MAC to check the TX descriptor */
 	iowrite16(TM2TX, ioaddr + MTPR);
 	lp->tx_insert_ptr = descptr->vndescp;
@@ -854,6 +866,7 @@ static netdev_tx_t r6040_start_xmit(struct sk_buff *skb,
 	if (!lp->tx_free_desc)
 		netif_stop_queue(dev);
 
+out:
 	spin_unlock_irqrestore(&lp->lock, flags);
 
 	return NETDEV_TX_OK;
---

Ben.

-- 
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

RE: [PATCH net-next 2/2] r6040: use ETH_ZLEN instead of MISR for SKB length checking

[David Laight]

From: Ben Hutchings
> On Wed, 2014-01-15 at 13:04 -0800, Florian Fainelli wrote:
> > Ever since this driver was merged the following code was included:
...
> > -	if (skb->len < MISR)
> > -		descptr->len = MISR;
> > +	if (skb->len < ETH_ZLEN)
> > +		descptr->len = ETH_ZLEN;
> 
> It looks like this is just increasing the TX descriptor length so the
> packet is tail-padded with whatever happens to come next in the skb.
> This is absolutely incorrect behaviour and may leak sensitive
> information.

And possibly page fault if the data is right at the end of a page.

> Presumably it is necessary to pad the frame because the MAC is too lame
> to do it, and the correct way to do that is using skb_padto(skb,
> ETH_ZLEN).  But this may fail as it might have to allocate memory

Alternatively use two ring entries with the 'more' bit set on
the first one and transmit the padding from a permanently allocated
and dma-mapped block of zeros.
Assuming the hardware support SG transmit and doesn't have a
constraint on the length of the first fragment.

Or have a pre-allocated an dma-mapped array of short buffers (one
for each ring slot) and copy short packets into the array instead
of dma-mapping the skb data.

	David

Re: [PATCH net-next 2/2] r6040: use ETH_ZLEN instead of MISR for SKB length checking

[Ben Hutchings]

On Thu, 2014-01-16 at 09:10 +0000, David Laight wrote:
> From: Ben Hutchings
> > On Wed, 2014-01-15 at 13:04 -0800, Florian Fainelli wrote:
> > > Ever since this driver was merged the following code was included:
> ...
> > > -	if (skb->len < MISR)
> > > -		descptr->len = MISR;
> > > +	if (skb->len < ETH_ZLEN)
> > > +		descptr->len = ETH_ZLEN;
> > 
> > It looks like this is just increasing the TX descriptor length so the
> > packet is tail-padded with whatever happens to come next in the skb.
> > This is absolutely incorrect behaviour and may leak sensitive
> > information.
> 
> And possibly page fault if the data is right at the end of a page.

If the CPU was accessing it, it would be fine as the skb linear area
ends with struct skb_shared_info.  (sfc actually takes advantage of that
in efx_enqueue_skb_pio() which can add padding (*not* transmitted) to
ensure that the CPU does write-combining.)

But the NIC, if it's connected through an IOMMU, might get a page fault
due to the incorrect length of the DMA mapping.

> > Presumably it is necessary to pad the frame because the MAC is too lame
> > to do it, and the correct way to do that is using skb_padto(skb,
> > ETH_ZLEN).  But this may fail as it might have to allocate memory
> 
> Alternatively use two ring entries with the 'more' bit set on
> the first one and transmit the padding from a permanently allocated
> and dma-mapped block of zeros.
> Assuming the hardware support SG transmit and doesn't have a
> constraint on the length of the first fragment.

Do you see NETIF_F_SG in this driver?

Ben.

> Or have a pre-allocated an dma-mapped array of short buffers (one
> for each ring slot) and copy short packets into the array instead
> of dma-mapping the skb data.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next 2/2] r6040: use ETH_ZLEN instead of MISR for SKB length checking

[Florian Fainelli]

2014/1/16 Ben Hutchings <bhutchings@solarflare.com>:
> On Thu, 2014-01-16 at 09:10 +0000, David Laight wrote:
>> From: Ben Hutchings
>> > On Wed, 2014-01-15 at 13:04 -0800, Florian Fainelli wrote:
>> > > Ever since this driver was merged the following code was included:
>> ...
>> > > - if (skb->len < MISR)
>> > > -         descptr->len = MISR;
>> > > + if (skb->len < ETH_ZLEN)
>> > > +         descptr->len = ETH_ZLEN;
>> >
>> > It looks like this is just increasing the TX descriptor length so the
>> > packet is tail-padded with whatever happens to come next in the skb.
>> > This is absolutely incorrect behaviour and may leak sensitive
>> > information.
>>
>> And possibly page fault if the data is right at the end of a page.
>
> If the CPU was accessing it, it would be fine as the skb linear area
> ends with struct skb_shared_info.  (sfc actually takes advantage of that
> in efx_enqueue_skb_pio() which can add padding (*not* transmitted) to
> ensure that the CPU does write-combining.)
>
> But the NIC, if it's connected through an IOMMU, might get a page fault
> due to the incorrect length of the DMA mapping.
>
>> > Presumably it is necessary to pad the frame because the MAC is too lame
>> > to do it, and the correct way to do that is using skb_padto(skb,
>> > ETH_ZLEN).  But this may fail as it might have to allocate memory
>>
>> Alternatively use two ring entries with the 'more' bit set on
>> the first one and transmit the padding from a permanently allocated
>> and dma-mapped block of zeros.
>> Assuming the hardware support SG transmit and doesn't have a
>> constraint on the length of the first fragment.
>
> Do you see NETIF_F_SG in this driver?

No this driver does not it, your fix padding the SKB to max(skb->len,
ETH_ZLEN) looks good to me. Could you submit these as two separate
patches?

Thanks!
-- 
Florian

Re: [PATCH net-next 0/2] r6040: misc fixes

[David Miller]

From: Florian Fainelli <florian@openwrt.org>
Date: Wed, 15 Jan 2014 13:04:24 -0800

> Here are two small fixes, patch 1 could potentially be backported to stable
> trees since it affects MDIO operations.

Both applied, thanks.