[PATCH v2 0/5] net: designware: eqos: fixes

[PATCH v2 0/5] net: designware: eqos: fixes

[Oleksij Rempel]

changes v2:
- spelling fixes in the "initialize MAC address" patch

Oleksij Rempel (5):
  net: designware: eqos: initialize MAC address specific DMA channel
    configuration
  net: designware: eqos: fix non-working promisc mode when set before
    interface start
  net: designware: eqos: add comment about external clock dependencies
    for the soft reset
  net: designware: eqos: fix NULL pointer dereference on LLDP packets
  net: designware: eqos: do not receive pause frames

 drivers/net/designware_eqos.c | 78 +++++++++++++++++++++++++----------
 drivers/net/designware_eqos.h |  7 ++++
 2 files changed, 63 insertions(+), 22 deletions(-)

-- 
2.39.2

[PATCH v2 1/5] net: designware: eqos: initialize MAC address specific DMA channel configuration

[Oleksij Rempel]

Make sure we use predictable DMA Channel Select configuration. Otherwise
bad things may happen.
So far this fix is not related to any known issue and was noticed by
investigating other bugs.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/designware_eqos.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/designware_eqos.c b/drivers/net/designware_eqos.c
index 5e5c9ebe68..845f9f51ef 100644
--- a/drivers/net/designware_eqos.c
+++ b/drivers/net/designware_eqos.c
@@ -351,6 +351,11 @@ int eqos_set_ethaddr(struct eth_device *edev, const unsigned char *mac)
 
 	memcpy(eqos->macaddr, mac, ETH_ALEN);
 
+	/* mac_hi is only partially overwritten by the following code. Part of
+	 * this variable is DCS (DMA Channel Select). If this variable is not
+	 * zeroed, we may get some random DMA RX channel.
+	 */
+	mac_hi = 0;
 	/* Update the MAC address */
 	memcpy(&mac_hi, &mac[4], 2);
 	memcpy(&mac_lo, &mac[0], 4);
-- 
2.39.2

[PATCH v2 2/5] net: designware: eqos: fix non-working promisc mode when set before interface start

[Oleksij Rempel]

Promisc mode is not working if set before starting interface. This seen
more with DSA switch driver when having many interfaces and different
MACs.

Make promisc mode work by saving configs before soft reset, then use
them after reset.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/designware_eqos.c | 12 +++++++++++-
 drivers/net/designware_eqos.h |  3 +++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/designware_eqos.c b/drivers/net/designware_eqos.c
index 845f9f51ef..6caf3a436f 100644
--- a/drivers/net/designware_eqos.c
+++ b/drivers/net/designware_eqos.c
@@ -351,6 +351,9 @@ int eqos_set_ethaddr(struct eth_device *edev, const unsigned char *mac)
 
 	memcpy(eqos->macaddr, mac, ETH_ALEN);
 
+	if (!eqos->is_started)
+		return 0;
+
 	/* mac_hi is only partially overwritten by the following code. Part of
 	 * this variable is DCS (DMA Channel Select). If this variable is not
 	 * zeroed, we may get some random DMA RX channel.
@@ -371,6 +374,11 @@ static int eqos_set_promisc(struct eth_device *edev, bool enable)
 	struct eqos *eqos = edev->priv;
 	u32 mask;
 
+	eqos->promisc_enabled = enable;
+
+	if (!eqos->is_started)
+		return 0;
+
 	mask = EQOS_MAC_PACKET_FILTER_PR | EQOS_MAC_PACKET_FILTER_PCF;
 
 	if (enable)
@@ -429,8 +437,10 @@ static int eqos_start(struct eth_device *edev)
 		return ret;
 	}
 
-	/* Reset above clears MAC address */
+	/* Reset above clears any previously made configuration */
+	eqos->is_started = true;
 	eqos_set_ethaddr(edev, eqos->macaddr);
+	eqos_set_promisc(edev, eqos->promisc_enabled);
 
 	/* Required for accurate time keeping with EEE counters */
 	rate = eqos->ops->get_csr_clk_rate(eqos);
diff --git a/drivers/net/designware_eqos.h b/drivers/net/designware_eqos.h
index 31d0dc8632..58ba912cd0 100644
--- a/drivers/net/designware_eqos.h
+++ b/drivers/net/designware_eqos.h
@@ -60,6 +60,9 @@ struct eqos {
 
 	const struct eqos_ops *ops;
 	void *priv;
+
+	bool is_started;
+	bool promisc_enabled;
 };
 
 struct device;
-- 
2.39.2

[PATCH v2 3/5] net: designware: eqos: add comment about external clock dependencies for the soft reset

[Oleksij Rempel]

This part of code is not error proof and may fail depending on
implementation state of external HW. I hope this note help to find bugs
later.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/designware_eqos.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/designware_eqos.c b/drivers/net/designware_eqos.c
index 6caf3a436f..4489725e87 100644
--- a/drivers/net/designware_eqos.c
+++ b/drivers/net/designware_eqos.c
@@ -427,6 +427,10 @@ static int eqos_start(struct eth_device *edev)
 	if (ret)
 		return ret;
 
+	/* In some cases where PHY or DSA switch is the clock provider for
+	 * EQOS, we need to probe and configure them before issuing software
+	 * reset here.
+	 */
 	setbits_le32(&eqos->dma_regs->mode, EQOS_DMA_MODE_SWR);
 
 	ret = readl_poll_timeout(&eqos->dma_regs->mode, mode_set,
-- 
2.39.2

[PATCH v2 4/5] net: designware: eqos: fix NULL pointer dereference on LLDP packets

[Oleksij Rempel]

If promisc mode is enabled (which is enabled for DSA switches by default) a
LLDP frame received by barebox will trigger following panic:
 DABT (current EL) exception (ESR 0x9600014b) at 0x0000000000000000
 elr: 00000000bfd967d8 lr : 00000000bfd963e0
 x0 : 0000000000000000 x1 : 00000000000000e9
 x2 : 0000000000000040 x3 : 000000000000003f
 x4 : 0000000000000000 x5 : 00000000000001a0
 x6 : 0000000000000005 x7 : 0000000030200000
 x8 : 000000007feda6a8 x9 : 0000000000000000
 x10: 0000000000000000 x11: 00000000fffffffa
 x12: 00000000fffffffa x13: 0000000000000020
 x14: 0000000000000000 x15: 0000000000000001
 x16: 00000000bfff74c8 x17: 0000000000000001
 x18: 00000000bfff7a60 x19: 000000007fee20c0
 x20: 0000000000000000 x21: 000000007fee0dc8
 x22: 000000007fee0dc8 x23: 00000000000000ea
 x24: 0000000000000080 x25: 0000000000000000
 x26: 00000000000000ea x27: 000000007fee2040
 x28: 0000000000000000 x29: 00000000bfff7a60
 Call trace:
 [<bfd967d8>] (v8_inv_dcache_range+0x1c/0x34) from [<bfd953fc>] (arch_sync_dma_for_cpu+0x14/0x20)
 [<bfd953fc>] (arch_sync_dma_for_cpu+0x14/0x20) from [<bfd25a64>] (eqos_recv+0x78/0x104)
 [<bfd25a64>] (eqos_recv+0x78/0x104) from [<bfd80724>] (eth_rx+0xd0/0x160)
 [<bfd80724>] (eth_rx+0xd0/0x160) from [<bfd80b84>] (net_poll+0x24/0x34)
 [<bfd80b84>] (net_poll+0x24/0x34) from [<bfd80bbc>] (__net_poll+0x28/0x3c)
 [<bfd80bbc>] (__net_poll+0x28/0x3c) from [<bfd0ec7c>] (poller_call+0x58/0x68)
 [<bfd0ec7c>] (poller_call+0x58/0x68) from [<bfd0ea98>] (resched+0x38/0x48)
 [<bfd0ea98>] (resched+0x38/0x48) from [<bfd77be0>] (readline+0xb4/0x89c)
 [<bfd77be0>] (readline+0xb4/0x89c) from [<bfd0f17c>] (file_get+0x94/0x1d8)
 [<bfd0f17c>] (file_get+0x94/0x1d8) from [<bfd0f92c>] (parse_stream.constprop.0+0x40/0x534)
 [<bfd0f92c>] (parse_stream.constprop.0+0x40/0x534) from [<bfd0ff10>] (parse_stream_outer+0xf0/0x1ec)
 [<bfd0ff10>] (parse_stream_outer+0xf0/0x1ec) from [<bfd10e0c>] (run_shell+0x60/0x98)
 [<bfd10e0c>] (run_shell+0x60/0x98) from [<bfd01854>] (run_init+0x170/0x2b0)
 [<bfd01854>] (run_init+0x170/0x2b0) from [<bfd019e4>] (start_barebox+0x50/0x8c)
 [<bfd019e4>] (start_barebox+0x50/0x8c) from [<bfd95794>] (barebox_non_pbl_start+0x11c/0x150)
 [<bfd95794>] (barebox_non_pbl_start+0x11c/0x150) from [<bfd0000c>] (__bare_init_start+0x0/0x4)
 [<bfd0000c>] (__bare_init_start+0x0/0x4) from [<402041fc>] (0x402041fc)
 [<402041fc>] (0x402041fc) from [<40203bac>] (0x40203bac)
 panic: unhandled exception

This issue can be reproduced by using following command and link partner:
mausezahn enp1s0f1 01:80:c2:00:00:0e:08:60:6e:1f:a3:9c:88:cc

The problem caused this issue is wrong or may be different (depending on the HW)
interpretation of DMA RX buffers.  The DMA descriptors have distinct formats
depending on their direction: Read Format for CPU-to-DMA and Write-Back Format
for DMA-to-CPU. Previously, the driver did not distinguish between these,
leading to misinterpretations of the descriptor fields. For example the
driver expected a DMA buffer pointer in the Write-Back desc0 which is
actually a VLAN tag descriptor and contains artifacts of DMA buffer
pointer only by accident. To fix it we should store DMA buffer pointers
in a separate array and use them. To prevent more of misunderstandings,
i renamed variables to make it visible what format of DMA buffer we
actually using.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/designware_eqos.c | 55 ++++++++++++++++++++++-------------
 drivers/net/designware_eqos.h |  4 +++
 2 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/drivers/net/designware_eqos.c b/drivers/net/designware_eqos.c
index 4489725e87..825c8e0140 100644
--- a/drivers/net/designware_eqos.c
+++ b/drivers/net/designware_eqos.c
@@ -172,8 +172,6 @@ struct eqos_dma_regs {
 #define EQOS_DESCRIPTOR_SIZE	(EQOS_DESCRIPTOR_WORDS * 4)
 /* We assume ARCH_DMA_MINALIGN >= 16; 16 is the EQOS HW minimum */
 #define EQOS_DESCRIPTOR_ALIGN	64
-#define EQOS_DESCRIPTORS_TX	4
-#define EQOS_DESCRIPTORS_RX	64
 #define EQOS_DESCRIPTORS_NUM	(EQOS_DESCRIPTORS_TX + EQOS_DESCRIPTORS_RX)
 #define EQOS_DESCRIPTORS_SIZE	ALIGN(EQOS_DESCRIPTORS_NUM * \
 				      EQOS_DESCRIPTOR_SIZE, EQOS_DESCRIPTOR_ALIGN)
@@ -416,7 +414,7 @@ static int eqos_start(struct eth_device *edev)
 {
 	struct eqos *eqos = edev->priv;
 	u32 val, tx_fifo_sz, rx_fifo_sz, tqs, rqs, pbl;
-	unsigned long last_rx_desc;
+	unsigned long last_rx_rf_desc;
 	unsigned long rate;
 	u32 mode_set;
 	int ret;
@@ -626,9 +624,9 @@ static int eqos_start(struct eth_device *edev)
 	eqos->tx_currdescnum = eqos->rx_currdescnum = 0;
 
 	for (i = 0; i < EQOS_DESCRIPTORS_RX; i++) {
-		struct eqos_desc *rx_desc = &eqos->rx_descs[i];
+		struct eqos_desc *rx_rf_desc = &eqos->rx_descs[i];
 
-		writel(EQOS_DESC3_BUF1V | EQOS_DESC3_OWN, &rx_desc->des3);
+		writel(EQOS_DESC3_BUF1V | EQOS_DESC3_OWN, &rx_rf_desc->des3);
 	}
 
 	writel(0, &eqos->dma_regs->ch0_txdesc_list_haddress);
@@ -658,8 +656,8 @@ static int eqos_start(struct eth_device *edev)
 	 * that's not distinguishable from none of the descriptors being
 	 * available.
 	 */
-	last_rx_desc = (ulong)&eqos->rx_descs[(EQOS_DESCRIPTORS_RX - 1)];
-	writel(last_rx_desc, &eqos->dma_regs->ch0_rxdesc_tail_pointer);
+	last_rx_rf_desc = (ulong)&eqos->rx_descs[(EQOS_DESCRIPTORS_RX - 1)];
+	writel(last_rx_rf_desc, &eqos->dma_regs->ch0_rxdesc_tail_pointer);
 
 	return 0;
 }
@@ -746,16 +744,30 @@ static int eqos_send(struct eth_device *edev, void *packet, int length)
 static int eqos_recv(struct eth_device *edev)
 {
 	struct eqos *eqos = edev->priv;
-	struct eqos_desc *rx_desc;
+	struct eqos_desc *rx_wbf_desc, *rx_rf_desc;
+	dma_addr_t dma;
 	void *frame;
 	int length;
 
-	rx_desc = &eqos->rx_descs[eqos->rx_currdescnum];
-	if (readl(&rx_desc->des3) & EQOS_DESC3_OWN)
+	/* We have two types of RX descriptors at some pointer: Read and
+	 * Write-Back:
+	 * All RX descriptors are prepared by the software and given to the
+	 * DMA as "Normal" Descriptors with the content as shown in Receive
+	 * Normal Descriptor (Read Format). The DMA reads this descriptor and
+	 * after transferring a received packet (or part of) to the buffers
+	 * indicated by the descriptor, the Rx DMA will close the descriptor
+	 * with the corresponding packet status. The format of this status is
+	 * given in the "Receive Normal Descriptor (Write-Back Format)"
+	 */
+
+	/* Write-Back Format RX descriptor */
+	rx_wbf_desc = &eqos->rx_descs[eqos->rx_currdescnum];
+	if (readl(&rx_wbf_desc->des3) & EQOS_DESC3_OWN)
 		return 0;
 
-	frame = phys_to_virt(rx_desc->des0);
-	length = rx_desc->des3 & 0x7fff;
+	dma = eqos->dma_rx_buf[eqos->rx_currdescnum];
+	frame = phys_to_virt(dma);
+	length = rx_wbf_desc->des3 & 0x7fff;
 
 	dma_sync_single_for_cpu(edev->parent, (unsigned long)frame,
 				length, DMA_FROM_DEVICE);
@@ -763,18 +775,20 @@ static int eqos_recv(struct eth_device *edev)
 	dma_sync_single_for_device(edev->parent, (unsigned long)frame,
 				   length, DMA_FROM_DEVICE);
 
-	rx_desc->des0 = (unsigned long)frame;
-	rx_desc->des1 = 0;
-	rx_desc->des2 = 0;
+	/* Read Format RX descriptor */
+	rx_rf_desc = &eqos->rx_descs[eqos->rx_currdescnum];
+	rx_rf_desc->des0 = dma;
+	rx_rf_desc->des1 = 0;
+	rx_rf_desc->des2 = 0;
 	/*
 	 * Make sure that if HW sees the _OWN write below, it will see all the
 	 * writes to the rest of the descriptor too.
 	 */
-	rx_desc->des3 |= EQOS_DESC3_BUF1V;
-	rx_desc->des3 |= EQOS_DESC3_OWN;
+	rx_rf_desc->des3 |= EQOS_DESC3_BUF1V;
+	rx_rf_desc->des3 |= EQOS_DESC3_OWN;
 	barrier();
 
-	writel((ulong)rx_desc, &eqos->dma_regs->ch0_rxdesc_tail_pointer);
+	writel((ulong)rx_rf_desc, &eqos->dma_regs->ch0_rxdesc_tail_pointer);
 
 	eqos->rx_currdescnum++;
 	eqos->rx_currdescnum %= EQOS_DESCRIPTORS_RX;
@@ -802,7 +816,7 @@ static int eqos_init_resources(struct eqos *eqos)
 		goto err_free_desc;
 
 	for (i = 0; i < EQOS_DESCRIPTORS_RX; i++) {
-		struct eqos_desc *rx_desc = &eqos->rx_descs[i];
+		struct eqos_desc *rx_rf_desc = &eqos->rx_descs[i];
 		dma_addr_t dma;
 
 		dma = dma_map_single(edev->parent, p, EQOS_MAX_PACKET_SIZE, DMA_FROM_DEVICE);
@@ -811,7 +825,8 @@ static int eqos_init_resources(struct eqos *eqos)
 			goto err_free_rx_bufs;
 		}
 
-		rx_desc->des0 = dma;
+		rx_rf_desc->des0 = dma;
+		eqos->dma_rx_buf[i] = dma;
 
 		p += EQOS_MAX_PACKET_SIZE;
 	}
diff --git a/drivers/net/designware_eqos.h b/drivers/net/designware_eqos.h
index 58ba912cd0..951565e8f9 100644
--- a/drivers/net/designware_eqos.h
+++ b/drivers/net/designware_eqos.h
@@ -40,6 +40,9 @@ struct eqos_dma_regs;
 struct eqos_mac_regs;
 struct eqos_mtl_regs;
 
+#define EQOS_DESCRIPTORS_TX	4
+#define EQOS_DESCRIPTORS_RX	64
+
 struct eqos {
 	struct eth_device netdev;
 	struct mii_bus miibus;
@@ -49,6 +52,7 @@ struct eqos {
 	u32 tx_currdescnum, rx_currdescnum;
 
 	struct eqos_desc *tx_descs, *rx_descs;
+	dma_addr_t dma_rx_buf[EQOS_DESCRIPTORS_RX];
 
 	void __iomem *regs;
 	struct eqos_mac_regs __iomem *mac_regs;
-- 
2.39.2

[PATCH v2 5/5] net: designware: eqos: do not receive pause frames

[Oleksij Rempel]

Normally we need to care only about packets with not local MAC address
destination. It is needed to support HW setups with multiple MAC
addresses forwarded over one MAC. For example systems using DSA switch
as port multiplexer. In this case one single MAC should handle packets
with different MAC address destinations.

This functionality is provided by the EQOS_MAC_PACKET_FILTER_PR.
EQOS_MAC_PACKET_FILTER_PCF on other hand allow to capture ethernet
control frames like pause frames which are not handled by barebox.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/designware_eqos.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/designware_eqos.c b/drivers/net/designware_eqos.c
index 825c8e0140..ccce51b6af 100644
--- a/drivers/net/designware_eqos.c
+++ b/drivers/net/designware_eqos.c
@@ -377,7 +377,7 @@ static int eqos_set_promisc(struct eth_device *edev, bool enable)
 	if (!eqos->is_started)
 		return 0;
 
-	mask = EQOS_MAC_PACKET_FILTER_PR | EQOS_MAC_PACKET_FILTER_PCF;
+	mask = EQOS_MAC_PACKET_FILTER_PR;
 
 	if (enable)
 		setbits_le32(&eqos->mac_regs->packet_filter, mask);
-- 
2.39.2

Re: [PATCH v2 0/5] net: designware: eqos: fixes

[Sascha Hauer]

On Mon, Aug 14, 2023 at 07:32:24AM +0200, Oleksij Rempel wrote:
> changes v2:
> - spelling fixes in the "initialize MAC address" patch
> 
> Oleksij Rempel (5):
>   net: designware: eqos: initialize MAC address specific DMA channel
>     configuration
>   net: designware: eqos: fix non-working promisc mode when set before
>     interface start
>   net: designware: eqos: add comment about external clock dependencies
>     for the soft reset
>   net: designware: eqos: fix NULL pointer dereference on LLDP packets
>   net: designware: eqos: do not receive pause frames

Applied, thanks

Sascha

> 
>  drivers/net/designware_eqos.c | 78 +++++++++++++++++++++++++----------
>  drivers/net/designware_eqos.h |  7 ++++
>  2 files changed, 63 insertions(+), 22 deletions(-)
> 
> -- 
> 2.39.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |