| Message ID | 20220505030217.1651422-1-wanjiabing@vivo.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: micrel: Remove unnecessary comparison in lan8814_handle_interrupt | expand |
On Thu, May 05, 2022 at 11:02:17AM +0800, Wan Jiabing wrote: > Fix following coccicheck warning: > ./drivers/net/phy/micrel.c:2679:6-20: WARNING: Unsigned expression compared with zero: tsu_irq_status > 0 There are at least two different possibilities here: As you say, the comparison is pointless, in which case, it can be removed. The code author really did have something in mind here, the comparison is correct, but there is another bug. I would generally assume the second, and try to first find the other bug. If that bug really exists, removing the comparisons just adds one bug on top of another. So, check the return type of lanphy_read_page_reg(). It is int. If you dig down, you get to __phy_read(), which calls __mdiobus_read(), all of which return int. All these functions return a negative error code, or a positive register value. So the real problem here is, tsu_irq_status is defined as u16, when in fact it should be an int. As a result, a negative error code is going to get cast positive, and then used as the value of the interrupt register. The code author wanted to avoid this, so added a comparison. In an interrupt handler you cannot actually return an error code, so the safe thing to do is ignore it. Please consider coccicheck just a hint, there is something wrong somewhere around here. You then need to really investigate and figure out what the real issue is, which might be exactly what coccicheck says, but more likely it is something else. NACK Andrew
Hi, Andrew Thanks a lot for your priceless advice! On 2022/5/5 20:13, Andrew Lunn wrote: > On Thu, May 05, 2022 at 11:02:17AM +0800, Wan Jiabing wrote: >> Fix following coccicheck warning: >> ./drivers/net/phy/micrel.c:2679:6-20: WARNING: Unsigned expression compared with zero: tsu_irq_status > 0 > There are at least two different possibilities here: > > As you say, the comparison is pointless, in which case, it can be > removed. > > The code author really did have something in mind here, the comparison > is correct, but there is another bug. > > I would generally assume the second, and try to first find the other > bug. If that bug really exists, removing the comparisons just adds one > bug on top of another. > > So, check the return type of lanphy_read_page_reg(). It is int. If you > dig down, you get to __phy_read(), which calls __mdiobus_read(), all > of which return int. All these functions return a negative error code, > or a positive register value. Yes, I actually check the lanphy_read_page_reg and I notice 'data' is declared as a 'u32' variable. So I think the comparison is meaningless. But the return type is int. 1960 static int lanphy_read_page_reg(struct phy_device *phydev, int page, u32 addr) 1961 { 1962 u32 data; 1963 1964 phy_lock_mdio_bus(phydev); 1965 __phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page); 1966 __phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr); 1967 __phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, 1968 (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC)); 1969 data = __phy_read(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA); 1970 phy_unlock_mdio_bus(phydev); 1971 1972 return data; 1973 } > > So the real problem here is, tsu_irq_status is defined as u16, when in > fact it should be an int. Should the 'data' in lanphy_read_page_reg be declared by 'int'? > > As a result, a negative error code is going to get cast positive, and > then used as the value of the interrupt register. The code author > wanted to avoid this, so added a comparison. In an interrupt handler > you cannot actually return an error code, so the safe thing to do is > ignore it. > > Please consider coccicheck just a hint, there is something wrong > somewhere around here. You then need to really investigate and figure > out what the real issue is, which might be exactly what coccicheck > says, but more likely it is something else. > > NACK > > Andrew Finally, I also find other variable, for example, 'u16 addr' in lan8814_probe. I think they all should be declared by 'int'. Thanks, Wan Jiabing
> Yes, I actually check the lanphy_read_page_reg and I notice 'data' is > declared > as a 'u32' variable. So I think the comparison is meaningless. But the > return type is int. > > 1960 static int lanphy_read_page_reg(struct phy_device *phydev, int page, > u32 addr) > 1961 { > 1962 u32 data; > 1963 > 1964 phy_lock_mdio_bus(phydev); > 1965 __phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page); > 1966 __phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr); > 1967 __phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, > 1968 (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC)); > 1969 data = __phy_read(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA); > 1970 phy_unlock_mdio_bus(phydev); > 1971 > 1972 return data; > 1973 } > > > > So the real problem here is, tsu_irq_status is defined as u16, when in > > fact it should be an int. > > Should the 'data' in lanphy_read_page_reg be declared by 'int'? Yes. Another one of those learning over time. If you find a bug, look around and you will probably find the same bug in other places nearby. This is actually a pretty common issue we have with Ethernet PHY drivers, the sign bit getting thrown away. Developers look at the datasheet and see 16 bit registers, and so use u16, and forget about the error code. Maybe somebody can write a coccicheck script looking for calls to and of the phy_read() variants and the result value is assigned to an unsigned int? > Finally, I also find other variable, for example, 'u16 addr' in > lan8814_probe. > I think they all should be declared by 'int'. addr should never be used as a return type, so can never carry an error code. Also, PHYs only have 32 registers, so address is never greater than 0x1f. So this is O.K. Andrew
On 2022/5/5 20:47, Andrew Lunn wrote: >> Yes, I actually check the lanphy_read_page_reg and I notice 'data' is >> declared >> as a 'u32' variable. So I think the comparison is meaningless. But the >> return type is int. >> >> 1960 static int lanphy_read_page_reg(struct phy_device *phydev, int page, >> u32 addr) >> 1961 { >> 1962 u32 data; >> 1963 >> 1964 phy_lock_mdio_bus(phydev); >> 1965 __phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page); >> 1966 __phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr); >> 1967 __phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, >> 1968 (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC)); >> 1969 data = __phy_read(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA); >> 1970 phy_unlock_mdio_bus(phydev); >> 1971 >> 1972 return data; >> 1973 } >>> So the real problem here is, tsu_irq_status is defined as u16, when in >>> fact it should be an int. >> Should the 'data' in lanphy_read_page_reg be declared by 'int'? > Yes. > > Another one of those learning over time. If you find a bug, look > around and you will probably find the same bug in other places nearby. > > This is actually a pretty common issue we have with Ethernet PHY > drivers, the sign bit getting thrown away. Developers look at the > datasheet and see 16 bit registers, and so use u16, and forget about > the error code. Maybe somebody can write a coccicheck script looking > for calls to and of the phy_read() variants and the result value is > assigned to an unsigned int? I write the coccicheck and find these reports: For directly call __phy_read(): ./drivers/net/phy/micrel.c:1969:59-60: WARNING: __phy_read() assigned to an unsigned int 'data' ./drivers/net/phy/mscc/mscc_macsec.c:49:50-51: WARNING: __phy_read() assigned to an unsigned int 'val' ./drivers/net/phy/mscc/mscc_macsec.c:52:51-52: WARNING: __phy_read() assigned to an unsigned int 'val_l' ./drivers/net/phy/mscc/mscc_macsec.c:53:51-52: WARNING: __phy_read() assigned to an unsigned int 'val_h' ./drivers/net/phy/mscc/mscc_macsec.c:89:50-51: WARNING: __phy_read() assigned to an unsigned int 'val' ./drivers/net/phy/mscc/mscc_main.c:1511:50-51: WARNING: __phy_read() assigned to an unsigned int 'addr' ./drivers/net/phy/mscc/mscc_main.c:1514:47-48: WARNING: __phy_read() assigned to an unsigned int 'val' ./drivers/net/phy/mscc/mscc_main.c:366:54-55: WARNING: __phy_read() assigned to an unsigned int 'reg_val' ./drivers/net/phy/mscc/mscc_main.c:370:55-56: WARNING: __phy_read() assigned to an unsigned int 'pwd [ 0 ]' ./drivers/net/phy/mscc/mscc_main.c:371:53-54: WARNING: __phy_read() assigned to an unsigned int 'pwd [ 1 ]' ./drivers/net/phy/mscc/mscc_main.c:372:55-56: WARNING: __phy_read() assigned to an unsigned int 'pwd [ 2 ]' ./drivers/net/phy/mscc/mscc_main.c:317:54-55: WARNING: __phy_read() assigned to an unsigned int 'reg_val' Should all of them be added a check for error code? >> Finally, I also find other variable, for example, 'u16 addr' in >> lan8814_probe. >> I think they all should be declared by 'int'. > addr should never be used as a return type, so can never carry an > error code. Also, PHYs only have 32 registers, so address is never > greater than 0x1f. So this is O.K. Oh, yes. I miss the ' & 0x1F'. Thanks, Wan Jiabing
> I write the coccicheck and find these reports: Nice! > For directly call __phy_read(): > > ./drivers/net/phy/micrel.c:1969:59-60: WARNING: __phy_read() assigned to an > unsigned int 'data' This one we know about. > ./drivers/net/phy/mscc/mscc_macsec.c:49:50-51: WARNING: __phy_read() > assigned to an unsigned int 'val' > ./drivers/net/phy/mscc/mscc_macsec.c:52:51-52: WARNING: __phy_read() > assigned to an unsigned int 'val_l' > ./drivers/net/phy/mscc/mscc_macsec.c:53:51-52: WARNING: __phy_read() > assigned to an unsigned int 'val_h' > ./drivers/net/phy/mscc/mscc_macsec.c:89:50-51: WARNING: __phy_read() > assigned to an unsigned int 'val' These are all in the same function. Looking at the first check, it has been decided that if something goes wrong, return 0. Not the best solution, but better than returning something random. So the do/while loop can goto failed: val_l and val_h are a bit more messy, but a check would be nice, return 0 on error. Actually returning an error code is not going to be easy. The rest of the code assumes vsc8584_macsec_phy_read() never fails :-( > ./drivers/net/phy/mscc/mscc_main.c:1511:50-51: WARNING: __phy_read() > assigned to an unsigned int 'addr' > ./drivers/net/phy/mscc/mscc_main.c:1514:47-48: WARNING: __phy_read() > assigned to an unsigned int 'val' More code which assumes a read can never fail. vsc8514_probe() calls this, so it should be reasonable easy to catch the error, return it, and make the probe fail. > ./drivers/net/phy/mscc/mscc_main.c:366:54-55: WARNING: __phy_read() assigned > to an unsigned int 'reg_val' > ./drivers/net/phy/mscc/mscc_main.c:370:55-56: WARNING: __phy_read() assigned > to an unsigned int 'pwd [ 0 ]' > ./drivers/net/phy/mscc/mscc_main.c:371:53-54: WARNING: __phy_read() assigned > to an unsigned int 'pwd [ 1 ]' > ./drivers/net/phy/mscc/mscc_main.c:372:55-56: WARNING: __phy_read() assigned > to an unsigned int 'pwd [ 2 ]' > ./drivers/net/phy/mscc/mscc_main.c:317:54-55: WARNING: __phy_read() assigned > to an unsigned int 'reg_val' Another void function which assumes it cannot fail. Error checks would be nice, don't return random data in the wol structure. Thanks Andrew
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 685a0ab5453c..6820882be59b 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -2676,11 +2676,10 @@ static irqreturn_t lan8814_handle_interrupt(struct phy_device *phydev) tsu_irq_status = lanphy_read_page_reg(phydev, 4, LAN8814_INTR_STS_REG); - if (tsu_irq_status > 0 && - (tsu_irq_status & (LAN8814_INTR_STS_REG_1588_TSU0_ | - LAN8814_INTR_STS_REG_1588_TSU1_ | - LAN8814_INTR_STS_REG_1588_TSU2_ | - LAN8814_INTR_STS_REG_1588_TSU3_))) + if (tsu_irq_status & (LAN8814_INTR_STS_REG_1588_TSU0_ | + LAN8814_INTR_STS_REG_1588_TSU1_ | + LAN8814_INTR_STS_REG_1588_TSU2_ | + LAN8814_INTR_STS_REG_1588_TSU3_)) lan8814_handle_ptp_interrupt(phydev); else break;
Fix following coccicheck warning: ./drivers/net/phy/micrel.c:2679:6-20: WARNING: Unsigned expression compared with zero: tsu_irq_status > 0 Remove unnecessary comparison to make code better. Signed-off-by: Wan Jiabing <wanjiabing@vivo.com> --- drivers/net/phy/micrel.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)