On Tue, May 30, 2023 at 10:09:24PM +0100, Russell King (Oracle) wrote:
> Having thought about this, the best I can come up with is this, which
> I think gives us everything we want without needing BUILD_BUG_ONs:
>
> #define phy_read_poll_timeout(phydev, regnum, val, cond, sleep_us, \
> timeout_us, sleep_before_read) \
> ({ \
> int __ret, __val;
> __ret = read_poll_timeout(__val = phy_read, val, __val < 0 || (cond), \
> sleep_us, timeout_us, sleep_before_read, phydev, regnum); \
> if (__val < 0) \
> __ret = __val; \
> if (__ret) \
> phydev_err(phydev, "%s failed: %d\n", __func__, __ret); \
> __ret; \
> })
>
> This looks rather horrid, but what it essentially does is:
>
> (val) = op(args); \
> if (cond) \
> break; \
>
> expands to:
>
> (val) = __val = phy_read(args);
> if (__val < 0 || (cond))
> break;
>
> As phy_read() returns an int, there is no cast or loss assigning it
> to __val, since that is also an int. The conversion from int to
> something else happens at the same point it always has.
... and actually produces nicer code on 32-bit ARM:
Old (with the u16 val changed to an int val):
2f8: ebfffffe bl 0 <mdiobus_read>
2fc: e7e03150 ubfx r3, r0, #2, #1 extract bit 2 into r3
300: e1a04000 mov r4, r0 save return value
304: e2002004 and r2, r0, #4 extract bit 2 again
308: e1933fa0 orrs r3, r3, r0, lsr #31 grab sign bit
30c: 1a00000d bne 348 <genphy_loopback+0xd8>
breaks out of loop if r3 is nonzero
... rest of loop ...
...
348: e3520000 cmp r2, #0
34c: 0a00000b beq 380 <genphy_loopback+0x110>
basically tests whether bit 2 was zero, and jumps if it
was. Basically (cond) is false.
350: e3540000 cmp r4, #0
354: a3a04000 movge r4, #0
358: ba00000a blt 388 <genphy_loopback+0x118>
tests whether a phy_read returned an error and jumps
if it did. r4 is basically __ret.
...
380: e3540000 cmp r4, #0
384: a3e0406d mvnge r4, #109 ; 0x6d
if r4 (__ret) was >= 0, sets an error code (-ETIMEDOUT).
388: e1a03004 mov r3, r4
... dev_err() bit.
The new generated code is:
2f8: ebfffffe bl 0 <mdiobus_read>
2f8: R_ARM_CALL mdiobus_read
2fc: e2504000 subs r4, r0, #0 __val assignment
300: ba000014 blt 358 <genphy_loopback+0xe8>
if <0, go direct to dev_err code
304: e3140004 tst r4, #4 cond test within loop
308: 1a00000d bne 344 <genphy_loopback+0xd4>
... rest of loop ...
344: e6ff4074 uxth r4, r4 cast to 16-bit uint
348: e3140004 tst r4, #4 test
34c: 13a04000 movne r4, #0 __ret is zero if bit set
350: 1a000007 bne 374 <genphy_loopback+0x104> basically returns
354: e3e0406d mvn r4, #109 ; 0x6d
... otherwise sets __ret to -ETIMEDOUT
... dev_err() code
Is there a reason why it was written (cond) || val < 0 rather than
val < 0 || (cond) ? Note that the order of these tests makes no
difference in this situation, but I'm wondering whether it was
intentional?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
