netdev - Re: [PATCH] net: phy: micrel: Remove unnecessary comparison in lan8814_handle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2ec61428-d9af-7712-b008-cf6b7e445aaa@vivo.com>
Date:   Thu, 5 May 2022 20:37:48 +0800
From:   Jiabing Wan <wanjiabing@...o.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: phy: micrel: Remove unnecessary comparison in
 lan8814_handle_interrupt

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