On Mon, May 29, 2023 at 09:58:02PM -0700, Jakub Kicinski wrote:
> On Fri, 26 May 2023 14:45:54 +0300 Dan Carpenter wrote:
> > The "val" variable is used to store error codes from phy_read() so
> > it needs to be signed for the error handling to work as expected.
> >
> > Fixes: 014068dcb5b1 ("net: phy: genphy_loopback: add link speed configuration")
> > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>
> Is it going to be obvious to PHY-savvy folks that the val passed to
> phy_read_poll_timeout() must be an int? Is it a very common pattern?
> My outsider intuition is that since regs are 16b, u16 is reasonable,
> and more people may make the same mistake.
It is common to get this wrong in general with PHY drivers. Dan
regularly posts fixes like this soon after a PHY driver patch it
merged. I really wish we could somehow get the compiler to warn when
the result from phy_read() is stored into a unsigned type. It would
save Dan a lot of work.
> Therefore we should try to fix phy_read_poll_timeout() instead to
> use a local variable like it does for __ret.
The problem with that is val is supposed to be available to the
caller. I don't know if it is every actually used, but if it is, using
an internal signed variable and then throwing away the sign bit on
return is going to result in similar bugs.
> Weaker version would be to add a compile time check to ensure val
> is signed (assert(typeof(val)~0ULL < 0) or such?).
I think the BUILD BUG is a better solution, since it catches all
problems. And at developer compile time, rather than at Dan runs his
static checker time.
Andrew
