On Wed, Feb 22, 2023 at 04:17:38AM +0100, Marek Vasut wrote:
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index 729b36eeb2c46..7fc2155d93d6e 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -319,7 +319,7 @@ static const u16 ksz8795_regs[] = { [S_BROADCAST_CTRL] = 0x06, [S_MULTICAST_CTRL] = 0x04, [P_XMII_CTRL_0] = 0x06,
- [P_XMII_CTRL_1] = 0x56,
- [P_XMII_CTRL_1] = 0x06,
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.
You mention you're using a KSZ8794. This uses the ksz8795_regs array, and ksz8_dev_ops. You claim this is about the P_GMII_1GBIT_M bit, which is bit 6.
This bit is accessed only by ksz_get_gbit() and ksz_set_gbit().
Firstly, ksz_set_gbit() is only called from ksz_port_set_xmii_speed(), which is only called from ksz9477_phylink_mac_link_up(). This is only referenced by ksz9477_dev_ops and lan937x_dev_ops, but not ksz8_dev_ops. Therefore, ksz_set_gbit() is not called for KSZ8794.
ksz_get_gbit() is only referenced by ksz9477.c in ksz9477_get_interface(), called only by ksz9477_config_cpu_port(). This is only referenced by ksz9477_dev_ops, but not ksz8_dev_ops.
Therefore, my conclusion is that neither of the ksz_*_gbit() functions are called on KSZ8794, and thus your change has no effect on the driver's use of P_GMII_1GBIT_M - I think if you put some debugging printk()s into both ksz_*_gbit() functions, it'll prove that.
There's other places that P_XMII_CTRL_1 is accessed - ksz_set_xmii() and ksz_get_xmii(). These look at the P_MII_SEL_M, P_RGMII_ID_IG_ENABLE and P_RGMII_ID_EG_ENABLE bits - bits 0, 1, 3 and 4.
ksz_get_xmii() is only called by ksz9477_get_interface(), which we've already looked at above as not being called.
ksz_set_xmii() is only called by ksz_phylink_mac_config(), which is always called irrespective of the KSZ chip.
Now, let's look at functions that access P_XMII_CTRL_0. These are ksz_set_100_10mbit() and ksz_duplex_flowctrl(). The former accesses bit P_MII_100MBIT_M, which is bit 4. The latter looks at bits 6, bit 5, and possibly bit 3 depending on the masks being used. KSZ8795 uses ksz8795_masks, which omits bit 3, so bits 5 and 6. Note... bit 6 is also P_GMII_1GBIT_M. So if ksz_duplex_flowctrl() is ever called for the KSZ8795, then we have a situation where the P_GMII_1GBIT_M will be manipulated.
ksz_set_100_10mbit() is only called from ksz_port_set_xmii_speed(), which we've established won't be called.
ksz_duplex_flowctrl() is only called from ksz9477_phylink_mac_link_up() which we've also established won't be called.
So, as far as I can see, P_XMII_CTRL_0 won't be accessed on this device.
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?
I'm not going to give a reviewed-by for this, because... I could have made a mistake in the above analysis given the vile nature of this driver.
Thanks.