On Wed, Feb 22, 2023 at 12:50:07PM +0000, Russell King (Oracle) wrote:
Looking at this driver, I have to say that it looks utterly vile from the point of view of being sure that it is correct, and I think this patch illustrates why.
...
Now, what about other KSZ devices - I've analysed this for the KSZ8795, but what about any of the others which use this register table? It looks to me like those that use ksz8795_regs[] all use ksz8_dev_ops and the same masks and bitvals, so they should be the same.
That is a hell of a lot of work to prove that setting both P_XMII_CTRL_0 and P_XMII_CTRL_1 to point at the same register is in fact safe. Given the number of registers, the masks, and bitval arrays, doing this to prove every combination and then analysing the code is utterly impractical - and thus why I label this driver as "vile". Is there really no better option to these register arrays, bitval arrays and mask arrays - something that makes it easier to review and prove correctness?
Only my 2 cents. What is utterly vile is the decision of hardware design to break software compatibility in such a deliberate and gratuitous way across switch generations. A driver can only do so much when fed with such hardware as input.
The ksz driver could use struct reg_field from regmap to mitigate that to a certain extent (like the ocelot driver does), but certain quirks will still remain present in the ksz driver. For example, the "bitval" array. The value "1" written to the P_GMII_1GBIT reg_field indicates gigabit for ksz8795, but !gigabit on ksz9477. I am not aware of any abstraction to mask that away in common code other than the bitvals.
Even with struct reg_field, it would still not address the fundamental problem which is simply that the register fields responsible for a certain function have hopped so much from generation to generation, that getting all offsets and bits right for each generation is a challenge in itself.