[PATCH net-next 0/2] net: dsa: mxl862xx: MDIO bus integrity and optimization

[PATCH net-next 0/2] net: dsa: mxl862xx: MDIO bus integrity and optimization

[Daniel Golle]

The MxL862xx firmware offers opt-in CRC validation on the MDIO/MMD
command interface to guard against bit errors on the bus.
The driver has not used this until now.

This series enables CRC protection on both the command registers (CRC-6)
and data payloads (CRC-16), and reduces MDIO bus traffic by bulk-zeroing
registers instead of writing zero-valued words individually.

Daniel Golle (2):
  net: dsa: mxl862xx: add CRC for MDIO communication
  net: dsa: mxl862xx: use RST_DATA to skip writing zero words

 drivers/net/dsa/mxl862xx/Kconfig         |   1 +
 drivers/net/dsa/mxl862xx/mxl862xx-host.c | 365 ++++++++++++++++++-----
 2 files changed, 297 insertions(+), 69 deletions(-)

-- 
2.53.0

[PATCH net-next 1/2] net: dsa: mxl862xx: add CRC for MDIO communication

[Daniel Golle]

Enable the firmware's opt-in CRC validation on the MDIO/MMD command
interface to detect bit errors on the bus. The firmware bundles CRC-6
and CRC-16 under a single enable flag, so both are implemented together.

CRC-6 protects the ctrl and len_ret command registers using a table-
driven 3GPP algorithm. It is applied to every command exchange including
SET_DATA/GET_DATA batch transfers. With CRC enabled, the firmware
encodes its return value as a signed 11-bit integer within the CRC-
protected register fields, replacing the previous 16-bit interpretation.

CRC-16 protects the data payload using the kernel's crc16() library.
The driver appends a CRC-16 checksum to outgoing data and verifies the
firmware-appended checksum on responses. The checksum is placed at the
exact byte offset where the struct data ends, correctly handling packed
structs with odd sizes by splitting the checksum across word boundaries.
SET_DATA/GET_DATA sub-commands carry only CRC-6.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/dsa/mxl862xx/Kconfig         |   1 +
 drivers/net/dsa/mxl862xx/mxl862xx-host.c | 333 ++++++++++++++++++-----
 2 files changed, 262 insertions(+), 72 deletions(-)

diff --git a/drivers/net/dsa/mxl862xx/Kconfig b/drivers/net/dsa/mxl862xx/Kconfig
index 3e772298cc894..e51a67a3cf9bf 100644
--- a/drivers/net/dsa/mxl862xx/Kconfig
+++ b/drivers/net/dsa/mxl862xx/Kconfig
@@ -2,6 +2,7 @@
 config NET_DSA_MXL862
 	tristate "MaxLinear MxL862xx"
 	depends on NET_DSA
+	select CRC16
 	select NET_DSA_TAG_MXL_862XX
 	help
 	  This enables support for the MaxLinear MxL862xx switch family.
diff --git a/drivers/net/dsa/mxl862xx/mxl862xx-host.c b/drivers/net/dsa/mxl862xx/mxl862xx-host.c
index 4eefd2a759a7d..19ebb1dd724dd 100644
--- a/drivers/net/dsa/mxl862xx/mxl862xx-host.c
+++ b/drivers/net/dsa/mxl862xx/mxl862xx-host.c
@@ -7,7 +7,9 @@
  * Copyright (C) 2024 MaxLinear Inc.
  */
 
+#include <linux/bitops.h>
 #include <linux/bits.h>
+#include <linux/crc16.h>
 #include <linux/iopoll.h>
 #include <linux/limits.h>
 #include <net/dsa.h>
@@ -15,6 +17,9 @@
 #include "mxl862xx-host.h"
 
 #define CTRL_BUSY_MASK			BIT(15)
+#define CTRL_CRC_FLAG			BIT(14)
+
+#define LEN_RET_LEN_MASK		GENMASK(9, 0)
 
 #define MXL862XX_MMD_REG_CTRL		0
 #define MXL862XX_MMD_REG_LEN_RET	1
@@ -27,7 +32,142 @@
 #define MMD_API_GET_DATA_0		5
 #define MMD_API_RST_DATA		8
 
-#define MXL862XX_SWITCH_RESET 0x9907
+#define MXL862XX_SWITCH_RESET		0x9907
+
+/* Firmware CRC error codes (outside normal Zephyr errno range). */
+#define MXL862XX_FW_CRC6_ERR		(-1024)
+#define MXL862XX_FW_CRC16_ERR		(-1023)
+
+/* 3GPP CRC-6 lookup table (polynomial 0x6F).
+ * Matches the firmware's default CRC-6 implementation.
+ */
+static const u8 mxl862xx_crc6_table[256] = {
+	0x00, 0x2f, 0x31, 0x1e, 0x0d, 0x22, 0x3c, 0x13,
+	0x1a, 0x35, 0x2b, 0x04, 0x17, 0x38, 0x26, 0x09,
+	0x34, 0x1b, 0x05, 0x2a, 0x39, 0x16, 0x08, 0x27,
+	0x2e, 0x01, 0x1f, 0x30, 0x23, 0x0c, 0x12, 0x3d,
+	0x07, 0x28, 0x36, 0x19, 0x0a, 0x25, 0x3b, 0x14,
+	0x1d, 0x32, 0x2c, 0x03, 0x10, 0x3f, 0x21, 0x0e,
+	0x33, 0x1c, 0x02, 0x2d, 0x3e, 0x11, 0x0f, 0x20,
+	0x29, 0x06, 0x18, 0x37, 0x24, 0x0b, 0x15, 0x3a,
+	0x0e, 0x21, 0x3f, 0x10, 0x03, 0x2c, 0x32, 0x1d,
+	0x14, 0x3b, 0x25, 0x0a, 0x19, 0x36, 0x28, 0x07,
+	0x3a, 0x15, 0x0b, 0x24, 0x37, 0x18, 0x06, 0x29,
+	0x20, 0x0f, 0x11, 0x3e, 0x2d, 0x02, 0x1c, 0x33,
+	0x09, 0x26, 0x38, 0x17, 0x04, 0x2b, 0x35, 0x1a,
+	0x13, 0x3c, 0x22, 0x0d, 0x1e, 0x31, 0x2f, 0x00,
+	0x3d, 0x12, 0x0c, 0x23, 0x30, 0x1f, 0x01, 0x2e,
+	0x27, 0x08, 0x16, 0x39, 0x2a, 0x05, 0x1b, 0x34,
+	0x1c, 0x33, 0x2d, 0x02, 0x11, 0x3e, 0x20, 0x0f,
+	0x06, 0x29, 0x37, 0x18, 0x0b, 0x24, 0x3a, 0x15,
+	0x28, 0x07, 0x19, 0x36, 0x25, 0x0a, 0x14, 0x3b,
+	0x32, 0x1d, 0x03, 0x2c, 0x3f, 0x10, 0x0e, 0x21,
+	0x1b, 0x34, 0x2a, 0x05, 0x16, 0x39, 0x27, 0x08,
+	0x01, 0x2e, 0x30, 0x1f, 0x0c, 0x23, 0x3d, 0x12,
+	0x2f, 0x00, 0x1e, 0x31, 0x22, 0x0d, 0x13, 0x3c,
+	0x35, 0x1a, 0x04, 0x2b, 0x38, 0x17, 0x09, 0x26,
+	0x12, 0x3d, 0x23, 0x0c, 0x1f, 0x30, 0x2e, 0x01,
+	0x08, 0x27, 0x39, 0x16, 0x05, 0x2a, 0x34, 0x1b,
+	0x26, 0x09, 0x17, 0x38, 0x2b, 0x04, 0x1a, 0x35,
+	0x3c, 0x13, 0x0d, 0x22, 0x31, 0x1e, 0x00, 0x2f,
+	0x15, 0x3a, 0x24, 0x0b, 0x18, 0x37, 0x29, 0x06,
+	0x0f, 0x20, 0x3e, 0x11, 0x02, 0x2d, 0x33, 0x1c,
+	0x21, 0x0e, 0x10, 0x3f, 0x2c, 0x03, 0x1d, 0x32,
+	0x3b, 0x14, 0x0a, 0x25, 0x36, 0x19, 0x07, 0x28,
+};
+
+/* Compute 3GPP CRC-6 over the ctrl register (16 bits) and the lower
+ * 10 bits of the len_ret register. The 26-bit input is packed as
+ * { len_ret[9:0], ctrl[15:0] } and processed LSB-first through the
+ * lookup table.
+ */
+static u8 mxl862xx_crc6(u16 ctrl, u16 len_ret)
+{
+	u32 data = ((u32)(len_ret & LEN_RET_LEN_MASK) << 16) | ctrl;
+	u8 crc = 0;
+	int i;
+
+	for (i = 0; i < sizeof(data); i++, data >>= 8)
+		crc = mxl862xx_crc6_table[(crc << 2) ^ (data & 0xff)] & 0x3f;
+
+	return crc;
+}
+
+/* Encode CRC-6 into the ctrl and len_ret registers before writing them
+ * to MDIO. The caller must set ctrl = API_ID | CTRL_BUSY_MASK |
+ * CTRL_CRC_FLAG, and len_ret = parameter length (bits 0-9 only).
+ *
+ * After encoding:
+ *   ctrl[12:0]     = API ID (unchanged)
+ *   ctrl[14:13]    = CRC-6 bits 5-4
+ *   ctrl[15]       = busy flag (unchanged)
+ *   len_ret[9:0]   = parameter length (unchanged)
+ *   len_ret[13:10] = CRC-6 bits 3-0
+ *   len_ret[14]    = original ctrl[14] (CRC check flag, forwarded to FW)
+ *   len_ret[15]    = original ctrl[13] (magic bit, always 1)
+ */
+static void mxl862xx_crc6_encode(u16 *pctrl, u16 *plen_ret)
+{
+	u16 crc, ctrl, len_ret;
+
+	/* Set magic bit before CRC computation */
+	*pctrl |= BIT(13);
+
+	crc = mxl862xx_crc6(*pctrl, *plen_ret);
+
+	/* Place CRC MSB (bits 5-4) into ctrl bits 13-14 */
+	ctrl = (*pctrl & ~GENMASK(14, 13));
+	ctrl |= (crc & 0x30) << 9;
+
+	/* Place CRC LSB (bits 3-0) into len_ret bits 10-13 */
+	len_ret = *plen_ret | ((crc & 0x0f) << 10);
+
+	/* Forward ctrl[14] (CRC check flag) to len_ret[14],
+	 * and ctrl[13] (magic, always 1) to len_ret[15].
+	 */
+	len_ret |= (*pctrl & BIT(14)) | ((*pctrl & BIT(13)) << 2);
+
+	*pctrl = ctrl;
+	*plen_ret = len_ret;
+}
+
+/* Verify CRC-6 on a firmware response and extract the return value.
+ *
+ * The firmware encodes the return value as a signed 11-bit integer:
+ *   - Sign bit (bit 10) in ctrl[14]
+ *   - Magnitude (bits 9-0) in len_ret[9:0]
+ * These are recoverable after CRC-6 verification by restoring the
+ * original ctrl from the auxiliary copies in len_ret[15:14].
+ *
+ * Return: 0 on CRC match (with *result set), or -EIO on mismatch.
+ */
+static int mxl862xx_crc6_verify(u16 ctrl, u16 len_ret, int *result)
+{
+	u16 crc_recv, crc_calc;
+
+	/* Extract the received CRC-6 */
+	crc_recv = ((ctrl >> 9) & 0x30) | ((len_ret >> 10) & 0x0f);
+
+	/* Reconstruct the original ctrl for re-computation:
+	 *   ctrl[14] = len_ret[14] (sign bit / CRC check flag)
+	 *   ctrl[13] = len_ret[15] >> 2 (magic bit)
+	 */
+	ctrl &= ~GENMASK(14, 13);
+	ctrl |= len_ret & BIT(14);
+	ctrl |= (len_ret & BIT(15)) >> 2;
+
+	crc_calc = mxl862xx_crc6(ctrl, len_ret);
+	if (crc_recv != crc_calc)
+		return -EIO;
+
+	/* Extract signed 11-bit return value:
+	 *   bit 10 (sign) from ctrl[14], bits 9-0 from len_ret[9:0]
+	 */
+	*result = sign_extend32((len_ret & LEN_RET_LEN_MASK) |
+				((ctrl & CTRL_CRC_FLAG) >> 4), 10);
+
+	return 0;
+}
 
 static int mxl862xx_reg_read(struct mxl862xx_priv *priv, u32 addr)
 {
@@ -52,60 +192,78 @@ static int mxl862xx_busy_wait(struct mxl862xx_priv *priv)
 				  !(val & CTRL_BUSY_MASK), 15, 500000);
 }
 
-static int mxl862xx_set_data(struct mxl862xx_priv *priv, u16 words)
+/* Issue a firmware command with CRC-6 protection on the ctrl and len_ret
+ * registers, wait for completion, and verify the response CRC-6.
+ *
+ * Return: firmware result value (>= 0) on success, or negative errno.
+ */
+static int mxl862xx_issue_cmd(struct mxl862xx_priv *priv, u16 cmd, u16 len)
 {
-	int ret;
-	u16 cmd;
+	u16 ctrl_enc, len_enc;
+	int ret, fw_result;
+
+	ctrl_enc = cmd | CTRL_BUSY_MASK | CTRL_CRC_FLAG;
+	len_enc = len;
+	mxl862xx_crc6_encode(&ctrl_enc, &len_enc);
+
+	ret = mxl862xx_reg_write(priv, MXL862XX_MMD_REG_LEN_RET, len_enc);
+	if (ret < 0)
+		return ret;
+
+	ret = mxl862xx_reg_write(priv, MXL862XX_MMD_REG_CTRL, ctrl_enc);
+	if (ret < 0)
+		return ret;
+
+	ret = mxl862xx_busy_wait(priv);
+	if (ret < 0)
+		return ret;
+
+	ret = mxl862xx_reg_read(priv, MXL862XX_MMD_REG_CTRL);
+	if (ret < 0)
+		return ret;
+	ctrl_enc = ret;
 
-	ret = mxl862xx_reg_write(priv, MXL862XX_MMD_REG_LEN_RET,
-				 MXL862XX_MMD_REG_DATA_MAX_SIZE * sizeof(u16));
+	ret = mxl862xx_reg_read(priv, MXL862XX_MMD_REG_LEN_RET);
 	if (ret < 0)
 		return ret;
+	len_enc = ret;
+
+	ret = mxl862xx_crc6_verify(ctrl_enc, len_enc, &fw_result);
+	if (ret < 0) {
+		dev_err(&priv->mdiodev->dev,
+			"CRC-6 error on response to CMD %04x\n", cmd);
+		return -EIO;
+	}
+
+	return fw_result;
+}
+
+static int mxl862xx_set_data(struct mxl862xx_priv *priv, u16 words)
+{
+	u16 cmd;
 
 	cmd = words / MXL862XX_MMD_REG_DATA_MAX_SIZE - 1;
 	if (!(cmd < 2))
 		return -EINVAL;
 
 	cmd += MMD_API_SET_DATA_0;
-	ret = mxl862xx_reg_write(priv, MXL862XX_MMD_REG_CTRL,
-				 cmd | CTRL_BUSY_MASK);
-	if (ret < 0)
-		return ret;
 
-	return mxl862xx_busy_wait(priv);
+	return mxl862xx_issue_cmd(priv, cmd,
+				  MXL862XX_MMD_REG_DATA_MAX_SIZE * sizeof(u16));
 }
 
 static int mxl862xx_get_data(struct mxl862xx_priv *priv, u16 words)
 {
-	int ret;
 	u16 cmd;
 
-	ret = mxl862xx_reg_write(priv, MXL862XX_MMD_REG_LEN_RET,
-				 MXL862XX_MMD_REG_DATA_MAX_SIZE * sizeof(u16));
-	if (ret < 0)
-		return ret;
-
 	cmd = words / MXL862XX_MMD_REG_DATA_MAX_SIZE;
 	if (!(cmd > 0 && cmd < 3))
 		return -EINVAL;
 
 	cmd += MMD_API_GET_DATA_0;
-	ret = mxl862xx_reg_write(priv, MXL862XX_MMD_REG_CTRL,
-				 cmd | CTRL_BUSY_MASK);
-	if (ret < 0)
-		return ret;
 
-	return mxl862xx_busy_wait(priv);
-}
-
-static int mxl862xx_firmware_return(int ret)
-{
-	/* Only 16-bit values are valid. */
-	if (WARN_ON(ret & GENMASK(31, 16)))
-		return -EINVAL;
-
-	/* Interpret value as signed 16-bit integer. */
-	return (s16)ret;
+	return mxl862xx_issue_cmd(priv, cmd,
+				  MXL862XX_MMD_REG_DATA_MAX_SIZE * sizeof(u16));
 }
 
 static int mxl862xx_send_cmd(struct mxl862xx_priv *priv, u16 cmd, u16 size,
@@ -113,33 +271,33 @@ static int mxl862xx_send_cmd(struct mxl862xx_priv *priv, u16 cmd, u16 size,
 {
 	int ret;
 
-	ret = mxl862xx_reg_write(priv, MXL862XX_MMD_REG_LEN_RET, size);
-	if (ret)
-		return ret;
+	ret = mxl862xx_issue_cmd(priv, cmd, size);
 
-	ret = mxl862xx_reg_write(priv, MXL862XX_MMD_REG_CTRL,
-				 cmd | CTRL_BUSY_MASK);
-	if (ret)
-		return ret;
-
-	ret = mxl862xx_busy_wait(priv);
-	if (ret)
-		return ret;
-
-	ret = mxl862xx_reg_read(priv, MXL862XX_MMD_REG_LEN_RET);
-	if (ret < 0)
-		return ret;
-
-	/* handle errors returned by the firmware as -EIO
+	/* Handle errors returned by the firmware as -EIO.
 	 * The firmware is based on Zephyr OS and uses the errors as
 	 * defined in errno.h of Zephyr OS. See
 	 * https://github.com/zephyrproject-rtos/zephyr/blob/v3.7.0/lib/libc/minimal/include/errno.h
+	 *
+	 * The firmware signals CRC validation failures with dedicated
+	 * error codes outside the normal Zephyr errno range:
+	 *   -1024: CRC-6 mismatch on ctrl/len_ret registers
+	 *   -1023: CRC-16 mismatch on data payload
 	 */
-	ret = mxl862xx_firmware_return(ret);
 	if (ret < 0) {
-		if (!quiet)
-			dev_err(&priv->mdiodev->dev,
-				"CMD %04x returned error %d\n", cmd, ret);
+		if (!quiet) {
+			if (ret == MXL862XX_FW_CRC6_ERR)
+				dev_err(&priv->mdiodev->dev,
+					"CMD %04x: firmware detected CRC-6 error\n",
+					cmd);
+			else if (ret == MXL862XX_FW_CRC16_ERR)
+				dev_err(&priv->mdiodev->dev,
+					"CMD %04x: firmware detected CRC-16 error\n",
+					cmd);
+			else
+				dev_err(&priv->mdiodev->dev,
+					"CMD %04x returned error %d\n",
+					cmd, ret);
+		}
 		return -EIO;
 	}
 
@@ -151,7 +309,7 @@ int mxl862xx_api_wrap(struct mxl862xx_priv *priv, u16 cmd, void *_data,
 {
 	__le16 *data = _data;
 	int ret, cmd_ret;
-	u16 max, i;
+	u16 max, crc, i;
 
 	dev_dbg(&priv->mdiodev->dev, "CMD %04x DATA %*ph\n", cmd, size, data);
 
@@ -163,26 +321,42 @@ int mxl862xx_api_wrap(struct mxl862xx_priv *priv, u16 cmd, void *_data,
 	if (ret < 0)
 		goto out;
 
-	for (i = 0; i < max; i++) {
+	/* Compute CRC-16 over the data payload; written as an extra word
+	 * after the data so the firmware can verify the transfer.
+	 */
+	crc = crc16(0xffff, (const u8 *)data, size);
+
+	for (i = 0; i < max + 1; i++) {
 		u16 off = i % MXL862XX_MMD_REG_DATA_MAX_SIZE;
+		u16 val;
 
 		if (i && off == 0) {
-			/* Send command to set data when every
-			 * MXL862XX_MMD_REG_DATA_MAX_SIZE of WORDs are written.
-			 */
 			ret = mxl862xx_set_data(priv, i);
 			if (ret < 0)
 				goto out;
 		}
 
-		if ((i * 2 + 1) == size)
-			ret = mxl862xx_reg_write(priv,
-						 MXL862XX_MMD_REG_DATA_FIRST + off,
-						 *(u8 *)&data[i]);
-		else
-			ret = mxl862xx_reg_write(priv,
-						 MXL862XX_MMD_REG_DATA_FIRST + off,
-						 le16_to_cpu(data[i]));
+		if (i == max) {
+			/* Even size: full CRC word.
+			 * Odd size: only CRC high byte remains (low byte
+			 * was packed into the previous word).
+			 */
+			val = (size & 1) ? crc >> 8 : crc;
+		} else if ((i * 2 + 1) == size) {
+			/* Special handling for last BYTE if it's not WORD
+			 * aligned to avoid reading beyond the allocated data
+			 * structure.  Pack the CRC low byte into the high
+			 * byte of this word so it sits at byte offset 'size'
+			 * in the firmware's contiguous buffer.
+			 */
+			val = *(u8 *)&data[i] | ((crc & 0xff) << 8);
+		} else {
+			val = le16_to_cpu(data[i]);
+		}
+
+		ret = mxl862xx_reg_write(priv,
+					 MXL862XX_MMD_REG_DATA_FIRST + off,
+					 val);
 		if (ret < 0)
 			goto out;
 	}
@@ -194,13 +368,10 @@ int mxl862xx_api_wrap(struct mxl862xx_priv *priv, u16 cmd, void *_data,
 	/* store result of mxl862xx_send_cmd() */
 	cmd_ret = ret;
 
-	for (i = 0; i < max; i++) {
+	for (i = 0; i < max + 1; i++) {
 		u16 off = i % MXL862XX_MMD_REG_DATA_MAX_SIZE;
 
 		if (i && off == 0) {
-			/* Send command to fetch next batch of data when every
-			 * MXL862XX_MMD_REG_DATA_MAX_SIZE of WORDs are read.
-			 */
 			ret = mxl862xx_get_data(priv, i);
 			if (ret < 0)
 				goto out;
@@ -210,17 +381,35 @@ int mxl862xx_api_wrap(struct mxl862xx_priv *priv, u16 cmd, void *_data,
 		if (ret < 0)
 			goto out;
 
-		if ((i * 2 + 1) == size) {
+		if (i == max) {
+			/* Even size: full CRC word.
+			 * Odd size: only CRC high byte remains (low byte
+			 * was in the previous word).
+			 */
+			if (size & 1)
+				crc = (crc & 0x00ff) |
+				      (((u16)ret & 0xff) << 8);
+			else
+				crc = (u16)ret;
+		} else if ((i * 2 + 1) == size) {
 			/* Special handling for last BYTE if it's not WORD
 			 * aligned to avoid writing beyond the allocated data
-			 * structure.
+			 * structure.  The high byte carries the CRC low byte.
 			 */
 			*(uint8_t *)&data[i] = ret & 0xff;
+			crc = (ret >> 8) & 0xff;
 		} else {
 			data[i] = cpu_to_le16((u16)ret);
 		}
 	}
 
+	if (crc16(0xffff, (const u8 *)data, size) != crc) {
+		dev_err(&priv->mdiodev->dev,
+			"CRC-16 error on response data for CMD %04x\n", cmd);
+		ret = -EIO;
+		goto out;
+	}
+
 	/* on success return the result of the mxl862xx_send_cmd() */
 	ret = cmd_ret;
 
-- 
2.53.0

Re: [PATCH net-next 1/2] net: dsa: mxl862xx: add CRC for MDIO communication

[Andrew Lunn]

> +/* Firmware CRC error codes (outside normal Zephyr errno range). */
> +#define MXL862XX_FW_CRC6_ERR		(-1024)
> +#define MXL862XX_FW_CRC16_ERR		(-1023)
> +
> +/* 3GPP CRC-6 lookup table (polynomial 0x6F).
> + * Matches the firmware's default CRC-6 implementation.
> + */
> +static const u8 mxl862xx_crc6_table[256] = {
> +	0x00, 0x2f, 0x31, 0x1e, 0x0d, 0x22, 0x3c, 0x13,
> +	0x1a, 0x35, 0x2b, 0x04, 0x17, 0x38, 0x26, 0x09,

I had a quick look around and count not find this table anywhere
else. Mellanox has a different CRC-6 polynomial. Until somebody else
needs this, i think it is O.K. here. But sometime in the future it
might be moved into lib/crc.

> +	ret = mxl862xx_crc6_verify(ctrl_enc, len_enc, &fw_result);
> +	if (ret < 0) {
> +		dev_err(&priv->mdiodev->dev,
> +			"CRC-6 error on response to CMD %04x\n", cmd);
> +		return -EIO;
> +	}

This is the question, what to do when you see a checksum failure? The
basic assumption is MDIO is reliable. PHYs don't do any sort of
checksums, nor any other switches using MDIO. Why would you want
checksums?

To detect the hardware is broken? If so, is returning EIO sufficient?
Would it not be better to admin down all the interfaces?

To allow the MDIO clock to run at a higher frequency, at the limit of
the bus, so you get occasionally failures? If so, should you not
retry? But are the commands idempotent? Can you safely retry?

	Andrew

Re: [PATCH net-next 1/2] net: dsa: mxl862xx: add CRC for MDIO communication

[Daniel Golle]

On Sat, Mar 21, 2026 at 04:24:06PM +0100, Andrew Lunn wrote:
> > +/* Firmware CRC error codes (outside normal Zephyr errno range). */
> > +#define MXL862XX_FW_CRC6_ERR		(-1024)
> > +#define MXL862XX_FW_CRC16_ERR		(-1023)
> > +
> > +/* 3GPP CRC-6 lookup table (polynomial 0x6F).
> > + * Matches the firmware's default CRC-6 implementation.
> > + */
> > +static const u8 mxl862xx_crc6_table[256] = {
> > +	0x00, 0x2f, 0x31, 0x1e, 0x0d, 0x22, 0x3c, 0x13,
> > +	0x1a, 0x35, 0x2b, 0x04, 0x17, 0x38, 0x26, 0x09,
> 
> I had a quick look around and count not find this table anywhere
> else. Mellanox has a different CRC-6 polynomial. Until somebody else
> needs this, i think it is O.K. here. But sometime in the future it
> might be moved into lib/crc.
> 
> > +	ret = mxl862xx_crc6_verify(ctrl_enc, len_enc, &fw_result);
> > +	if (ret < 0) {
> > +		dev_err(&priv->mdiodev->dev,
> > +			"CRC-6 error on response to CMD %04x\n", cmd);
> > +		return -EIO;
> > +	}
> 
> This is the question, what to do when you see a checksum failure? The
> basic assumption is MDIO is reliable. PHYs don't do any sort of
> checksums, nor any other switches using MDIO. Why would you want
> checksums?
> 
> To detect the hardware is broken? If so, is returning EIO sufficient?
> Would it not be better to admin down all the interfaces?
> 
> To allow the MDIO clock to run at a higher frequency, at the limit of
> the bus, so you get occasionally failures? If so, should you not
> retry? But are the commands idempotent? Can you safely retry?
> 

Your guesses are all correct, and your concerns are justified.

Let me explain the whole picture:
The switch driver transfers many rather large data structures over
MDIO, and lacking support for interrupts (firmware doesn't support),
this is often even interleaved with polling 8 PHYs and at least one
PCS. To not have things get very slow (and then even sometimes see
-ETIMEDOUT breaking the PHY state machine if timing is unlucky), it
is common to overclock the MDIO bus to 20 MHz instead of the 2.5 MHz
default (the switch claims to support up to 25 MHz, but 20 Mhz is
sufficient and conservative enough).

That, combined with higher temperature of the switch IC (but still
within the spec'ed range), can lead to bit-errors -- which, in case
they remain unnoticed can introduce subtle (security relevant) issues
such as bridging ports which should not be bridged or flooding to a
port on which flooding should be disabled.

At this point I just wanted to make errors visible at all.

In case of the switch reporting back a CRC error for data received,
a limited number of retries would be ok in every case.
However, the same is not true for the opposite direction, ie. an
error detected by the Linux host for data received from the switch:
In case one of the *_ALLOC API calls we cannot simply repeat the
call, and as the data was corrupted, we wouldn't even know how to
undo the failed call. Resetting the switch and replaying the
complete state would be the only way to avoid a firmware resource
leak in this case.

Setting all interfaces to admin-down is probably the best compromise
in a case like this, as it would also reduce the energy consumption
and hence heat emission of the IC (as all built-in PHYs are then down;
that's where most of the heat comes from) and prevent damage -- I've
only observed CRC errors with the heatsink removed and artifically
overheating the IC...

Re: [PATCH net-next 1/2] net: dsa: mxl862xx: add CRC for MDIO communication

[Andrew Lunn]

> > This is the question, what to do when you see a checksum failure? The
> > basic assumption is MDIO is reliable. PHYs don't do any sort of
> > checksums, nor any other switches using MDIO. Why would you want
> > checksums?
> > 
> > To detect the hardware is broken? If so, is returning EIO sufficient?
> > Would it not be better to admin down all the interfaces?
> > 
> > To allow the MDIO clock to run at a higher frequency, at the limit of
> > the bus, so you get occasionally failures? If so, should you not
> > retry? But are the commands idempotent? Can you safely retry?
> > 
> 
> Your guesses are all correct, and your concerns are justified.
> 
> Let me explain the whole picture:
> The switch driver transfers many rather large data structures over
> MDIO, and lacking support for interrupts (firmware doesn't support),
> this is often even interleaved with polling 8 PHYs and at least one
> PCS. To not have things get very slow (and then even sometimes see
> -ETIMEDOUT breaking the PHY state machine if timing is unlucky), it
> is common to overclock the MDIO bus to 20 MHz instead of the 2.5 MHz
> default (the switch claims to support up to 25 MHz, but 20 Mhz is
> sufficient and conservative enough).
> 
> That, combined with higher temperature of the switch IC (but still
> within the spec'ed range), can lead to bit-errors -- which, in case
> they remain unnoticed can introduce subtle (security relevant) issues
> such as bridging ports which should not be bridged or flooding to a
> port on which flooding should be disabled.

O.K. "Interesting" design.

You could solve the PHY timeout issue by claiming to support PHY
interrupts, doing the polling in the DSA driver, and raise an
interrupt if the conditions are met. The mv88e6xxx driver does
something like this. It has an interrupt controller which the PHYs are
connect to. Some designs have the switch interrupt output connected to
a GPIO and so can do real interrupts. Some don't. Rather than have all
the internal PHYs polled one per second by phylib, the mv88e6xxx polls
the interrupt status register every 1/5 of a second and raises the
interrupts instead. Bot faster, and less MDIO transfers.

> In case of the switch reporting back a CRC error for data received,
> a limited number of retries would be ok in every case.
> However, the same is not true for the opposite direction, ie. an
> error detected by the Linux host for data received from the switch:
> In case one of the *_ALLOC API calls we cannot simply repeat the
> call, and as the data was corrupted, we wouldn't even know how to
> undo the failed call.

Seems like somebody did not think through the design. I assume the
vendor driver does not attempt a retry?

> Setting all interfaces to admin-down is probably the best compromise
> in a case like this, as it would also reduce the energy consumption
> and hence heat emission of the IC (as all built-in PHYs are then down;
> that's where most of the heat comes from) and prevent damage -- I've
> only observed CRC errors with the heatsink removed and artifically
> overheating the IC...

So in the normal use cases you don't expect CRC errors. That seems
like it should driver the design. Consider any CRCs as fatal and
shutdown.

	Andrew

Re: [PATCH net-next 1/2] net: dsa: mxl862xx: add CRC for MDIO communication

[Daniel Golle]

On Sat, Mar 21, 2026 at 08:29:20PM +0100, Andrew Lunn wrote:
> > > This is the question, what to do when you see a checksum failure? The
> > > basic assumption is MDIO is reliable. PHYs don't do any sort of
> > > checksums, nor any other switches using MDIO. Why would you want
> > > checksums?
> > > 
> > > To detect the hardware is broken? If so, is returning EIO sufficient?
> > > Would it not be better to admin down all the interfaces?
> > > 
> > > To allow the MDIO clock to run at a higher frequency, at the limit of
> > > the bus, so you get occasionally failures? If so, should you not
> > > retry? But are the commands idempotent? Can you safely retry?
> > > 
> > 
> > Your guesses are all correct, and your concerns are justified.
> > 
> > Let me explain the whole picture:
> > The switch driver transfers many rather large data structures over
> > MDIO, and lacking support for interrupts (firmware doesn't support),
> > this is often even interleaved with polling 8 PHYs and at least one
> > PCS. To not have things get very slow (and then even sometimes see
> > -ETIMEDOUT breaking the PHY state machine if timing is unlucky), it
> > is common to overclock the MDIO bus to 20 MHz instead of the 2.5 MHz
> > default (the switch claims to support up to 25 MHz, but 20 Mhz is
> > sufficient and conservative enough).
> > 
> > That, combined with higher temperature of the switch IC (but still
> > within the spec'ed range), can lead to bit-errors -- which, in case
> > they remain unnoticed can introduce subtle (security relevant) issues
> > such as bridging ports which should not be bridged or flooding to a
> > port on which flooding should be disabled.
> 
> O.K. "Interesting" design.

🤐

> 
> You could solve the PHY timeout issue by claiming to support PHY
> interrupts, doing the polling in the DSA driver, and raise an
> interrupt if the conditions are met. The mv88e6xxx driver does
> something like this. It has an interrupt controller which the PHYs are
> connect to. Some designs have the switch interrupt output connected to
> a GPIO and so can do real interrupts. Some don't. Rather than have all
> the internal PHYs polled one per second by phylib, the mv88e6xxx polls
> the interrupt status register every 1/5 of a second and raises the
> interrupts instead. Bot faster, and less MDIO transfers.

Sadly, the firmware doesn't provide any API for that. The *hardware*
would likely be capable, even has an external pin to signal interrupts
to the host. But no firmware API to setup/mask interrupts, nor to read
or clear the interrupt status.

Raw access to the relevant hardware registers representing the
interrupt controller also isn't an option, as the only documented raw
register access interface only covers the switch engine itself, and
another (undocumented) more broad debugging register access API only
allows write operations on a very limited range of registers (and the
firmware applies bitmasks for each of them individually) basically
covering the GPIO and LED controller parts...

Plus, most likely interrupts of the built-in PHYs are as broken as for
all other GPY2xx PHYs (see PHY driver comments), because it's more or
less the same IP.

tl;dr: Due to firmware and hardware limitation that's not an option.

> I assume the vendor driver does not attempt a retry?

The vendor driver doesn't retry, it merely reports CRC errors.
I thought about it, and retrying would be an option, as on pure WRITE
operations the host could retry sending (as the original request is
dropped by the firmware in case of a CRC error), and READ operations
could only repeat the data retreaval part, but not the actual command
itself. However, I don't think any of that is worth the added complexity.
If the we end up with CRC errors, something is very wrong, most likely
overheat, so trying to forcefully continue normal operation imho isn't
even a good idea even if somehow possible.

> 
> > Setting all interfaces to admin-down is probably the best compromise
> > in a case like this, as it would also reduce the energy consumption
> > and hence heat emission of the IC (as all built-in PHYs are then down;
> > that's where most of the heat comes from) and prevent damage -- I've
> > only observed CRC errors with the heatsink removed and artifically
> > overheating the IC...
> 
> So in the normal use cases you don't expect CRC errors. That seems
> like it should driver the design. Consider any CRCs as fatal and
> shutdown.

Ack, I also think that's the most reasonable thing to do.

As the shutdown itself can trigger more CRC errors (PHYs being put
into PDOWN, for example, will cause dozends of MDIO operations to
indirectly access the internal MDIO bus via the firmware interface...)
I will guard this using an atomic flag to prevent the ongoing shutdown
from triggering another shutdown...

[PATCH net-next 2/2] net: dsa: mxl862xx: use RST_DATA to skip writing zero words

[Daniel Golle]

Issue the firmware's RST_DATA command before writing data payloads that
contain many zero words. RST_DATA zeroes both the firmware's internal
buffer and the MMD data registers in a single command, allowing the
driver to skip individual MDIO writes for zero-valued words. This
reduces bus traffic for the common case where API structs have many
unused or default-zero fields.

The optimization is applied when at least 5 zero words are found in the
payload, roughly the break-even point against the cost of the extra
RST_DATA command round-trip.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/dsa/mxl862xx/mxl862xx-host.c | 38 ++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/net/dsa/mxl862xx/mxl862xx-host.c b/drivers/net/dsa/mxl862xx/mxl862xx-host.c
index 19ebb1dd724dd..cb129a182b0f7 100644
--- a/drivers/net/dsa/mxl862xx/mxl862xx-host.c
+++ b/drivers/net/dsa/mxl862xx/mxl862xx-host.c
@@ -266,6 +266,17 @@ static int mxl862xx_get_data(struct mxl862xx_priv *priv, u16 words)
 				  MXL862XX_MMD_REG_DATA_MAX_SIZE * sizeof(u16));
 }
 
+static int mxl862xx_rst_data(struct mxl862xx_priv *priv)
+{
+	return mxl862xx_issue_cmd(priv, MMD_API_RST_DATA, 0);
+}
+
+/* Minimum number of zero words in the data payload before issuing a
+ * RST_DATA command is worthwhile.  RST_DATA costs one full command
+ * round-trip (~5 MDIO transactions), so the threshold must offset that.
+ */
+#define RST_DATA_THRESHOLD	5
+
 static int mxl862xx_send_cmd(struct mxl862xx_priv *priv, u16 cmd, u16 size,
 			     bool quiet)
 {
@@ -308,6 +319,8 @@ int mxl862xx_api_wrap(struct mxl862xx_priv *priv, u16 cmd, void *_data,
 		      u16 size, bool read, bool quiet)
 {
 	__le16 *data = _data;
+	bool use_rst = false;
+	unsigned int zeros;
 	int ret, cmd_ret;
 	u16 max, crc, i;
 
@@ -321,6 +334,24 @@ int mxl862xx_api_wrap(struct mxl862xx_priv *priv, u16 cmd, void *_data,
 	if (ret < 0)
 		goto out;
 
+	/* If the data contains enough zero words, issue RST_DATA to zero
+	 * both the firmware buffer and MMD registers, then skip writing
+	 * zero words individually.
+	 */
+	for (i = 0, zeros = 0; i < size / 2 && zeros < RST_DATA_THRESHOLD; i++)
+		if (!data[i])
+			zeros++;
+
+	if (zeros < RST_DATA_THRESHOLD && (size & 1) && !*(u8 *)&data[i])
+		zeros++;
+
+	if (zeros >= RST_DATA_THRESHOLD) {
+		ret = mxl862xx_rst_data(priv);
+		if (ret < 0)
+			goto out;
+		use_rst = true;
+	}
+
 	/* Compute CRC-16 over the data payload; written as an extra word
 	 * after the data so the firmware can verify the transfer.
 	 */
@@ -354,6 +385,13 @@ int mxl862xx_api_wrap(struct mxl862xx_priv *priv, u16 cmd, void *_data,
 			val = le16_to_cpu(data[i]);
 		}
 
+		/* After RST_DATA, skip zero data words as the registers
+		 * already contain zeros, but never skip the CRC at the final
+		 * word.
+		 */
+		if (use_rst && i < max && val == 0)
+			continue;
+
 		ret = mxl862xx_reg_write(priv,
 					 MXL862XX_MMD_REG_DATA_FIRST + off,
 					 val);
-- 
2.53.0

Re: [PATCH net-next 2/2] net: dsa: mxl862xx: use RST_DATA to skip writing zero words

[Andrew Lunn]

On Sat, Mar 21, 2026 at 05:20:14AM +0000, Daniel Golle wrote:
> Issue the firmware's RST_DATA command before writing data payloads that
> contain many zero words. RST_DATA zeroes both the firmware's internal
> buffer and the MMD data registers in a single command, allowing the
> driver to skip individual MDIO writes for zero-valued words. This
> reduces bus traffic for the common case where API structs have many
> unused or default-zero fields.
> 
> The optimization is applied when at least 5 zero words are found in the
> payload, roughly the break-even point against the cost of the extra
> RST_DATA command round-trip.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew