Hi Andrew,
On Sat, Aug 10, 2019 at 06:53:17PM +0200, Andrew Lunn wrote:
> > 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.
>
> 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.
I agree, it's still a hack to have those read/write functions acting
differently based on an 'init' flag.
> > +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.
Right, having readable code is always better. I'll fix that.
Thanks!
Antoine
--
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
