| Message ID | 20230222031738.189025-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: microchip: Fix gigabit set and get function for KSZ87xx | expand |
Hi Marek, On Wed, 2023-02-22 at 04:17 +0100, Marek Vasut wrote: > Per KSZ8794 [1] datasheet DS00002134D page 54 TABLE 4-4: PORT > REGISTERS, > it is Register 86 (0x56): Port 4 Interface Control 6 which contains > the > Is_1Gbps field. > > Currently, the driver uses PORT read function on register > P_XMII_CTRL_1 > to access the P_GMII_1GBIT_M, i.e. Is_1Gbps, bit. The problem is, the > register P_XMII_CTRL_1 address is already 0x56, which is the > converted > PORT register address instead of the offset within PORT register > space > that PORT read function expects and converts into the PORT register > address internally. The incorrectly double-converted register address > becomes 0xa6, which is what the PORT read function ultimatelly > accesses, > and which is a non-existent register on the KSZ8794/KSZ8795 . > > The correct value for P_XMII_CTRL_1 is 0x6, which gets converted into > port address 0x56, which is Register 86 (0x56): Port 4 Interface > Control 6 > per KSZ8794 datasheet, i.e. the correct register address. > > To make this worse, there are multiple other call sites which read > and > even write the P_XMII_CTRL_1 register, one of them is ksz_set_xmii(), > which is responsible for configuration of RGMII delays. These delays > are incorrectly configured and a non-existent register is written > without this change. > > Fix the P_XMII_CTRL_1 register offset to resolve these problems. > > [1] > https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ8794CNX-Data-Sheet-DS00002134.pdf > > Fixes: 46f80fa8981b ("net: dsa: microchip: add common gigabit set and > get function") > Signed-off-by: Marek Vasut <marex@denx.de> Thanks for the catch. I overlooked the ksz_write to ksz_pwrite when refactoring the code for common KSZ implementation. Acked-by: Arun Ramadoss <arun.ramadoss@microchip.com>
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.
On Wed, Feb 22, 2023 at 12:50:07PM +0000, Russell King (Oracle) wrote: > 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. However, I should add that - as a result of neither ksz_*_gbit() functions being used, I consider at least the subject line to be rather misleading! While it may be something that you spotted, I suspect the other bits that are actually written are more the issue you're fixing.
On 2/22/23 14:03, Russell King (Oracle) wrote: > On Wed, Feb 22, 2023 at 12:50:07PM +0000, Russell King (Oracle) wrote: >> 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. > > However, I should add that - as a result of neither ksz_*_gbit() > functions being used, I consider at least the subject line to be > rather misleading! While it may be something that you spotted, > I suspect the other bits that are actually written are more the > issue you're fixing. Thank you for the lengthy review, I agree the driver and the register offset calculation are hideous. However, I did spent quite a bit of time on it already and checked both P_XMII_CTRL_0 and P_XMII_CTRL_1 mappings with printks and by dumping the register values via regmap debugfs interface. Also note that KSZ8794 and KSZ8795 seem to be the same chip die, just different package (the former has fewer ports) and different chip ID.
On Wed, Feb 22, 2023 at 04:10:33PM +0100, Marek Vasut wrote: > On 2/22/23 14:03, Russell King (Oracle) wrote: > > On Wed, Feb 22, 2023 at 12:50:07PM +0000, Russell King (Oracle) wrote: > > > 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. > > > > However, I should add that - as a result of neither ksz_*_gbit() > > functions being used, I consider at least the subject line to be > > rather misleading! While it may be something that you spotted, > > I suspect the other bits that are actually written are more the > > issue you're fixing. > > Thank you for the lengthy review, I agree the driver and the register offset > calculation are hideous. > > However, I did spent quite a bit of time on it already and checked both > P_XMII_CTRL_0 and P_XMII_CTRL_1 mappings with printks and by dumping the > register values via regmap debugfs interface. > > Also note that KSZ8794 and KSZ8795 seem to be the same chip die, just > different package (the former has fewer ports) and different chip ID. It's not clear what you think of my review and whether you are going to take any action at all... So, let me try again... The fundamental question that my review raises was whether this gigabit bit is actually used, and your response remains silent on that point. As the gigabit bit is not actually used given the code structure, it is irrelevant for this commit, despite Is_Gbit being the thing that lead to the patch. Therefore, I believe that the patch description needs to be updated to state what the effective fix for this change is (which is to fix ksz_set_xmii()) rather than making it sound like it's fixing a wrong access for Is_Gbit. The reason I think this is important is that if we need to look back at the history, current description leads one to think that this change is about fixing the Is_Gbit bit - but that isn't used as the code stands. The effective change that this patch makes is to the only access the driver makes to this register in ksz_set_xmii(), and I think that needs to be explained as the primary reason for this patch. Fixing Is_Gbit seems to be merely incidental.
On 2/22/23 16:56, Russell King (Oracle) wrote: > On Wed, Feb 22, 2023 at 04:10:33PM +0100, Marek Vasut wrote: >> On 2/22/23 14:03, Russell King (Oracle) wrote: >>> On Wed, Feb 22, 2023 at 12:50:07PM +0000, Russell King (Oracle) wrote: >>>> 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. >>> >>> However, I should add that - as a result of neither ksz_*_gbit() >>> functions being used, I consider at least the subject line to be >>> rather misleading! While it may be something that you spotted, >>> I suspect the other bits that are actually written are more the >>> issue you're fixing. >> >> Thank you for the lengthy review, I agree the driver and the register offset >> calculation are hideous. >> >> However, I did spent quite a bit of time on it already and checked both >> P_XMII_CTRL_0 and P_XMII_CTRL_1 mappings with printks and by dumping the >> register values via regmap debugfs interface. >> >> Also note that KSZ8794 and KSZ8795 seem to be the same chip die, just >> different package (the former has fewer ports) and different chip ID. > > It's not clear what you think of my review and whether you are going to > take any action at all... So, let me try again... > > The fundamental question that my review raises was whether this gigabit > bit is actually used, and your response remains silent on that point. > > As the gigabit bit is not actually used given the code structure, it > is irrelevant for this commit, despite Is_Gbit being the thing that > lead to the patch. > > Therefore, I believe that the patch description needs to be updated > to state what the effective fix for this change is (which is to fix > ksz_set_xmii()) rather than making it sound like it's fixing a wrong > access for Is_Gbit. > > The reason I think this is important is that if we need to look back > at the history, current description leads one to think that this > change is about fixing the Is_Gbit bit - but that isn't used as the > code stands. The effective change that this patch makes is to the > only access the driver makes to this register in ksz_set_xmii(), > and I think that needs to be explained as the primary reason for > this patch. Fixing Is_Gbit seems to be merely incidental. On the hardware I use here, the P_XMII_CTRL_1 register ends up being populated with all bits set, 0xff. Without this change, the driver writes to non-existent register when it attempts to access P_XMII_CTRL_1 .
On Wed, Feb 22, 2023 at 05:30:43PM +0100, Marek Vasut wrote: > On 2/22/23 16:56, Russell King (Oracle) wrote: > > On Wed, Feb 22, 2023 at 04:10:33PM +0100, Marek Vasut wrote: > > > On 2/22/23 14:03, Russell King (Oracle) wrote: > > > > On Wed, Feb 22, 2023 at 12:50:07PM +0000, Russell King (Oracle) wrote: > > > > > 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. > > > > > > > > However, I should add that - as a result of neither ksz_*_gbit() > > > > functions being used, I consider at least the subject line to be > > > > rather misleading! While it may be something that you spotted, > > > > I suspect the other bits that are actually written are more the > > > > issue you're fixing. > > > > > > Thank you for the lengthy review, I agree the driver and the register offset > > > calculation are hideous. > > > > > > However, I did spent quite a bit of time on it already and checked both > > > P_XMII_CTRL_0 and P_XMII_CTRL_1 mappings with printks and by dumping the > > > register values via regmap debugfs interface. > > > > > > Also note that KSZ8794 and KSZ8795 seem to be the same chip die, just > > > different package (the former has fewer ports) and different chip ID. > > > > It's not clear what you think of my review and whether you are going to > > take any action at all... So, let me try again... > > > > The fundamental question that my review raises was whether this gigabit > > bit is actually used, and your response remains silent on that point. > > > > As the gigabit bit is not actually used given the code structure, it > > is irrelevant for this commit, despite Is_Gbit being the thing that > > lead to the patch. > > > > Therefore, I believe that the patch description needs to be updated > > to state what the effective fix for this change is (which is to fix > > ksz_set_xmii()) rather than making it sound like it's fixing a wrong > > access for Is_Gbit. > > > > The reason I think this is important is that if we need to look back > > at the history, current description leads one to think that this > > change is about fixing the Is_Gbit bit - but that isn't used as the > > code stands. The effective change that this patch makes is to the > > only access the driver makes to this register in ksz_set_xmii(), > > and I think that needs to be explained as the primary reason for > > this patch. Fixing Is_Gbit seems to be merely incidental. > > On the hardware I use here, the P_XMII_CTRL_1 register ends up being > populated with all bits set, 0xff. Without this change, the driver writes to > non-existent register when it attempts to access P_XMII_CTRL_1 . Why is this so difficult? I'm *not* disagreeing with the patch. I'm disagreeing with your commit description. Okay, at this point, it seems I sadly have no option but to NAK this patch. Hopefully someone else can pick it up and give it a more reasonable commit description that properly describes the patch. Thanks.
On 2/22/23 17:39, Russell King (Oracle) wrote: > On Wed, Feb 22, 2023 at 05:30:43PM +0100, Marek Vasut wrote: >> On 2/22/23 16:56, Russell King (Oracle) wrote: >>> On Wed, Feb 22, 2023 at 04:10:33PM +0100, Marek Vasut wrote: >>>> On 2/22/23 14:03, Russell King (Oracle) wrote: >>>>> On Wed, Feb 22, 2023 at 12:50:07PM +0000, Russell King (Oracle) wrote: >>>>>> 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. >>>>> >>>>> However, I should add that - as a result of neither ksz_*_gbit() >>>>> functions being used, I consider at least the subject line to be >>>>> rather misleading! While it may be something that you spotted, >>>>> I suspect the other bits that are actually written are more the >>>>> issue you're fixing. >>>> >>>> Thank you for the lengthy review, I agree the driver and the register offset >>>> calculation are hideous. >>>> >>>> However, I did spent quite a bit of time on it already and checked both >>>> P_XMII_CTRL_0 and P_XMII_CTRL_1 mappings with printks and by dumping the >>>> register values via regmap debugfs interface. >>>> >>>> Also note that KSZ8794 and KSZ8795 seem to be the same chip die, just >>>> different package (the former has fewer ports) and different chip ID. >>> >>> It's not clear what you think of my review and whether you are going to >>> take any action at all... So, let me try again... >>> >>> The fundamental question that my review raises was whether this gigabit >>> bit is actually used, and your response remains silent on that point. >>> >>> As the gigabit bit is not actually used given the code structure, it >>> is irrelevant for this commit, despite Is_Gbit being the thing that >>> lead to the patch. >>> >>> Therefore, I believe that the patch description needs to be updated >>> to state what the effective fix for this change is (which is to fix >>> ksz_set_xmii()) rather than making it sound like it's fixing a wrong >>> access for Is_Gbit. >>> >>> The reason I think this is important is that if we need to look back >>> at the history, current description leads one to think that this >>> change is about fixing the Is_Gbit bit - but that isn't used as the >>> code stands. The effective change that this patch makes is to the >>> only access the driver makes to this register in ksz_set_xmii(), >>> and I think that needs to be explained as the primary reason for >>> this patch. Fixing Is_Gbit seems to be merely incidental. >> >> On the hardware I use here, the P_XMII_CTRL_1 register ends up being >> populated with all bits set, 0xff. Without this change, the driver writes to >> non-existent register when it attempts to access P_XMII_CTRL_1 . > > Why is this so difficult? > > I'm *not* disagreeing with the patch. I'm disagreeing with your > commit description. Why not comment on the commit message part which you disagree with, and suggest an improved wording you do agree with ? Then I can send a V2 with that part changed, and be done with it.
On Wed, Feb 22, 2023 at 07:43:35PM +0100, Marek Vasut wrote: > On 2/22/23 17:39, Russell King (Oracle) wrote: > > On Wed, Feb 22, 2023 at 05:30:43PM +0100, Marek Vasut wrote: > > > On 2/22/23 16:56, Russell King (Oracle) wrote: > > > > On Wed, Feb 22, 2023 at 04:10:33PM +0100, Marek Vasut wrote: > > > > > On 2/22/23 14:03, Russell King (Oracle) wrote: > > > > > > On Wed, Feb 22, 2023 at 12:50:07PM +0000, Russell King (Oracle) wrote: > > > > > > > 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. > > > > > > > > > > > > However, I should add that - as a result of neither ksz_*_gbit() > > > > > > functions being used, I consider at least the subject line to be > > > > > > rather misleading! While it may be something that you spotted, > > > > > > I suspect the other bits that are actually written are more the > > > > > > issue you're fixing. > > > > > > > > > > Thank you for the lengthy review, I agree the driver and the register offset > > > > > calculation are hideous. > > > > > > > > > > However, I did spent quite a bit of time on it already and checked both > > > > > P_XMII_CTRL_0 and P_XMII_CTRL_1 mappings with printks and by dumping the > > > > > register values via regmap debugfs interface. > > > > > > > > > > Also note that KSZ8794 and KSZ8795 seem to be the same chip die, just > > > > > different package (the former has fewer ports) and different chip ID. > > > > > > > > It's not clear what you think of my review and whether you are going to > > > > take any action at all... So, let me try again... > > > > > > > > The fundamental question that my review raises was whether this gigabit > > > > bit is actually used, and your response remains silent on that point. > > > > > > > > As the gigabit bit is not actually used given the code structure, it > > > > is irrelevant for this commit, despite Is_Gbit being the thing that > > > > lead to the patch. > > > > > > > > Therefore, I believe that the patch description needs to be updated > > > > to state what the effective fix for this change is (which is to fix > > > > ksz_set_xmii()) rather than making it sound like it's fixing a wrong > > > > access for Is_Gbit. > > > > > > > > The reason I think this is important is that if we need to look back > > > > at the history, current description leads one to think that this > > > > change is about fixing the Is_Gbit bit - but that isn't used as the > > > > code stands. The effective change that this patch makes is to the > > > > only access the driver makes to this register in ksz_set_xmii(), > > > > and I think that needs to be explained as the primary reason for > > > > this patch. Fixing Is_Gbit seems to be merely incidental. > > > > > > On the hardware I use here, the P_XMII_CTRL_1 register ends up being > > > populated with all bits set, 0xff. Without this change, the driver writes to > > > non-existent register when it attempts to access P_XMII_CTRL_1 . > > > > Why is this so difficult? > > > > I'm *not* disagreeing with the patch. I'm disagreeing with your > > commit description. > > Why not comment on the commit message part which you disagree with, and > suggest an improved wording you do agree with ? Then I can send a V2 with > that part changed, and be done with it. I have made it crystal clear which bit of the commit message I don't agree with. Thanks.
Please summarize in the commit title what is the user-visible impact of the problem that is being fixed. Short and to the point. On Wed, Feb 22, 2023 at 04:17:38AM +0100, Marek Vasut wrote: > Per KSZ8794 [1] datasheet DS00002134D page 54 TABLE 4-4: PORT REGISTERS, > it is Register 86 (0x56): Port 4 Interface Control 6 which contains the > Is_1Gbps field. Good thing you mention Is_1Gbps (even though it's irrelevant to the change you're proposing, since ksz_port_set_xmii_speed() is only called by ksz9477_phylink_mac_link_up()). That is actually what I want to bring up. If you change the speed in your fixed-link nodes (CPU port and DSA master) to 100 Mbps on KSZ87xx, does it work? No, right? Because P_GMII_1GBIT_M always remains at its hardware default value, which is selected based on pin strapping. That's a bug, and should be fixed too. Good thing you brought this up, I wouldn't have mentioned it if it wasn't in the commit message. > Currently, the driver uses PORT read function on register P_XMII_CTRL_1 > to access the P_GMII_1GBIT_M, i.e. Is_1Gbps, bit. Provably false. The driver does do that, but not for KSZ87xx. Please delete red herrings from the commit message, they do not help assess users if they care about backporting a patch to a custom tree or not. > The problem is, the register P_XMII_CTRL_1 address is already 0x56, > which is the converted PORT register address instead of the offset > within PORT register space that PORT read function expects and > converts into the PORT register address internally. The incorrectly > double-converted register address becomes 0xa6, which is what the PORT > read function ultimatelly accesses, and which is a non-existent ~~~~~~~~~~~ ultimately > register on the KSZ8794/KSZ8795 . > > The correct value for P_XMII_CTRL_1 is 0x6, which gets converted into > port address 0x56, which is Register 86 (0x56): Port 4 Interface Control 6 > per KSZ8794 datasheet, i.e. the correct register address. > > To make this worse, there are multiple other call sites which read and ~~~~~~~~ multiple implies more than 1. There is no call site other than ksz_set_xmii(). Please delete false information from the commit message. > even write the P_XMII_CTRL_1 register, one of them is ksz_set_xmii(), > which is responsible for configuration of RGMII delays. These delays > are incorrectly configured and a non-existent register is written > without this change. Not only RGMII delays, but also P_MII_SEL_M (interface mode selection). The implication of writing the value at an undocumented address is that the real register 0x56 remains with the value decided by pin strapping (which may or may not be adequate for Linux runtime). This is absolutely the same class of bug as what happens with Is_1Gbps. > Fix the P_XMII_CTRL_1 register offset to resolve these problems. > > [1] https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ8794CNX-Data-Sheet-DS00002134.pdf > > Fixes: 46f80fa8981b ("net: dsa: microchip: add common gigabit set and get function") Technically, the problem was introduced by: Fixes: c476bede4b0f ("net: dsa: microchip: ksz8795: use common xmii function") because that's when ksz87xx was transitioned from the old logic (which also used to set Is_1Gbps) to the new one. And that same commit is also to blame for the Is_1Gbps bug, because the new logic from ksz8795_cpu_interface_select() should have called not only ksz_set_xmii(), but also ksz_set_gbit() for code-wise identical behavior. It didn't do that. Then with commit f3d890f5f90e ("net: dsa: microchip: add support for phylink mac config"), this incomplete configuration just got moved around. > Signed-off-by: Marek Vasut <marex@denx.de> The contents of the patch is not wrong, but the commit message that describes it misses a lot of points which make non-zero difference to someone trying to assess whether a patch fixes a problem he's seeing or not. Even worse, there is another _actual_ Is_1Gbps bug which I've presented above, which this patch does *not* fix.
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.
On 2/22/23 22:08, Vladimir Oltean wrote: > Please summarize in the commit title what is the user-visible impact of > the problem that is being fixed. Short and to the point. Can you suggest a Subject which is acceptable ? > On Wed, Feb 22, 2023 at 04:17:38AM +0100, Marek Vasut wrote: >> Per KSZ8794 [1] datasheet DS00002134D page 54 TABLE 4-4: PORT REGISTERS, >> it is Register 86 (0x56): Port 4 Interface Control 6 which contains the >> Is_1Gbps field. > > Good thing you mention Is_1Gbps (even though it's irrelevant to the > change you're proposing, since ksz_port_set_xmii_speed() is only called > by ksz9477_phylink_mac_link_up()). > > That is actually what I want to bring up. If you change the speed in > your fixed-link nodes (CPU port and DSA master) to 100 Mbps on KSZ87xx, > does it work? No, right? Because P_GMII_1GBIT_M always remains at its > hardware default value, which is selected based on pin strapping. > That's a bug, and should be fixed too. Sure, separate patch. The system I use has gigabit link to the switch. > Good thing you brought this up, I wouldn't have mentioned it if it > wasn't in the commit message. > >> Currently, the driver uses PORT read function on register P_XMII_CTRL_1 >> to access the P_GMII_1GBIT_M, i.e. Is_1Gbps, bit. > > Provably false. The driver does do that, but not for KSZ87xx. The driver uses port read function with register value 0x56 instead of 0x06 , which means the remapping happens twice, which provably breaks the driver since commit Fixes below . > Please delete red herrings from the commit message, they do not help > assess users if they care about backporting a patch to a custom tree > or not. > >> The problem is, the register P_XMII_CTRL_1 address is already 0x56, >> which is the converted PORT register address instead of the offset >> within PORT register space that PORT read function expects and >> converts into the PORT register address internally. The incorrectly >> double-converted register address becomes 0xa6, which is what the PORT >> read function ultimatelly accesses, and which is a non-existent > ~~~~~~~~~~~ > ultimately > >> register on the KSZ8794/KSZ8795 . >> >> The correct value for P_XMII_CTRL_1 is 0x6, which gets converted into >> port address 0x56, which is Register 86 (0x56): Port 4 Interface Control 6 >> per KSZ8794 datasheet, i.e. the correct register address. >> >> To make this worse, there are multiple other call sites which read and > ~~~~~~~~ > multiple implies more than 1. > > There is no call site other than ksz_set_xmii(). Please delete false > information from the commit message. $ git grep P_XMII_CTRL_1 drivers/net/dsa/microchip/ drivers/net/dsa/microchip/ksz_common.c: [P_XMII_CTRL_1] = 0x06, drivers/net/dsa/microchip/ksz_common.c: [P_XMII_CTRL_1] = 0x0301, drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8); drivers/net/dsa/microchip/ksz_common.c: ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8); drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8); drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8); drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8); drivers/net/dsa/microchip/ksz_common.c: ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8); drivers/net/dsa/microchip/ksz_common.h: P_XMII_CTRL_1, I count 6. >> even write the P_XMII_CTRL_1 register, one of them is ksz_set_xmii(), >> which is responsible for configuration of RGMII delays. These delays >> are incorrectly configured and a non-existent register is written >> without this change. > > Not only RGMII delays, but also P_MII_SEL_M (interface mode selection). > > The implication of writing the value at an undocumented address is that > the real register 0x56 remains with the value decided by pin strapping > (which may or may not be adequate for Linux runtime). This is absolutely > the same class of bug as what happens with Is_1Gbps. > >> Fix the P_XMII_CTRL_1 register offset to resolve these problems. >> >> [1] https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ8794CNX-Data-Sheet-DS00002134.pdf >> >> Fixes: 46f80fa8981b ("net: dsa: microchip: add common gigabit set and get function") > > Technically, the problem was introduced by: > > Fixes: c476bede4b0f ("net: dsa: microchip: ksz8795: use common xmii function") > > because that's when ksz87xx was transitioned from the old logic (which > also used to set Is_1Gbps) to the new one. > > And that same commit is also to blame for the Is_1Gbps bug, because the > new logic from ksz8795_cpu_interface_select() should have called not > only ksz_set_xmii(), but also ksz_set_gbit() for code-wise identical > behavior. It didn't do that. Then with commit f3d890f5f90e ("net: dsa: > microchip: add support for phylink mac config"), this incomplete > configuration just got moved around. > >> Signed-off-by: Marek Vasut <marex@denx.de> > > The contents of the patch is not wrong, but the commit message that > describes it misses a lot of points which make non-zero difference to > someone trying to assess whether a patch fixes a problem he's seeing or not. OK, to make this simple, can you write a commit message which you consider acceptable, to close this discussion ?
On Wed, Feb 22, 2023 at 11:05:10PM +0100, Marek Vasut wrote: > On 2/22/23 22:08, Vladimir Oltean wrote: > > Please summarize in the commit title what is the user-visible impact of > > the problem that is being fixed. Short and to the point. > > Can you suggest a Subject which is acceptable ? Nope. The thing is, I don't know what you're seeing, only you do. I can only review and comment if it's plausible or not. I'm sure you can come up with something. > > > Currently, the driver uses PORT read function on register P_XMII_CTRL_1 > > > to access the P_GMII_1GBIT_M, i.e. Is_1Gbps, bit. > > > > Provably false. The driver does do that, but not for KSZ87xx. > > The driver uses port read function with register value 0x56 instead of 0x06 > , which means the remapping happens twice, which provably breaks the driver > since commit Fixes below . The sentence is false in the context of ksz87xx, which is what is the implied context of this patch (see commit title written by yourself). The P_GMII_1GBIT_M field is not accessed, and that is a bug in itself. Also, the (lack of) access to the P_GMII_1GBIT_M field is not what causes the breakage that you see, but to other fields from that register. > > There is no call site other than ksz_set_xmii(). Please delete false > > information from the commit message. > > $ git grep P_XMII_CTRL_1 drivers/net/dsa/microchip/ > drivers/net/dsa/microchip/ksz_common.c: [P_XMII_CTRL_1] = 0x06, > drivers/net/dsa/microchip/ksz_common.c: [P_XMII_CTRL_1] = 0x0301, > drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8); > drivers/net/dsa/microchip/ksz_common.c: ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8); > drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8); > drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8); > drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8); > drivers/net/dsa/microchip/ksz_common.c: ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8); > drivers/net/dsa/microchip/ksz_common.h: P_XMII_CTRL_1, > > I count 6. So your response to 2 reviewers wasting their time to do a detailed analysis of the code paths that apply to the KSZ87xx model in particular, to tell you precisely why your commit message is incorrect is "git grep"? > OK, to make this simple, can you write a commit message which you consider > acceptable, to close this discussion ? Nope. The thing is, I'm sure you can, too. Maybe you need to take a break and think about this some more.
On 2/22/23 23:31, Vladimir Oltean wrote: > On Wed, Feb 22, 2023 at 11:05:10PM +0100, Marek Vasut wrote: >> On 2/22/23 22:08, Vladimir Oltean wrote: >>> Please summarize in the commit title what is the user-visible impact of >>> the problem that is being fixed. Short and to the point. >> >> Can you suggest a Subject which is acceptable ? > > Nope. The thing is, I don't know what you're seeing, only you do. I can > only review and comment if it's plausible or not. I'm sure you can come > up with something. > >>>> Currently, the driver uses PORT read function on register P_XMII_CTRL_1 >>>> to access the P_GMII_1GBIT_M, i.e. Is_1Gbps, bit. >>> >>> Provably false. The driver does do that, but not for KSZ87xx. >> >> The driver uses port read function with register value 0x56 instead of 0x06 >> , which means the remapping happens twice, which provably breaks the driver >> since commit Fixes below . > > The sentence is false in the context of ksz87xx, which is what is the > implied context of this patch (see commit title written by yourself). > The P_GMII_1GBIT_M field is not accessed, and that is a bug in itself. > Also, the (lack of) access to the P_GMII_1GBIT_M field is not what > causes the breakage that you see, but to other fields from that register. > >>> There is no call site other than ksz_set_xmii(). Please delete false >>> information from the commit message. >> >> $ git grep P_XMII_CTRL_1 drivers/net/dsa/microchip/ >> drivers/net/dsa/microchip/ksz_common.c: [P_XMII_CTRL_1] = 0x06, >> drivers/net/dsa/microchip/ksz_common.c: [P_XMII_CTRL_1] = 0x0301, >> drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8); >> drivers/net/dsa/microchip/ksz_common.c: ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8); >> drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8); >> drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8); >> drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8); >> drivers/net/dsa/microchip/ksz_common.c: ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8); >> drivers/net/dsa/microchip/ksz_common.h: P_XMII_CTRL_1, >> >> I count 6. > > So your response to 2 reviewers wasting their time to do a detailed > analysis of the code paths that apply to the KSZ87xx model in particular, > to tell you precisely why your commit message is incorrect is "git grep"? > >> OK, to make this simple, can you write a commit message which you consider >> acceptable, to close this discussion ? > > Nope. The thing is, I'm sure you can, too. Maybe you need to take a > break and think about this some more. Sorry, not like this and not with this feedback tone. If Arun wants to send V2 to fix the actual bug, fine by me.
On 2/22/23 14:58, Marek Vasut wrote: > On 2/22/23 23:31, Vladimir Oltean wrote: >> On Wed, Feb 22, 2023 at 11:05:10PM +0100, Marek Vasut wrote: >>> On 2/22/23 22:08, Vladimir Oltean wrote: >>>> Please summarize in the commit title what is the user-visible impact of >>>> the problem that is being fixed. Short and to the point. >>> >>> Can you suggest a Subject which is acceptable ? >> >> Nope. The thing is, I don't know what you're seeing, only you do. I can >> only review and comment if it's plausible or not. I'm sure you can come >> up with something. >> >>>>> Currently, the driver uses PORT read function on register >>>>> P_XMII_CTRL_1 >>>>> to access the P_GMII_1GBIT_M, i.e. Is_1Gbps, bit. >>>> >>>> Provably false. The driver does do that, but not for KSZ87xx. >>> >>> The driver uses port read function with register value 0x56 instead >>> of 0x06 >>> , which means the remapping happens twice, which provably breaks the >>> driver >>> since commit Fixes below . >> >> The sentence is false in the context of ksz87xx, which is what is the >> implied context of this patch (see commit title written by yourself). >> The P_GMII_1GBIT_M field is not accessed, and that is a bug in itself. >> Also, the (lack of) access to the P_GMII_1GBIT_M field is not what >> causes the breakage that you see, but to other fields from that register. >> >>>> There is no call site other than ksz_set_xmii(). Please delete false >>>> information from the commit message. >>> >>> $ git grep P_XMII_CTRL_1 drivers/net/dsa/microchip/ >>> drivers/net/dsa/microchip/ksz_common.c: [P_XMII_CTRL_1] = 0x06, >>> drivers/net/dsa/microchip/ksz_common.c: [P_XMII_CTRL_1] = 0x0301, >>> drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, >>> regs[P_XMII_CTRL_1], &data8); >>> drivers/net/dsa/microchip/ksz_common.c: ksz_pwrite8(dev, port, >>> regs[P_XMII_CTRL_1], data8); >>> drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, >>> regs[P_XMII_CTRL_1], &data8); >>> drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, >>> regs[P_XMII_CTRL_1], &data8); >>> drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, >>> regs[P_XMII_CTRL_1], &data8); >>> drivers/net/dsa/microchip/ksz_common.c: ksz_pwrite8(dev, port, >>> regs[P_XMII_CTRL_1], data8); >>> drivers/net/dsa/microchip/ksz_common.h: P_XMII_CTRL_1, >>> >>> I count 6. >> >> So your response to 2 reviewers wasting their time to do a detailed >> analysis of the code paths that apply to the KSZ87xx model in particular, >> to tell you precisely why your commit message is incorrect is "git grep"? >> >>> OK, to make this simple, can you write a commit message which you >>> consider >>> acceptable, to close this discussion ? >> >> Nope. The thing is, I'm sure you can, too. Maybe you need to take a >> break and think about this some more. > > Sorry, not like this and not with this feedback tone. > > If Arun wants to send V2 to fix the actual bug, fine by me. Seems like we are getting a masterclass in how not communicate, be perceived to be rude from contributors, and also lacking the persistence to be expected from a contributor, just what we need. Very encouraging.
On Wed, Feb 22, 2023 at 11:58:23PM +0100, Marek Vasut wrote: > On 2/22/23 23:31, Vladimir Oltean wrote: > > On Wed, Feb 22, 2023 at 11:05:10PM +0100, Marek Vasut wrote: > > > On 2/22/23 22:08, Vladimir Oltean wrote: > > > > Please summarize in the commit title what is the user-visible impact of > > > > the problem that is being fixed. Short and to the point. > > > > > > Can you suggest a Subject which is acceptable ? > > > > Nope. The thing is, I don't know what you're seeing, only you do. I can > > only review and comment if it's plausible or not. I'm sure you can come > > up with something. > > > > > > > Currently, the driver uses PORT read function on register P_XMII_CTRL_1 > > > > > to access the P_GMII_1GBIT_M, i.e. Is_1Gbps, bit. > > > > > > > > Provably false. The driver does do that, but not for KSZ87xx. > > > > > > The driver uses port read function with register value 0x56 instead of 0x06 > > > , which means the remapping happens twice, which provably breaks the driver > > > since commit Fixes below . > > > > The sentence is false in the context of ksz87xx, which is what is the > > implied context of this patch (see commit title written by yourself). > > The P_GMII_1GBIT_M field is not accessed, and that is a bug in itself. > > Also, the (lack of) access to the P_GMII_1GBIT_M field is not what > > causes the breakage that you see, but to other fields from that register. > > > > > > There is no call site other than ksz_set_xmii(). Please delete false > > > > information from the commit message. > > > > > > $ git grep P_XMII_CTRL_1 drivers/net/dsa/microchip/ > > > drivers/net/dsa/microchip/ksz_common.c: [P_XMII_CTRL_1] = 0x06, > > > drivers/net/dsa/microchip/ksz_common.c: [P_XMII_CTRL_1] = 0x0301, > > > drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8); > > > drivers/net/dsa/microchip/ksz_common.c: ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8); > > > drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8); > > > drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8); > > > drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8); > > > drivers/net/dsa/microchip/ksz_common.c: ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8); > > > drivers/net/dsa/microchip/ksz_common.h: P_XMII_CTRL_1, > > > > > > I count 6. > > > > So your response to 2 reviewers wasting their time to do a detailed > > analysis of the code paths that apply to the KSZ87xx model in particular, > > to tell you precisely why your commit message is incorrect is "git grep"? > > > > > OK, to make this simple, can you write a commit message which you consider > > > acceptable, to close this discussion ? > > > > Nope. The thing is, I'm sure you can, too. Maybe you need to take a > > break and think about this some more. > > Sorry, not like this and not with this feedback tone. I gave you reasonable technical feedback. I spent a lot of time working that out. However, your responses were consistently difficult, but I kept my cool. I completely agree with Vladimir's sentiments. Had you come straight out and asked for help writing the commit message, then I might have been more receptive, but to "play dumb" as you seemed to do, only to then eventually ask for me to write the commit message for you... sorry, but no, you don't get that free cookie anymore and that ship sailed when you decided to be difficult.
On Wed, Feb 22, 2023 at 11:58:23PM +0100, Marek Vasut wrote: > On 2/22/23 23:31, Vladimir Oltean wrote: > > On Wed, Feb 22, 2023 at 11:05:10PM +0100, Marek Vasut wrote: > > > OK, to make this simple, can you write a commit message which you consider > > > acceptable, to close this discussion ? > > > > Nope. The thing is, I'm sure you can, too. Maybe you need to take a > > break and think about this some more. > > Sorry, not like this and not with this feedback tone. > > If Arun wants to send V2 to fix the actual bug, fine by me. I don't see what is wrong with this feedback tone, but if you could tell me, I will make an effort to think about it and see what I can do to change it. On the other hand, I will not write the commit message for you and that's not negotiable, because from the replies to me and to Russell, I get the suspicion that there's some sort of hidden intention for this to be used against me somehow, and I really have nothing else to base my judgement on, than your hint that there is a bug there, and the code. But the driver might behave in much more subtle ways which I may be completely missing, and I may think that I'm fixing something when I'm not. I have no way to know that except by booting a board, which I do not have (but you do). For example, I don't even know which KSZ8 boards rely on pin strapping and which ones do really need the configuration to be done by Linux. I'm completely blind, and the refusal to tell you what to write word by word is a self defense mechanism. It's good that you gave Arun permission to take your patch, test it on a KSZ8 (which seems like something he wasn't doing that often during refactoring), give it an accurate description of the problem, and resubmit it while keeping your authorship. Arun is an active contributor and reviewer on the KSZ driver and there's a good chance he might actually even do it. This is good not because you gave up (IMO for an unjustified reason, but maybe that's just my perspective), but because there still is a path forward for the actual bug to get fixed.
On 2/23/23 00:21, Vladimir Oltean wrote: [...] , and I really have nothing else to base my judgement > on, than your hint that there is a bug there, and the code. But the > driver might behave in much more subtle ways which I may be completely > missing, and I may think that I'm fixing something when I'm not. I have > no way to know that except by booting a board, which I do not have (but > you do). The old code, removed in: c476bede4b0f0 ("net: dsa: microchip: ksz8795: use common xmii function") used ksz_write8() (this part is important): ksz_write8(dev, REG_PORT_5_CTRL_6, data8); where: drivers/net/dsa/microchip/ksz8795_reg.h:#define REG_PORT_5_CTRL_6 0x56 The new code, where the relevant part is added in (see Fixes tag) 46f80fa8981bc ("net: dsa: microchip: add common gigabit set and get function") +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -257,6 +257,7 @@ static const u16 ksz8795_regs[] = { + [P_XMII_CTRL_1] = 0x56, uses ksz_pwrite8() (with p in the function name, p means PORT): ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8); which per drivers/net/dsa/microchip/ksz_common.h translates to ksz_write8(dev, dev->dev_ops->get_port_addr(port, offset), data); and that dev->dev_ops->get_port_addr(port, offset) remapping function is per drivers/net/dsa/microchip/ksz8795.c really call to the following macro: PORT_CTRL_ADDR(port, offset) which in turn from drivers/net/dsa/microchip/ksz8795_reg.h becomes #define PORT_CTRL_ADDR(port, addr) ((addr) + REG_PORT_1_CTRL_0 + (port) * (REG_PORT_2_CTRL_0 - REG_PORT_1_CTRL_0)) That means: ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8) writes register 0xa6 instead of register 0x56, because it calls the PORT_CTRL_ADDR(port, 0x56)=0xa6, but in reality it should call PORT_CTRL_ADDR(port, 0x06)=0x56, i.e. the remapping should happen ONCE, the value 0x56 is already remapped . All the call-sites which do ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8) or ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8) are affected by this, all six, that means the ksz_[gs]et_xmii() and the ksz_[gs]et_gbit(). ... If all that should be changed in the commit message is "to access the P_GMII_1GBIT_M, i.e. Is_1Gbps, bit" to something from the "ksz_set_xmii()" function instead, then just say so. [...]
Hi Marek, On Thu, Feb 23, 2023 at 12:55:08AM +0100, Marek Vasut wrote: > The old code, removed in: > c476bede4b0f0 ("net: dsa: microchip: ksz8795: use common xmii function") > used ksz_write8() (this part is important): > ksz_write8(dev, REG_PORT_5_CTRL_6, data8); > where: > drivers/net/dsa/microchip/ksz8795_reg.h:#define REG_PORT_5_CTRL_6 > 0x56 > > The new code, where the relevant part is added in (see Fixes tag) > 46f80fa8981bc ("net: dsa: microchip: add common gigabit set and get > function") > +++ b/drivers/net/dsa/microchip/ksz_common.c > @@ -257,6 +257,7 @@ static const u16 ksz8795_regs[] = { > + [P_XMII_CTRL_1] = 0x56, > uses ksz_pwrite8() (with p in the function name, p means PORT): > ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8); > which per drivers/net/dsa/microchip/ksz_common.h translates to > ksz_write8(dev, dev->dev_ops->get_port_addr(port, offset), data); > and that dev->dev_ops->get_port_addr(port, offset) remapping function is per > drivers/net/dsa/microchip/ksz8795.c really call to the following macro: > PORT_CTRL_ADDR(port, offset) > which in turn from drivers/net/dsa/microchip/ksz8795_reg.h becomes > #define PORT_CTRL_ADDR(port, addr) ((addr) + REG_PORT_1_CTRL_0 + (port) * (REG_PORT_2_CTRL_0 - REG_PORT_1_CTRL_0)) > > That means: > ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8) > writes register 0xa6 instead of register 0x56, because it calls the > PORT_CTRL_ADDR(port, 0x56)=0xa6, but in reality it should call > PORT_CTRL_ADDR(port, 0x06)=0x56, i.e. the remapping should happen ONCE, the > value 0x56 is already remapped . I never had any objection to this part. > All the call-sites which do > ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8) > or > ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8) > are affected by this, all six, that means the ksz_[gs]et_xmii() and the > ksz_[gs]et_gbit(). I'm going to say this with a very calm tone, please tell me where it's wrong and we can go from there. Not for the ksz_switch_chips[] elements which point to ksz8795_regs (which had the incorrect value you're fixing), it isn't. You're making an argument for code which never executes (5 out of 6 call paths), and basing your commit message off of it. Your commit message reads as if you didn't even notice that ksz_set_gbit() isn't called for ksz87xx, and that this is a bug in itself. Moreover, the problem you're seeing (I may speculate what that is, but I don't know) is surely not due to ksz_set_gbit() being called on the wrong register, because it's not called at all *for that hardware*. That gigabit bug was pointed out to you by reviewers, and you refuse to acknowledge this and keep bringing forth some other stuff which was never debated by anyone. The lack of acknowledgement plus your continuation to randomly keep singing another tune in another key completely is irritating to me on a very personal (non-technical) level. To respond to you, I am exercising some mental muscles which I wish I wouldn't have needed to, here, in this context. The same muscles I use when I need to identify manipulation on tass.com. [ in case the message above is misinterpreted: I did not say that you willingly manipulate. I said that your lack of acknowledgement to what is being said to you is giving me the same kind of frustration ] This is my feedback to the tone in your replies. I want you to give your feedback to my tone now too. You disregarded that. > ... > > If all that should be changed in the commit message is "to access the > P_GMII_1GBIT_M, i.e. Is_1Gbps, bit" to something from the "ksz_set_xmii()" > function instead, then just say so. > > [...] No, this is not all that I want. The gigabit bug changes things in ways in which I'm curious how you're going to try to defend, with this attitude of responding to anything except to what was asked. Your commit says it fixes gigabit on KSZ87xx, but the gigabit bug which *was pointed out to you by others* is still there. Your patch fixes something else, but *it says* it fixes a gigabit bug. What I want is for you to describe exactly what it fixes, or if you just don't know, say you noticed the bug during code review and you don't know what is its real life impact (considering pin strapping). I don't want a patch to be merged which says it fixes something it doesn't fix, while leaving the exact thing it says it fixes unfixed. I also don't want to entertain this game of "if it's just this small thing, why didn't you say so". I would be setting myself up for an endless time waste if I were to micromanage your commit message writing. I am looking forward to a productive conversation with you, but if your next reply is going to have the same strategy of avoiding my key message and focusing on some other random thing, then I'm very sorry, but I'll just have to focus my attention somewhere else.
On 2/23/23 01:22, Vladimir Oltean wrote: > Hi Marek, > > On Thu, Feb 23, 2023 at 12:55:08AM +0100, Marek Vasut wrote: >> The old code, removed in: >> c476bede4b0f0 ("net: dsa: microchip: ksz8795: use common xmii function") >> used ksz_write8() (this part is important): >> ksz_write8(dev, REG_PORT_5_CTRL_6, data8); >> where: >> drivers/net/dsa/microchip/ksz8795_reg.h:#define REG_PORT_5_CTRL_6 >> 0x56 >> >> The new code, where the relevant part is added in (see Fixes tag) >> 46f80fa8981bc ("net: dsa: microchip: add common gigabit set and get >> function") >> +++ b/drivers/net/dsa/microchip/ksz_common.c >> @@ -257,6 +257,7 @@ static const u16 ksz8795_regs[] = { >> + [P_XMII_CTRL_1] = 0x56, >> uses ksz_pwrite8() (with p in the function name, p means PORT): >> ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8); >> which per drivers/net/dsa/microchip/ksz_common.h translates to >> ksz_write8(dev, dev->dev_ops->get_port_addr(port, offset), data); >> and that dev->dev_ops->get_port_addr(port, offset) remapping function is per >> drivers/net/dsa/microchip/ksz8795.c really call to the following macro: >> PORT_CTRL_ADDR(port, offset) >> which in turn from drivers/net/dsa/microchip/ksz8795_reg.h becomes >> #define PORT_CTRL_ADDR(port, addr) ((addr) + REG_PORT_1_CTRL_0 + (port) * (REG_PORT_2_CTRL_0 - REG_PORT_1_CTRL_0)) >> >> That means: >> ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8) >> writes register 0xa6 instead of register 0x56, because it calls the >> PORT_CTRL_ADDR(port, 0x56)=0xa6, but in reality it should call >> PORT_CTRL_ADDR(port, 0x06)=0x56, i.e. the remapping should happen ONCE, the >> value 0x56 is already remapped . > > I never had any objection to this part. > >> All the call-sites which do >> ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8) >> or >> ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8) >> are affected by this, all six, that means the ksz_[gs]et_xmii() and the >> ksz_[gs]et_gbit(). > > I'm going to say this with a very calm tone, please tell me where it's wrong > and we can go from there. > > Not for the ksz_switch_chips[] elements which point to ksz8795_regs (which > had the incorrect value you're fixing), it isn't. You're making an argument > for code which never executes (5 out of 6 call paths), and basing your commit > message off of it. Your commit message reads as if you didn't even notice > that ksz_set_gbit() isn't called for ksz87xx, and that this is a bug in itself. > Moreover, the problem you're seeing (I may speculate what that is, but > I don't know) is surely not due to ksz_set_gbit() being called on the > wrong register, because it's not called at all *for that hardware*. > > That gigabit bug was pointed out to you by reviewers, and you refuse to > acknowledge this and keep bringing forth some other stuff which was never > debated by anyone. The lack of acknowledgement plus your continuation to > randomly keep singing another tune in another key completely is irritating > to me on a very personal (non-technical) level. To respond to you, I am > exercising some mental muscles which I wish I wouldn't have needed to, > here, in this context. The same muscles I use when I need to identify > manipulation on tass.com. > > [ in case the message above is misinterpreted: I did not say that you > willingly manipulate. I said that your lack of acknowledgement to what > is being said to you is giving me the same kind of frustration ] > > This is my feedback to the tone in your replies. I want you to give your > feedback to my tone now too. You disregarded that. > >> ... >> >> If all that should be changed in the commit message is "to access the >> P_GMII_1GBIT_M, i.e. Is_1Gbps, bit" to something from the "ksz_set_xmii()" >> function instead, then just say so. >> >> [...] > > No, this is not all that I want. > > The gigabit bug changes things in ways in which I'm curious how you're > going to try to defend, with this attitude of responding to anything > except to what was asked. Your commit says it fixes gigabit on KSZ87xx No, it does not say it fixes gigabit on KSZ87xx, the commit message says it fixes gigabit accessor functions, but what it really fixes (and what is the bulk of the commit message) is the incorrectly double-remapped register 0x56 used in those gigabit accessor functions and which is also used in the ksz_[gs]et_xmii function. > but the gigabit bug which *was pointed out to you by others* is still > there. Your patch fixes something else, but *it says* it fixes a gigabit > bug. What I want is for you to describe exactly what it fixes, or if you > just don't know, say you noticed the bug during code review and you > don't know what is its real life impact (considering pin strapping). I believe I wrote what the problem is in my previous email, the incorrectly double-remapped XMII_1 register . The register wasn't updated in my case in ksz_set_xmii() with RGMII delays. Why I picked the is_1Gbit bit for the commit message, probably was tired after lengthy debugging session of this problem. > I don't want a patch to be merged which says it fixes something it doesn't > fix, while leaving the exact thing it says it fixes unfixed. > > I also don't want to entertain this game of "if it's just this small > thing, why didn't you say so". I would be setting myself up for an > endless time waste if I were to micromanage your commit message writing. > > I am looking forward to a productive conversation with you, but if your > next reply is going to have the same strategy of avoiding my key message > and focusing on some other random thing, then I'm very sorry, but I'll > just have to focus my attention somewhere else. [...]
On Thu, Feb 23, 2023 at 06:17:28AM +0100, Marek Vasut wrote: > No, it does not say it fixes gigabit on KSZ87xx, the commit message says it > fixes gigabit accessor functions, but what it really fixes (and what is the > bulk of the commit message) is the incorrectly double-remapped register 0x56 > used in those gigabit accessor functions and which is also used in the > ksz_[gs]et_xmii function. >· > > but the gigabit bug which *was pointed out to you by others* is still > > there. Your patch fixes something else, but *it says* it fixes a gigabit > > bug. What I want is for you to describe exactly what it fixes, or if you > > just don't know, say you noticed the bug during code review and you > > don't know what is its real life impact (considering pin strapping). >· > I believe I wrote what the problem is in my previous email, the incorrectly > double-remapped XMII_1 register . The register wasn't updated in my case in > ksz_set_xmii() with RGMII delays. >· > Why I picked the is_1Gbit bit for the commit message, probably was tired > after lengthy debugging session of this problem. All that is requested from you is to stop being overly defensive about the commit message that you wrote, and to write another one, which is more representative of the real life impact that your change has, in light of the facts that were pointed out during review. Nobody is out to get you. Open source projects are a collaborative effort, and your part is to accept that your work can be improved, when given clear and specific indications from reviewers who have looked at the same problem as you from an angle independent from yours. If you're so sure that you cannot improve your work by yourself and ask for this passive-aggressive hand holding from reviewers, I don't know why you don't just set up your git trees where there's no review, and that's that. I don't know why I'm wasting my time to point this out to you, you have more experience with open source projects than I do, so you should know better, yet your attitude is completely unproductive. To me, this topic is closed. You have accused me of having an improper tone towards you, but you have not explained what was wrong with it, even when pressed to. I am wasting my time.
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, }; static const u32 ksz8795_masks[] = {
Per KSZ8794 [1] datasheet DS00002134D page 54 TABLE 4-4: PORT REGISTERS, it is Register 86 (0x56): Port 4 Interface Control 6 which contains the Is_1Gbps field. Currently, the driver uses PORT read function on register P_XMII_CTRL_1 to access the P_GMII_1GBIT_M, i.e. Is_1Gbps, bit. The problem is, the register P_XMII_CTRL_1 address is already 0x56, which is the converted PORT register address instead of the offset within PORT register space that PORT read function expects and converts into the PORT register address internally. The incorrectly double-converted register address becomes 0xa6, which is what the PORT read function ultimatelly accesses, and which is a non-existent register on the KSZ8794/KSZ8795 . The correct value for P_XMII_CTRL_1 is 0x6, which gets converted into port address 0x56, which is Register 86 (0x56): Port 4 Interface Control 6 per KSZ8794 datasheet, i.e. the correct register address. To make this worse, there are multiple other call sites which read and even write the P_XMII_CTRL_1 register, one of them is ksz_set_xmii(), which is responsible for configuration of RGMII delays. These delays are incorrectly configured and a non-existent register is written without this change. Fix the P_XMII_CTRL_1 register offset to resolve these problems. [1] https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ8794CNX-Data-Sheet-DS00002134.pdf Fixes: 46f80fa8981b ("net: dsa: microchip: add common gigabit set and get function") Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: "David S. Miller" <davem@davemloft.net> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Arun Ramadoss <arun.ramadoss@microchip.com> Cc: David S. Miller <davem@davemloft.net> Cc: Eric Dumazet <edumazet@google.com> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Paolo Abeni <pabeni@redhat.com> Cc: Russell King <linux@armlinux.org.uk> Cc: UNGLinuxDriver@microchip.com Cc: Vladimir Oltean <olteanv@gmail.com> Cc: Woojung Huh <woojung.huh@microchip.com> Cc: stable@vger.kernel.org Cc: netdev@vger.kernel.org --- drivers/net/dsa/microchip/ksz_common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)