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.
