diff mbox series

[net-next,v1,1/1] net: stmmac: Introduce set_rx_ic() for enabling RX interrupt-on-completion

Message ID 20240814092438.3129-1-ende.tan@starfivetech.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next,v1,1/1] net: stmmac: Introduce set_rx_ic() for enabling RX interrupt-on-completion | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 29 this patch: 29
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang success Errors and warnings before: 29 this patch: 29
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 39 this patch: 39
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 129 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 38 this patch: 38
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-08-15--18-00 (tests: 706)

Commit Message

EnDe Tan Aug. 14, 2024, 9:24 a.m. UTC 
From: Tan En De <ende.tan@starfivetech.com>

Currently, some set_rx_owner() callbacks set interrupt-on-completion bit
in addition to OWN bit, without inserting a dma_wmb() barrier. This
might cause missed interrupt if the DMA sees the OWN bit before the
interrupt-on-completion bit is set.

Thus, let's introduce set_rx_ic() for enabling interrupt-on-completion,
and call it before dma_wmb() and set_rx_owner() in the main driver,
ensuring proper ordering and preventing missed interrupt.

Signed-off-by: Tan En De <ende.tan@starfivetech.com>
---
v1:
- Generalized my previous patch to fix not only dwmac4.
- Link to my previous patch:
  Set OWN bit last in dwmac4_set_rx_owner()
  https://patchwork.kernel.org/project/netdevbpf/patch/20240809144229.1370-1-ende.tan@starfivetech.com/
---
 drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c | 14 +++++++++++---
 .../net/ethernet/stmicro/stmmac/dwxgmac2_descs.c   | 14 ++++++++++----
 drivers/net/ethernet/stmicro/stmmac/enh_desc.c     |  2 +-
 drivers/net/ethernet/stmicro/stmmac/hwif.h         |  6 +++++-
 drivers/net/ethernet/stmicro/stmmac/norm_desc.c    |  2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |  6 ++++--
 6 files changed, 32 insertions(+), 12 deletions(-)

Comments

Jakub Kicinski Aug. 16, 2024, 6:09 p.m. UTC | #1 
On Wed, 14 Aug 2024 17:24:38 +0800 ende.tan@starfivetech.com wrote:
> From: Tan En De <ende.tan@starfivetech.com>
> 
> Currently, some set_rx_owner() callbacks set interrupt-on-completion bit
> in addition to OWN bit, without inserting a dma_wmb() barrier. This
> might cause missed interrupt if the DMA sees the OWN bit before the
> interrupt-on-completion bit is set.
> 
> Thus, let's introduce set_rx_ic() for enabling interrupt-on-completion,
> and call it before dma_wmb() and set_rx_owner() in the main driver,
> ensuring proper ordering and preventing missed interrupt.

Having multiple indirect function calls to write a single descriptor 
is really not great. Looks like it's always bit 31, can't this be coded
up as common handler which sets bit 31 in the appropriate word (word
offset specified per platform)?
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
index 1c5802e0d7f4..e9f95ca88e34 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
@@ -184,14 +184,19 @@  static void dwmac4_set_tx_owner(struct dma_desc *p)
 	p->des3 |= cpu_to_le32(TDES3_OWN);
 }
 
-static void dwmac4_set_rx_owner(struct dma_desc *p, int disable_rx_ic)
+static void dwmac4_set_rx_ic(struct dma_desc *p, int disable_rx_ic)
 {
-	p->des3 |= cpu_to_le32(RDES3_OWN | RDES3_BUFFER1_VALID_ADDR);
+	p->des3 |= cpu_to_le32(RDES3_BUFFER1_VALID_ADDR);
 
 	if (!disable_rx_ic)
 		p->des3 |= cpu_to_le32(RDES3_INT_ON_COMPLETION_EN);
 }
 
+static void dwmac4_set_rx_owner(struct dma_desc *p)
+{
+	p->des3 |= cpu_to_le32(RDES3_OWN);
+}
+
 static int dwmac4_get_tx_ls(struct dma_desc *p)
 {
 	return (le32_to_cpu(p->des3) & TDES3_LAST_DESCRIPTOR)
@@ -304,7 +309,9 @@  static int dwmac4_wrback_get_rx_timestamp_status(void *desc, void *next_desc,
 static void dwmac4_rd_init_rx_desc(struct dma_desc *p, int disable_rx_ic,
 				   int mode, int end, int bfsize)
 {
-	dwmac4_set_rx_owner(p, disable_rx_ic);
+	dwmac4_set_rx_ic(p, disable_rx_ic);
+	dma_wmb();
+	dwmac4_set_rx_owner(p);
 }
 
 static void dwmac4_rd_init_tx_desc(struct dma_desc *p, int mode, int end)
@@ -560,6 +567,7 @@  const struct stmmac_desc_ops dwmac4_desc_ops = {
 	.get_tx_len = dwmac4_rd_get_tx_len,
 	.get_tx_owner = dwmac4_get_tx_owner,
 	.set_tx_owner = dwmac4_set_tx_owner,
+	.set_rx_ic = dwmac4_set_rx_ic,
 	.set_rx_owner = dwmac4_set_rx_owner,
 	.get_tx_ls = dwmac4_get_tx_ls,
 	.get_rx_vlan_tci = dwmac4_wrback_get_rx_vlan_tci,
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
index fc82862a612c..73b49d021508 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
@@ -54,14 +54,17 @@  static void dwxgmac2_set_tx_owner(struct dma_desc *p)
 	p->des3 |= cpu_to_le32(XGMAC_TDES3_OWN);
 }
 
-static void dwxgmac2_set_rx_owner(struct dma_desc *p, int disable_rx_ic)
+static void dwxgmac2_set_rx_ic(struct dma_desc *p, int disable_rx_ic)
 {
-	p->des3 |= cpu_to_le32(XGMAC_RDES3_OWN);
-
 	if (!disable_rx_ic)
 		p->des3 |= cpu_to_le32(XGMAC_RDES3_IOC);
 }
 
+static void dwxgmac2_set_rx_owner(struct dma_desc *p)
+{
+	p->des3 |= cpu_to_le32(XGMAC_RDES3_OWN);
+}
+
 static int dwxgmac2_get_tx_ls(struct dma_desc *p)
 {
 	return (le32_to_cpu(p->des3) & XGMAC_RDES3_LD) > 0;
@@ -129,7 +132,9 @@  static int dwxgmac2_get_rx_timestamp_status(void *desc, void *next_desc,
 static void dwxgmac2_init_rx_desc(struct dma_desc *p, int disable_rx_ic,
 				  int mode, int end, int bfsize)
 {
-	dwxgmac2_set_rx_owner(p, disable_rx_ic);
+	dwxgmac2_set_rx_ic(p, disable_rx_ic);
+	dma_wmb();
+	dwxgmac2_set_rx_owner(p);
 }
 
 static void dwxgmac2_init_tx_desc(struct dma_desc *p, int mode, int end)
@@ -347,6 +352,7 @@  const struct stmmac_desc_ops dwxgmac210_desc_ops = {
 	.get_tx_len = dwxgmac2_get_tx_len,
 	.get_tx_owner = dwxgmac2_get_tx_owner,
 	.set_tx_owner = dwxgmac2_set_tx_owner,
+	.set_rx_ic = dwxgmac2_set_rx_ic,
 	.set_rx_owner = dwxgmac2_set_rx_owner,
 	.get_tx_ls = dwxgmac2_get_tx_ls,
 	.get_rx_frame_len = dwxgmac2_get_rx_frame_len,
diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
index 937b7a0466fc..1f0666c43de6 100644
--- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
@@ -287,7 +287,7 @@  static void enh_desc_set_tx_owner(struct dma_desc *p)
 	p->des0 |= cpu_to_le32(ETDES0_OWN);
 }
 
-static void enh_desc_set_rx_owner(struct dma_desc *p, int disable_rx_ic)
+static void enh_desc_set_rx_owner(struct dma_desc *p)
 {
 	p->des0 |= cpu_to_le32(RDES0_OWN);
 }
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index e53c32362774..6f3f8aacb0b3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -66,7 +66,9 @@  struct stmmac_desc_ops {
 	/* Get the buffer size from the descriptor */
 	int (*get_tx_len)(struct dma_desc *p);
 	/* Handle extra events on specific interrupts hw dependent */
-	void (*set_rx_owner)(struct dma_desc *p, int disable_rx_ic);
+	void (*set_rx_ic)(struct dma_desc *p, int disable_rx_ic);
+	/* Set the OWN bit of the RX descriptor */
+	void (*set_rx_owner)(struct dma_desc *p);
 	/* Get the receive frame size */
 	int (*get_rx_frame_len)(struct dma_desc *p, int rx_coe_type);
 	/* Return the reception status looking at the RDES1 */
@@ -129,6 +131,8 @@  struct stmmac_desc_ops {
 	stmmac_do_callback(__priv, desc, tx_status, __args)
 #define stmmac_get_tx_len(__priv, __args...) \
 	stmmac_do_callback(__priv, desc, get_tx_len, __args)
+#define stmmac_set_rx_ic(__priv, __args...) \
+	stmmac_do_void_callback(__priv, desc, set_rx_ic, __args)
 #define stmmac_set_rx_owner(__priv, __args...) \
 	stmmac_do_void_callback(__priv, desc, set_rx_owner, __args)
 #define stmmac_get_rx_frame_len(__priv, __args...) \
diff --git a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
index 68a7cfcb1d8f..10a5f5aaabf1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
@@ -153,7 +153,7 @@  static void ndesc_set_tx_owner(struct dma_desc *p)
 	p->des0 |= cpu_to_le32(TDES0_OWN);
 }
 
-static void ndesc_set_rx_owner(struct dma_desc *p, int disable_rx_ic)
+static void ndesc_set_rx_owner(struct dma_desc *p)
 {
 	p->des0 |= cpu_to_le32(RDES0_OWN);
 }
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f3a1b179aaea..0d065166154a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4847,8 +4847,9 @@  static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
 		if (!priv->use_riwt)
 			use_rx_wd = false;
 
+		stmmac_set_rx_ic(priv, p, use_rx_wd);
 		dma_wmb();
-		stmmac_set_rx_owner(priv, p, use_rx_wd);
+		stmmac_set_rx_owner(priv, p);
 
 		entry = STMMAC_GET_ENTRY(entry, priv->dma_conf.dma_rx_size);
 	}
@@ -5204,8 +5205,9 @@  static bool stmmac_rx_refill_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
 		if (!priv->use_riwt)
 			use_rx_wd = false;
 
+		stmmac_set_rx_ic(priv, rx_desc, use_rx_wd);
 		dma_wmb();
-		stmmac_set_rx_owner(priv, rx_desc, use_rx_wd);
+		stmmac_set_rx_owner(priv, rx_desc);
 
 		entry = STMMAC_GET_ENTRY(entry, priv->dma_conf.dma_rx_size);
 	}