Bug in ks8851.c

Bug in ks8851.c

[Max.Nekludov]

According to the Datasheet (page 52):
15-12 Reserved
11-0 RXBC Receive Byte Count
This field indicates the present received frame byte size.

I suppose the code has a bug:
		rxh = ks8851_rdreg32(ks, KS_RXFHSR);
		rxstat = rxh & 0xffff;
		rxlen = rxh >> 16; // BUG!!! 0xFFF mask should be applied

P.S.
without bit mask applied I saw rxlen equal to 15360 which is bigger then
entire RX queue size (12KB).

Thanks,
Max Nekludov

>From cb3199cee4490f98d6062e32a75ca377a32b55bc Mon Sep 17 00:00:00 2001
From: Max Neklyudov <macscomp@gmail.com>
Date: Tue, 26 Mar 2013 11:46:57 +0400
Subject: [PATCH] Fix bug in ks8851 driver

---
 drivers/net/ks8851.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ks8851.c b/drivers/net/ks8851.c
index 91a93cb..0dc03da 100644
--- a/drivers/net/ks8851.c
+++ b/drivers/net/ks8851.c
@@ -553,7 +553,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 	for (; rxfc != 0; rxfc--) {
 		rxh = ks8851_rdreg32(ks, KS_RXFHSR);
 		rxstat = rxh & 0xffff;
-		rxlen = rxh >> 16;
+		rxlen = (rxh >> 16) & 0xFFF;

 		netif_dbg(ks, rx_status, ks->netdev,
 			  "rx: stat 0x%04x, len 0x%04x\n", rxstat, rxlen);
--
1.7.10.4

Re: Bug in ks8851.c

[Linus Torvalds]

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

Max,
 please cc the actual maintainers of the driver. The patch looks sane,
though. I assume you've tested it in practice?

You also seem to have based this on an ancient version, the code has
long since moved from drivers/net/ks8851.c to
drivers/net/ethernet/micrel/ks8851.c (back in June of 2011), and it's
missing a sign-off from you.

I'm attaching an updated patch for the rename/capitalization issue.

     Linus

On Thu, Mar 28, 2013 at 11:25 AM,  <Max.Nekludov@us.elster.com> wrote:
>
> According to the Datasheet (page 52):
> 15-12 Reserved
> 11-0 RXBC Receive Byte Count
> This field indicates the present received frame byte size.
>
> I suppose the code has a bug:
>                 rxh = ks8851_rdreg32(ks, KS_RXFHSR);
>                 rxstat = rxh & 0xffff;
>                 rxlen = rxh >> 16; // BUG!!! 0xFFF mask should be applied
>
> P.S.
> without bit mask applied I saw rxlen equal to 15360 which is bigger then
> entire RX queue size (12KB).
>
> Thanks,
> Max Nekludov
>
> From cb3199cee4490f98d6062e32a75ca377a32b55bc Mon Sep 17 00:00:00 2001
> From: Max Neklyudov <macscomp@gmail.com>
> Date: Tue, 26 Mar 2013 11:46:57 +0400
> Subject: [PATCH] Fix bug in ks8851 driver
>
> ---
>  drivers/net/ks8851.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ks8851.c b/drivers/net/ks8851.c
> index 91a93cb..0dc03da 100644
> --- a/drivers/net/ks8851.c
> +++ b/drivers/net/ks8851.c
> @@ -553,7 +553,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
>         for (; rxfc != 0; rxfc--) {
>                 rxh = ks8851_rdreg32(ks, KS_RXFHSR);
>                 rxstat = rxh & 0xffff;
> -               rxlen = rxh >> 16;
> +               rxlen = (rxh >> 16) & 0xFFF;
>
>                 netif_dbg(ks, rx_status, ks->netdev,
>                           "rx: stat 0x%04x, len 0x%04x\n", rxstat, rxlen);
> --
> 1.7.10.4
>

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 625 bytes --]

 drivers/net/ethernet/micrel/ks8851.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index 33bcb63d56a2..8fb481252e2c 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -528,7 +528,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 	for (; rxfc != 0; rxfc--) {
 		rxh = ks8851_rdreg32(ks, KS_RXFHSR);
 		rxstat = rxh & 0xffff;
-		rxlen = rxh >> 16;
+		rxlen = (rxh >> 16) & 0xfff;
 
 		netif_dbg(ks, rx_status, ks->netdev,
 			  "rx: stat 0x%04x, len 0x%04x\n", rxstat, rxlen);

Re: Bug in ks8851.c

[Max.Nekludov]

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

Linus,

I tried to send the mail to 'Ben Dooks <ben@simtec.co.uk>' but the address
is dead now.
> I assume you've tested it in practice?
Yes, I'm running the modified code both in bootloader and Linux kernel on
my board.

Thanks,
Max

 Signed-off-by: Max Nekludov <Max.Nekludov@us.elster.com>
 ---
 drivers/net/ethernet/micrel/ks8851.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c
b/drivers/net/ethernet/micrel/ks8851.c
index 33bcb63d56a2..8fb481252e2c 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -528,7 +528,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 	for (; rxfc != 0; rxfc--) {
 		rxh = ks8851_rdreg32(ks, KS_RXFHSR);
 		rxstat = rxh & 0xffff;
-		rxlen = rxh >> 16;
+		rxlen = (rxh >> 16) & 0xfff;

 		netif_dbg(ks, rx_status, ks->netdev,
 			  "rx: stat 0x%04x, len 0x%04x\n", rxstat, rxlen);




Max,
 please cc the actual maintainers of the driver. The patch looks sane,
though. I assume you've tested it in practice?

You also seem to have based this on an ancient version, the code has
long since moved from drivers/net/ks8851.c to
drivers/net/ethernet/micrel/ks8851.c (back in June of 2011), and it's
missing a sign-off from you.

I'm attaching an updated patch for the rename/capitalization issue.

     Linus

On Thu, Mar 28, 2013 at 11:25 AM,  <Max.Nekludov@us.elster.com> wrote:
>
> According to the Datasheet (page 52):
> 15-12 Reserved
> 11-0 RXBC Receive Byte Count
> This field indicates the present received frame byte size.
>
> I suppose the code has a bug:
>                 rxh = ks8851_rdreg32(ks, KS_RXFHSR);
>                 rxstat = rxh & 0xffff;
>                 rxlen = rxh >> 16; // BUG!!! 0xFFF mask should be applied
>
> P.S.
> without bit mask applied I saw rxlen equal to 15360 which is bigger then
> entire RX queue size (12KB).
>
> Thanks,
> Max Nekludov
>
> From cb3199cee4490f98d6062e32a75ca377a32b55bc Mon Sep 17 00:00:00 2001
> From: Max Neklyudov <macscomp@gmail.com>
> Date: Tue, 26 Mar 2013 11:46:57 +0400
> Subject: [PATCH] Fix bug in ks8851 driver
>
> ---
>  drivers/net/ks8851.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ks8851.c b/drivers/net/ks8851.c
> index 91a93cb..0dc03da 100644
> --- a/drivers/net/ks8851.c
> +++ b/drivers/net/ks8851.c
> @@ -553,7 +553,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
>         for (; rxfc != 0; rxfc--) {
>                 rxh = ks8851_rdreg32(ks, KS_RXFHSR);
>                 rxstat = rxh & 0xffff;
> -               rxlen = rxh >> 16;
> +               rxlen = (rxh >> 16) & 0xFFF;
>
>                 netif_dbg(ks, rx_status, ks->netdev,
>                           "rx: stat 0x%04x, len 0x%04x\n", rxstat,
rxlen);
> --
> 1.7.10.4
>


______________________________________________________________________
This email has been spam and virus checked by Elster IT Services.(See
attached file: patch.diff)

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 625 bytes --]

 drivers/net/ethernet/micrel/ks8851.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index 33bcb63d56a2..8fb481252e2c 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -528,7 +528,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 	for (; rxfc != 0; rxfc--) {
 		rxh = ks8851_rdreg32(ks, KS_RXFHSR);
 		rxstat = rxh & 0xffff;
-		rxlen = rxh >> 16;
+		rxlen = (rxh >> 16) & 0xfff;
 
 		netif_dbg(ks, rx_status, ks->netdev,
 			  "rx: stat 0x%04x, len 0x%04x\n", rxstat, rxlen);

Re: Bug in ks8851.c

[Max.Nekludov]

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

Linus,

I tried to send the mail to 'Ben Dooks <ben@simtec.co.uk>' but the address
is dead now.
> I assume you've tested it in practice?
Yes, I'm running the modified code both in bootloader and Linux kernel on
my board.

Thanks,
Max

 Signed-off-by: Max Nekludov <Max.Nekludov@us.elster.com>
 ---
 drivers/net/ethernet/micrel/ks8851.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c
b/drivers/net/ethernet/micrel/ks8851.c
index 33bcb63d56a2..8fb481252e2c 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -528,7 +528,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 	for (; rxfc != 0; rxfc--) {
 		rxh = ks8851_rdreg32(ks, KS_RXFHSR);
 		rxstat = rxh & 0xffff;
-		rxlen = rxh >> 16;
+		rxlen = (rxh >> 16) & 0xfff;

 		netif_dbg(ks, rx_status, ks->netdev,
 			  "rx: stat 0x%04x, len 0x%04x\n", rxstat, rxlen);




Max,
 please cc the actual maintainers of the driver. The patch looks sane,
though. I assume you've tested it in practice?

You also seem to have based this on an ancient version, the code has
long since moved from drivers/net/ks8851.c to
drivers/net/ethernet/micrel/ks8851.c (back in June of 2011), and it's
missing a sign-off from you.

I'm attaching an updated patch for the rename/capitalization issue.

     Linus

On Thu, Mar 28, 2013 at 11:25 AM,  <Max.Nekludov@us.elster.com> wrote:
>
> According to the Datasheet (page 52):
> 15-12 Reserved
> 11-0 RXBC Receive Byte Count
> This field indicates the present received frame byte size.
>
> I suppose the code has a bug:
>                 rxh = ks8851_rdreg32(ks, KS_RXFHSR);
>                 rxstat = rxh & 0xffff;
>                 rxlen = rxh >> 16; // BUG!!! 0xFFF mask should be applied
>
> P.S.
> without bit mask applied I saw rxlen equal to 15360 which is bigger then
> entire RX queue size (12KB).
>
> Thanks,
> Max Nekludov
>
> From cb3199cee4490f98d6062e32a75ca377a32b55bc Mon Sep 17 00:00:00 2001
> From: Max Neklyudov <macscomp@gmail.com>
> Date: Tue, 26 Mar 2013 11:46:57 +0400
> Subject: [PATCH] Fix bug in ks8851 driver
>
> ---
>  drivers/net/ks8851.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ks8851.c b/drivers/net/ks8851.c
> index 91a93cb..0dc03da 100644
> --- a/drivers/net/ks8851.c
> +++ b/drivers/net/ks8851.c
> @@ -553,7 +553,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
>         for (; rxfc != 0; rxfc--) {
>                 rxh = ks8851_rdreg32(ks, KS_RXFHSR);
>                 rxstat = rxh & 0xffff;
> -               rxlen = rxh >> 16;
> +               rxlen = (rxh >> 16) & 0xFFF;
>
>                 netif_dbg(ks, rx_status, ks->netdev,
>                           "rx: stat 0x%04x, len 0x%04x\n", rxstat,
rxlen);
> --
> 1.7.10.4
>


______________________________________________________________________
This email has been spam and virus checked by Elster IT Services.(See
attached file: patch.diff)

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 625 bytes --]

 drivers/net/ethernet/micrel/ks8851.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index 33bcb63d56a2..8fb481252e2c 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -528,7 +528,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 	for (; rxfc != 0; rxfc--) {
 		rxh = ks8851_rdreg32(ks, KS_RXFHSR);
 		rxstat = rxh & 0xffff;
-		rxlen = rxh >> 16;
+		rxlen = (rxh >> 16) & 0xfff;
 
 		netif_dbg(ks, rx_status, ks->netdev,
 			  "rx: stat 0x%04x, len 0x%04x\n", rxstat, rxlen);

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Bug in ks8851.c

[Max.Nekludov at us.elster.com]

Linus,

I tried to send the mail to 'Ben Dooks <ben@simtec.co.uk>' but the address
is dead now.
> I assume you've tested it in practice?
Yes, I'm running the modified code both in bootloader and Linux kernel on
my board.

Thanks,
Max

 Signed-off-by: Max Nekludov <Max.Nekludov@us.elster.com>
 ---
 drivers/net/ethernet/micrel/ks8851.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c
b/drivers/net/ethernet/micrel/ks8851.c
index 33bcb63d56a2..8fb481252e2c 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -528,7 +528,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 	for (; rxfc != 0; rxfc--) {
 		rxh = ks8851_rdreg32(ks, KS_RXFHSR);
 		rxstat = rxh & 0xffff;
-		rxlen = rxh >> 16;
+		rxlen = (rxh >> 16) & 0xfff;

 		netif_dbg(ks, rx_status, ks->netdev,
 			  "rx: stat 0x%04x, len 0x%04x\n", rxstat, rxlen);




Max,
 please cc the actual maintainers of the driver. The patch looks sane,
though. I assume you've tested it in practice?

You also seem to have based this on an ancient version, the code has
long since moved from drivers/net/ks8851.c to
drivers/net/ethernet/micrel/ks8851.c (back in June of 2011), and it's
missing a sign-off from you.

I'm attaching an updated patch for the rename/capitalization issue.

     Linus

On Thu, Mar 28, 2013 at 11:25 AM,  <Max.Nekludov@us.elster.com> wrote:
>
> According to the Datasheet (page 52):
> 15-12 Reserved
> 11-0 RXBC Receive Byte Count
> This field indicates the present received frame byte size.
>
> I suppose the code has a bug:
>                 rxh = ks8851_rdreg32(ks, KS_RXFHSR);
>                 rxstat = rxh & 0xffff;
>                 rxlen = rxh >> 16; // BUG!!! 0xFFF mask should be applied
>
> P.S.
> without bit mask applied I saw rxlen equal to 15360 which is bigger then
> entire RX queue size (12KB).
>
> Thanks,
> Max Nekludov
>
> From cb3199cee4490f98d6062e32a75ca377a32b55bc Mon Sep 17 00:00:00 2001
> From: Max Neklyudov <macscomp@gmail.com>
> Date: Tue, 26 Mar 2013 11:46:57 +0400
> Subject: [PATCH] Fix bug in ks8851 driver
>
> ---
>  drivers/net/ks8851.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ks8851.c b/drivers/net/ks8851.c
> index 91a93cb..0dc03da 100644
> --- a/drivers/net/ks8851.c
> +++ b/drivers/net/ks8851.c
> @@ -553,7 +553,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
>         for (; rxfc != 0; rxfc--) {
>                 rxh = ks8851_rdreg32(ks, KS_RXFHSR);
>                 rxstat = rxh & 0xffff;
> -               rxlen = rxh >> 16;
> +               rxlen = (rxh >> 16) & 0xFFF;
>
>                 netif_dbg(ks, rx_status, ks->netdev,
>                           "rx: stat 0x%04x, len 0x%04x\n", rxstat,
rxlen);
> --
> 1.7.10.4
>


______________________________________________________________________
This email has been spam and virus checked by Elster IT Services.(See
attached file: patch.diff)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.diff
Type: application/octet-stream
Size: 625 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130329/d6b514d5/attachment.obj>

Re: Bug in ks8851.c

[David Miller]

From: "Nekludov, Max" <Max.Nekludov@elster.com>
Date: Tue, 1 Apr 2014 15:41:58 +0000

> Reset on RX failure and TX timeout.
> 
> Electromagnetic noice can make device to not send TX interrupts.
> As result outgoing queue will be suspended forever.
> Also EMI can raise RX interrupt with zero value in KS_RXFC register.
> 
> Patch attached.

netdev@vger.kernel.org and the driver maintainer (as listed in
MAINTAINERS) is sufficient for submitting this patch, lists
such as 'trivial' and sending such patches to Linus directly
is not really appropriate.

As per your patch, please do not submit it as an attachment,
as per Documentation/SubmittingPatches, it should be the main
content with the commit message header line being in your
Subject.

Furthermore, in:

+static void ks8851_wdt_work(struct delayed_work *work)
+{
+	struct ks8851_net *ks = container_of(work, struct ks8851_net, wdt_work);
+	mutex_lock(&ks->lock);

You should have an empty line beteen local variable declarations
and where real code starts.

Bug in ks8851.c

[Nekludov, Max]

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

Reset on RX failure and TX timeout.

Electromagnetic noice can make device to not send TX interrupts.
As result outgoing queue will be suspended forever.
Also EMI can raise RX interrupt with zero value in KS_RXFC register.

Patch attached.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ks8851-reset-on-RX-falure-and-TX-timeout.patch --]
[-- Type: text/x-diff; name="0001-ks8851-reset-on-RX-falure-and-TX-timeout.patch", Size: 4553 bytes --]

ks8851: reset on RX failure and TX timeout

Electromagnetic noice can make device to not send TX interrupts.
As result outgoing queue will be suspended forever.
Also EMI can raise RX interrupt with zero value in KS_RXFC register.

Signed-off-by: Max Nekludov <Max.Nekludov@elster.com>
---
 drivers/net/ethernet/micrel/ks8851.c | 66 +++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index e0c92e0..25b7229 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -123,6 +123,7 @@ struct ks8851_net {
 
 	struct work_struct	tx_work;
 	struct work_struct	rxctrl_work;
+	struct delayed_work	wdt_work;
 
 	struct sk_buff_head	txq;
 
@@ -140,6 +141,9 @@ static int msg_enable;
 /* shift for byte-enable data */
 #define BYTE_EN(_x)	((_x) << 2)
 
+#define KS8851_TX_QUEUE_SIZE 6144
+#define KS8851_TX_TIMEOUT_MS 100
+
 /* turn register number and byte-enable mask into data for start of packet */
 #define MK_OP(_byteen, _reg) (BYTE_EN(_byteen) | (_reg)  << (8+2) | (_reg) >> 6)
 
@@ -495,6 +499,7 @@ static void ks8851_dbg_dumpkkt(struct ks8851_net *ks, u8 *rxpkt)
 		   rxpkt[8], rxpkt[9], rxpkt[10], rxpkt[11],
 		   rxpkt[12], rxpkt[13], rxpkt[14], rxpkt[15]);
 }
+static void ks8851_reset(struct ks8851_net *ks);
 
 /**
  * ks8851_rx_pkts - receive packets from the host
@@ -517,6 +522,12 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 
 	netif_dbg(ks, rx_status, ks->netdev,
 		  "%s: %d packets\n", __func__, rxfc);
+	
+	if (rxfc == 0) {
+		mdelay(1);
+		/* RX interrupt but no data in buffer. Which means hardware failure condition. Reset. */
+		ks8851_reset(ks);
+	}
 
 	/* Currently we're issuing a read per packet, but we could possibly
 	 * improve the code by issuing a single read, getting the receive
@@ -619,6 +630,10 @@ static irqreturn_t ks8851_irq(int irq, void *_ks)
 
 	if (status & IRQ_TXI) {
 		handled |= IRQ_TXI;
+		
+		mutex_unlock(&ks->lock);
+		cancel_delayed_work_sync(&ks->wdt_work);
+		mutex_lock(&ks->lock);
 
 		/* no lock here, tx queue should have been stopped */
 
@@ -749,6 +764,17 @@ static void ks8851_done_tx(struct ks8851_net *ks, struct sk_buff *txb)
 	dev_kfree_skb(txb);
 }
 
+static void ks8851_wdt_work(struct delayed_work *work)
+{
+	struct ks8851_net *ks = container_of(work, struct ks8851_net, wdt_work);
+	mutex_lock(&ks->lock);
+
+	mdelay(1);
+	ks8851_reset(ks);
+
+	mutex_unlock(&ks->lock);
+}
+
 /**
  * ks8851_tx_work - process tx packet(s)
  * @work: The work strucutre what was scheduled.
@@ -778,6 +804,8 @@ static void ks8851_tx_work(struct work_struct *work)
 		}
 	}
 
+	schedule_delayed_work(&ks->wdt_work, msecs_to_jiffies(KS8851_TX_TIMEOUT_MS));
+
 	mutex_unlock(&ks->lock);
 }
 
@@ -858,6 +886,40 @@ static int ks8851_net_open(struct net_device *dev)
 	return 0;
 }
 
+static void ks8851_reset(struct ks8851_net *ks)
+{
+	netif_stop_queue(ks->netdev);
+
+	/* turn off the IRQs and ack any outstanding */
+	ks8851_wrreg16(ks, KS_IER, 0x0000);
+	ks8851_wrreg16(ks, KS_ISR, 0xffff);
+
+	/* shutdown RX process */
+	ks8851_wrreg16(ks, KS_RXCR1, 0x0000);
+
+	/* shutdown TX process */
+	ks8851_wrreg16(ks, KS_TXCR, 0x0000);
+
+	ks->tx_space = KS8851_TX_QUEUE_SIZE;
+
+	mutex_unlock(&ks->lock);
+	
+	/* ensure any queued tx buffers are dumped */
+	while (!skb_queue_empty(&ks->txq)) {
+		struct sk_buff *txb = skb_dequeue(&ks->txq);
+
+		netif_dbg(ks, ifdown, ks->netdev,
+			  "%s: freeing txb %p\n", __func__, txb);
+
+		dev_kfree_skb(txb);
+	}
+
+	/* reopen device */
+	ks8851_net_open(ks->netdev);
+
+	mutex_lock(&ks->lock);
+}
+
 /**
  * ks8851_net_stop - close network device
  * @dev: The device being closed.
@@ -883,6 +945,7 @@ static int ks8851_net_stop(struct net_device *dev)
 	/* stop any outstanding work */
 	flush_work(&ks->tx_work);
 	flush_work(&ks->rxctrl_work);
+	cancel_delayed_work_sync(&ks->wdt_work);
 
 	mutex_lock(&ks->lock);
 	/* shutdown RX process */
@@ -1415,7 +1478,7 @@ static int ks8851_probe(struct spi_device *spi)
 
 	ks->netdev = ndev;
 	ks->spidev = spi;
-	ks->tx_space = 6144;
+	ks->tx_space = KS8851_TX_QUEUE_SIZE;
 
 	ks->vdd_reg = regulator_get_optional(&spi->dev, "vdd");
 	if (IS_ERR(ks->vdd_reg)) {
@@ -1437,6 +1500,7 @@ static int ks8851_probe(struct spi_device *spi)
 
 	INIT_WORK(&ks->tx_work, ks8851_tx_work);
 	INIT_WORK(&ks->rxctrl_work, ks8851_rxctrl_work);
+	INIT_DELAYED_WORK(&ks->wdt_work, ks8851_wdt_work);
 
 	/* initialise pre-made spi transfer messages */
 
-- 
1.8.3.2

Re: Bug in ks8851.c

[David Miller]

From: Max.Nekludov@us.elster.com
Date: Fri, 29 Mar 2013 19:27:36 +0400

> Denis,
> here is patch with commit message:
> 
> Subject: [PATCH] Fix bug in ks8851 driver
> 
> According to the Datasheet (page 52):
> 15-12 Reserved
> 11-0 RXBC Receive Byte Count
> This field indicates the present received frame byte size.
> 
> The code has a bug:
>                  rxh = ks8851_rdreg32(ks, KS_RXFHSR);
>                  rxstat = rxh & 0xffff;
>                  rxlen = rxh >> 16; // BUG!!! 0xFFF mask should be applied
> 
> Signed-off-by: Max Nekludov <Max.Nekludov@us.elster.com>

Applied and queued up for -stable, thanks.

Bug in ks8851.c

[David Miller]

From: Max.Nekludov@us.elster.com
Date: Fri, 29 Mar 2013 19:27:36 +0400

> Denis,
> here is patch with commit message:
> 
> Subject: [PATCH] Fix bug in ks8851 driver
> 
> According to the Datasheet (page 52):
> 15-12 Reserved
> 11-0 RXBC Receive Byte Count
> This field indicates the present received frame byte size.
> 
> The code has a bug:
>                  rxh = ks8851_rdreg32(ks, KS_RXFHSR);
>                  rxstat = rxh & 0xffff;
>                  rxlen = rxh >> 16; // BUG!!! 0xFFF mask should be applied
> 
> Signed-off-by: Max Nekludov <Max.Nekludov@us.elster.com>

Applied and queued up for -stable, thanks.

Bug in ks8851.c

[Max.Nekludov]

----- Forwarded by Max Nekludov/USE/Elster on 28.03.2013 22:29 -----
|------------>
| From:      |
|------------>
  >--------------------------------------------------------------------------------------------------------------------------------------------------|
  |Max Nekludov/USE/Elster                                                                                                                           |
  >--------------------------------------------------------------------------------------------------------------------------------------------------|
|------------>
| To:        |
|------------>
  >--------------------------------------------------------------------------------------------------------------------------------------------------|
  |torvalds@osdl.org                                                                                                                                 |
  >--------------------------------------------------------------------------------------------------------------------------------------------------|
|------------>
| Cc:        |
|------------>
  >--------------------------------------------------------------------------------------------------------------------------------------------------|
  |linux-kernel@vger.kernel.org, trivial@rustcorp.com.au                                                                                             |
  >--------------------------------------------------------------------------------------------------------------------------------------------------|
|------------>
| Date:      |
|------------>
  >--------------------------------------------------------------------------------------------------------------------------------------------------|
  |28.03.2013 22:25                                                                                                                                  |
  >--------------------------------------------------------------------------------------------------------------------------------------------------|
|------------>
| Subject:   |
|------------>
  >--------------------------------------------------------------------------------------------------------------------------------------------------|
  |Bug in ks8851.c                                                                                                                                   |
  >--------------------------------------------------------------------------------------------------------------------------------------------------|




According to the Datasheet (page 52):
15-12 Reserved
11-0 RXBC Receive Byte Count
This field indicates the present received frame byte size.

I suppose the code has a bug:
		rxh = ks8851_rdreg32(ks, KS_RXFHSR);
		rxstat = rxh & 0xffff;
		rxlen = rxh >> 16; // BUG!!! 0xFFF mask should be applied

P.S.
without bit mask applied I saw rxlen equal to 15360 which is bigger then
entire RX queue size (12KB).

Thanks,
Max Nekludov

>From cb3199cee4490f98d6062e32a75ca377a32b55bc Mon Sep 17 00:00:00 2001
From: Max Neklyudov <macscomp@gmail.com>
Date: Tue, 26 Mar 2013 11:46:57 +0400
Subject: [PATCH] Fix bug in ks8851 driver

---
 drivers/net/ks8851.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ks8851.c b/drivers/net/ks8851.c
index 91a93cb..0dc03da 100644
--- a/drivers/net/ks8851.c
+++ b/drivers/net/ks8851.c
@@ -553,7 +553,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 	for (; rxfc != 0; rxfc--) {
 		rxh = ks8851_rdreg32(ks, KS_RXFHSR);
 		rxstat = rxh & 0xffff;
-		rxlen = rxh >> 16;
+		rxlen = (rxh >> 16) & 0xFFF;

 		netif_dbg(ks, rx_status, ks->netdev,
 			  "rx: stat 0x%04x, len 0x%04x\n", rxstat, rxlen);
--
1.7.10.4