Messages in this thread | | | Date | Sat, 10 Aug 2019 18:53:17 +0200 | From | Andrew Lunn <> | Subject | Re: [PATCH net-next v2 8/9] net: phy: mscc: macsec initialization |
| |
> The MACsec read and write functions are wrapped into two versions: one > called during the init phase, and the other one later on. This is > because the init functions in the Microsemi Ocelot PHY driver are called > while the MDIO bus lock is taken.
Hi Antoine
It is nice you have wrapped it all up, but it is still messy. Sometime in the future, we should maybe take another look at adding the concept of initialisation of a package, before the initialization of the PHYs in the package.
> +static u32 __vsc8584_macsec_phy_read(struct phy_device *phydev, > + enum macsec_bank bank, u32 reg, bool init) > +{ > + u32 val, val_l = 0, val_h = 0; > + unsigned long deadline; > + int rc; > + > + if (!init) { > + rc = phy_select_page(phydev, MSCC_PHY_PAGE_MACSEC); > + if (rc < 0) > + goto failed; > + } else { > + __phy_write_page(phydev, MSCC_PHY_PAGE_MACSEC); > + }
...
> + if (!init) { > +failed: > + phy_restore_page(phydev, rc, rc); > + } else { > + __phy_write_page(phydev, MSCC_PHY_PAGE_STANDARD); > + }
Having the failed label inside the if is correct, but i think it is potentially dangerous for future modifications to this function. I would move the label before the if. I doubt it makes any difference to the generated code, but it might prevent future bugs.
Andrew
| |