[PATCH] enc28j60: Fix sporadic packet loss (correction)

[PATCH] enc28j60: Fix sporadic packet loss (correction)

[Baruch Siach]

Packet data read from the RX buffer the when the RSV is at the end of the RX 
buffer does not warp around. This causes packet loss, as the actual data is 
never read. Fix this by calculating the right packet data location.

Thanks to Shachar Shemesh for suggesting the fix.

Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>

---

The previous patch didn't increase the read pointer when no warp around was 
needed. This is corrected here.


--- drivers/net/enc28j60.c-git	2008-12-02 09:08:24.000000000 +0200
+++ drivers/net/enc28j60.c	2008-12-02 09:38:33.000000000 +0200
@@ -568,6 +568,17 @@ static u16 erxrdpt_workaround(u16 next_p
 	return erxrdpt;
 }
 
+/*
+ * Calculate wrap around when reading beyond the end of the RX buffer
+ */
+static u16 rx_packet_start(u16 ptr)
+{
+	if (ptr + RSV_SIZE > RXEND_INIT)
+		return (ptr + RSV_SIZE) - RXEND_INIT - 1;
+	else
+		return ptr + RSV_SIZE;
+}
+
 static void nolock_rxfifo_init(struct enc28j60_net *priv, u16 start, u16 end)
 {
 	u16 erxrdpt;
@@ -938,8 +949,9 @@ static void enc28j60_hw_rx(struct net_de
 			skb->dev = ndev;
 			skb_reserve(skb, NET_IP_ALIGN);
 			/* copy the packet from the receive buffer */
-			enc28j60_mem_read(priv, priv->next_pk_ptr + sizeof(rsv),
-					len, skb_put(skb, len));
+			enc28j60_mem_read(priv,
+				rx_packet_start(priv->next_pk_ptr),
+				len, skb_put(skb, len));
 			if (netif_msg_pktdata(priv))
 				dump_packet(__func__, skb->len, skb->data);
 			skb->protocol = eth_type_trans(skb, ndev);

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [spi-devel-general] [PATCH] enc28j60: Fix sporadic packet loss (correction)

[David Brownell]

On Tuesday 02 December 2008, Baruch Siach wrote:
> Packet data read from the RX buffer the when the RSV is at the end of the RX 
> buffer does not warp around. This causes packet loss, as the actual data is 
> never read. Fix this by calculating the right packet data location.
> 
> Thanks to Shachar Shemesh for suggesting the fix.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> 
> ---

Seems plausible to me, but network patches should be submitted
to the netdev list.  :)

I CC'd them on this reply ... if nobody gives any feedback, I'll
suggest you resubmit there on Friday.

- Dave

p.s. Just for info ... what kind of Linux system are you using
   this driver on?

> 
> --- drivers/net/enc28j60.c-git	2008-12-02 09:08:24.000000000 +0200
> +++ drivers/net/enc28j60.c	2008-12-02 09:38:33.000000000 +0200
> @@ -568,6 +568,17 @@ static u16 erxrdpt_workaround(u16 next_p
>  	return erxrdpt;
>  }
>  
> +/*
> + * Calculate wrap around when reading beyond the end of the RX buffer
> + */
> +static u16 rx_packet_start(u16 ptr)
> +{
> +	if (ptr + RSV_SIZE > RXEND_INIT)
> +		return (ptr + RSV_SIZE) - RXEND_INIT - 1;
> +	else
> +		return ptr + RSV_SIZE;
> +}
> +
>  static void nolock_rxfifo_init(struct enc28j60_net *priv, u16 start, u16 end)
>  {
>  	u16 erxrdpt;
> @@ -938,8 +949,9 @@ static void enc28j60_hw_rx(struct net_de
>  			skb->dev = ndev;
>  			skb_reserve(skb, NET_IP_ALIGN);
>  			/* copy the packet from the receive buffer */
> -			enc28j60_mem_read(priv, priv->next_pk_ptr + sizeof(rsv),
> -					len, skb_put(skb, len));
> +			enc28j60_mem_read(priv,
> +				rx_packet_start(priv->next_pk_ptr),
> +				len, skb_put(skb, len));
>  			if (netif_msg_pktdata(priv))
>  				dump_packet(__func__, skb->len, skb->data);
>  			skb->protocol = eth_type_trans(skb, ndev);
> 
> -- 
>                                                      ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>    - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
> 
>

Re: [spi-devel-general] [PATCH] enc28j60: Fix sporadic packet loss (correction)

[Baruch Siach]

Hi David,

On Tue, Dec 02, 2008 at 01:05:05PM -0800, David Brownell wrote:
> Seems plausible to me, but network patches should be submitted
> to the netdev list.  :)

So I did after Claudio suggested the same (and acked the patch).

> p.s. Just for info ... what kind of Linux system are you using
>    this driver on?

I'm using this driver on a custom chip that includes ARM926 core. The kernel 
version is (currently) 2.6.26. The SPI bus on this chip is the only way to 
connect peripherals.  That's way we're stuck with this Ethernet interface.

Our on-chip SPI master is the Synopsys DesignWare one. I wrote a driver for 
this SPI master. This driver is somewhat usable, but not complete yet. I'm now 
in the process of adding DMA support to the driver (PL080) to overcome some 
nasty  design peculiarities of this SPI master.

Peter Pearse has poster a (incomplete) patch set with a driver for the PL080 
DMA controller to the LAKL last week, but I see no push for integrating it 
into the mainline. So I doubt there's any great value in this SPI master 
driver as of now.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH] enc28j60: Fix sporadic packet loss (correction)

[David Brownell]

On Tuesday 02 December 2008, Baruch Siach wrote:
> Hi David,
> 
> On Tue, Dec 02, 2008 at 01:05:05PM -0800, David Brownell wrote:
> > Seems plausible to me, but network patches should be submitted
> > to the netdev list.  :)
> 
> So I did after Claudio suggested the same (and acked the patch).

Good, thanks.


> > p.s. Just for info ... what kind of Linux system are you using
> >    this driver on?
> 
> I'm using this driver on a custom chip that includes ARM926 core. The kernel 
> version is (currently) 2.6.26. The SPI bus on this chip is the only way to 
> connect peripherals.  That's way we're stuck with this Ethernet interface.

It's a bit sub-optimal ... but I'm glad you've got that option!  :)

 
> Our on-chip SPI master is the Synopsys DesignWare one. I wrote a driver for 
> this SPI master. This driver is somewhat usable, but not complete yet. I'm now 
> in the process of adding DMA support to the driver (PL080) to overcome some 
> nasty  design peculiarities of this SPI master.
> 
> Peter Pearse has poster a (incomplete) patch set with a driver for the PL080 
> DMA controller to the LAKL last week, but I see no push for integrating it 
> into the mainline. So I doubt there's any great value in this SPI master 
> driver as of now.

I certainly wouldn't expect that on the ARM list.  It's
not exactly the responsibility of that list to address
anything other than arch/arm/* goodies.

Post it to the SPI list instead.  If it's useful to you,
it could well be useful to others... I'd have no problems
merging such a driver, if it's reasonably clean/correct
and is known to work well enough to support enc28j60.

- Dave

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [spi-devel-general] [PATCH] enc28j60: Fix sporadic packet loss (correction)

[Baruch Siach]

HI David,

On Tue, Dec 02, 2008 at 02:44:02PM -0800, David Brownell wrote:
> On Tuesday 02 December 2008, Baruch Siach wrote:
> > Our on-chip SPI master is the Synopsys DesignWare one. I wrote a driver 
> > for this SPI master. This driver is somewhat usable, but not complete yet. 
> > I'm now in the process of adding DMA support to the driver (PL080) to 
> > overcome some nasty  design peculiarities of this SPI master.
> > 
> > Peter Pearse has poster a (incomplete) patch set with a driver for the PL080 
> > DMA controller to the LAKL last week, but I see no push for integrating it 
> > into the mainline. So I doubt there's any great value in this SPI master 
> > driver as of now.
> 
> I certainly wouldn't expect that on the ARM list.  It's
> not exactly the responsibility of that list to address
> anything other than arch/arm/* goodies.

Apparently I haven't explained myself clear enough. The (partial) driver that 
Peter Pearse has posted[1] to LAKL is for the PL080 DMA controller. This 
driver is ARM specific, so LAKL seems to be reasonable place even though the 
driver itself is under drivers/amba (which is ARM specific as well).

My Synopsys DesignWare SPI master driver will, when complete, depend on this 
DMA controller driver, that's not in the mainline (yet).

> Post it to the SPI list instead.  If it's useful to you,
> it could well be useful to others... I'd have no problems
> merging such a driver, if it's reasonably clean/correct
> and is known to work well enough to support enc28j60.

I hope I'll have time to clean up the SPI master driver code in its current 
form, and post in a few days.

baruch

[1] 
http://lists.arm.linux.org.uk/lurker/message/20081121.134729.8fe35d0d.en.html

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH] enc28j60: Fix sporadic packet loss (correction)

[David Brownell]

On Tuesday 02 December 2008, Baruch Siach wrote:

> Apparently I haven't explained myself clear enough. The (partial) driver that 
> Peter Pearse has posted[1] to LAKL is for the PL080 DMA controller. This 
> driver is ARM specific, so LAKL seems to be reasonable place even though the 
> driver itself is under drivers/amba (which is ARM specific as well).

Actually AMBA has generalized past being ARM-specific.
It's just a chip-internal bus, and the silicon-IP
end of ARM Ltd has been successful enough that vendors
use it with non-ARM CPUs too.

Example:  you'll notice that drivers/dma/dw_dmac.c was
first used on a non-ARM system; it's a Synopsys DesignWare
(dw) DMA controller (dmac) for AMBA2, with AHB and APB.

So let me suggest LKML, with an FYI to the ARM list,
and putting that driver in drivers/dma instead. :)


But yes, now I see what you mean:  discrete DMA and SPI
drivers.

- Dave


> My Synopsys DesignWare SPI master driver will, when complete, depend on this 
> DMA controller driver, that's not in the mainline (yet).
> 
> > Post it to the SPI list instead.  ...

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/