Re: [PATCH] net: phy: micrel: Remove unnecessary comparison in lan8814_handle_interrupt

[Jiabing Wan]

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