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

[Jiabing Wan]

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