All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lukas Wunner <lukas@wunner.de>
To: "David S. Miller" <davem@davemloft.net>,
	Tristram Ha <Tristram.Ha@microchip.com>,
	netdev@vger.kernel.org
Cc: Frank Pavlic <f.pavlic@kunbus.de>,
	Eduard Mainhardt <e.mainhardt@kunbus.de>,
	Ben Dooks <ben.dooks@codethink.co.uk>, Nishanth Menon <nm@ti.com>,
	Stephen Boyd <sboyd@codeaurora.org>,
	Sergey Shcherbakov <shchers@gmail.com>,
	Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Subject: [PATCH 1/6] net: ks8851: Dequeue RX packets explicitly
Date: Wed, 20 Mar 2019 15:02:00 +0100	[thread overview]
Message-ID: <6fb721ae5a6dd93be47d2b09f17bbc8d17d4b06a.1553089248.git.lukas@wunner.de> (raw)
In-Reply-To: <cover.1553089248.git.lukas@wunner.de>

The ks8851 driver lets the chip auto-dequeue received packets once they
have been read in full. It achieves that by setting the ADRFE flag in
the RXQCR register ("Auto-Dequeue RXQ Frame Enable").

However if allocation of a packet's socket buffer or retrieval of the
packet over the SPI bus fails, the packet will not have been read in
full and is not auto-dequeued. Such partial retrieval of a packet
confuses the chip's RX queue management:  On the next RX interrupt,
the first packet read from the queue will be the one left there
previously and this one can be retrieved without issues. But for any
newly received packets, the frame header status and byte count registers
(RXFHSR and RXFHBCR) contain bogus values, preventing their retrieval.

The chip allows explicitly dequeueing a packet from the RX queue by
setting the RRXEF flag in the RXQCR register ("Release RX Error Frame").
This could be used to dequeue the packet in case of an error, but if
that error is a failed SPI transfer, it is unknown if the packet was
transferred in full and was auto-dequeued or if it was only transferred
in part and requires an explicit dequeue. The safest approach is thus
to always dequeue packets explicitly and forgo auto-dequeueing.

Without this change, I've witnessed packet retrieval break completely
when an SPI DMA transfer fails, requiring a chip reset. Explicit
dequeueing magically fixes this and makes packet retrieval absolutely
robust for me.

The chip's documentation suggests auto-dequeuing and uses the RRXEF
flag only to dequeue error frames which the driver doesn't want to
retrieve. But that seems to be a fair-weather approach.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Frank Pavlic <f.pavlic@kunbus.de>
Cc: Ben Dooks <ben.dooks@codethink.co.uk>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
---
 drivers/net/ethernet/micrel/ks8851.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index bd6e9014bc74..a93f8e842c07 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -535,9 +535,8 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 		/* set dma read address */
 		ks8851_wrreg16(ks, KS_RXFDPR, RXFDPR_RXFPAI | 0x00);
 
-		/* start the packet dma process, and set auto-dequeue rx */
-		ks8851_wrreg16(ks, KS_RXQCR,
-			       ks->rc_rxqcr | RXQCR_SDA | RXQCR_ADRFE);
+		/* start DMA access */
+		ks8851_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_SDA);
 
 		if (rxlen > 4) {
 			unsigned int rxalign;
@@ -568,7 +567,8 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 			}
 		}
 
-		ks8851_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr);
+		/* end DMA access and dequeue packet */
+		ks8851_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_RRXEF);
 	}
 }
 
-- 
2.20.1


  parent reply	other threads:[~2019-03-20 14:12 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-20 14:02 [PATCH 0/6] ks8851 fixes & cleanups Lukas Wunner
2019-03-20 14:02 ` [PATCH 2/6] net: ks8851: Reassert reset pin if chip ID check fails Lukas Wunner
2019-03-20 14:02 ` [PATCH 6/6] net: ks8851: Deduplicate register macros Lukas Wunner
2019-03-20 14:02 ` [PATCH 4/6] net: ks8851: Set initial carrier state to down Lukas Wunner
2019-03-20 14:02 ` Lukas Wunner [this message]
2019-03-20 14:02 ` [PATCH 5/6] net: ks8851: Fix register macro misnomers Lukas Wunner
2019-03-20 14:02 ` [PATCH 3/6] net: ks8851: Delay requesting IRQ until opened Lukas Wunner
2019-03-20 17:26 ` [PATCH 0/6] ks8851 fixes & cleanups Ben Dooks
2019-03-20 19:44 ` David Miller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6fb721ae5a6dd93be47d2b09f17bbc8d17d4b06a.1553089248.git.lukas@wunner.de \
    --to=lukas@wunner.de \
    --cc=Tristram.Ha@microchip.com \
    --cc=ben.dooks@codethink.co.uk \
    --cc=davem@davemloft.net \
    --cc=e.mainhardt@kunbus.de \
    --cc=f.pavlic@kunbus.de \
    --cc=netdev@vger.kernel.org \
    --cc=nm@ti.com \
    --cc=plagnioj@jcrosoft.com \
    --cc=sboyd@codeaurora.org \
    --cc=shchers@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.