Messages in this thread | | | Date | Tue, 10 May 2022 14:25:10 +0200 | From | Andrew Lunn <> | Subject | Re: [PATCH net] net: phy: mscc: Add error check when __phy_read() failed |
| |
On Tue, May 10, 2022 at 11:54:56AM +0800, Wan Jiabing wrote: > Calling __phy_read() might return a negative error code. Use 'int' > to declare variables which call __phy_read() and also add error check > for them.
It would be good to add a comment here:
The numerous callers of vsc8584_macsec_phy_read() don't expect it to fail. So don't return the error code from __phy_read(), but also don't return random values if it does fail.
The commit message should try to answer any questions to reviewer has. And when i first looked at the change, i thought this is wrong, the error code is thrown away. But then i remembers our discussion. So it is good to mention that in the commit message.
> Fixes: fa164e40c53b ("net: phy: mscc: split the driver into separate files") > Signed-off-by: Wan Jiabing <wanjiabing@vivo.com> > --- > drivers/net/phy/mscc/mscc_macsec.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/phy/mscc/mscc_macsec.c b/drivers/net/phy/mscc/mscc_macsec.c > index b7b2521c73fb..8a63e32fafa0 100644 > --- a/drivers/net/phy/mscc/mscc_macsec.c > +++ b/drivers/net/phy/mscc/mscc_macsec.c > @@ -22,9 +22,9 @@ > static u32 vsc8584_macsec_phy_read(struct phy_device *phydev, > enum macsec_bank bank, u32 reg) > { > - u32 val, val_l = 0, val_h = 0; > unsigned long deadline; > - int rc; > + int rc, val, val_l, val_h; > + u32 ret = 0;
Networking code uses "reverse christmas tree", meaning these lines should be sorted, longest first, shortest last. So deadline needs to come after val_h.
Andrew
| |