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

[Andrew Lunn <andrew@lunn.ch>]

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