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

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

[Wan Jiabing]

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(-)

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;
-- 
2.36.0

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

[Andrew Lunn]

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

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

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

[Andrew Lunn]

> 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

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

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

[Andrew Lunn]

> 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