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

[Daniel Golle <daniel@makrotopia.org>]

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...