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

[Andrew Lunn <andrew@lunn.ch>]

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