Messages in this thread | | | Date | Mon, 12 Aug 2019 10:12:25 +0200 | From | Antoine Tenart <> | Subject | Re: [PATCH net-next v2 8/9] net: phy: mscc: macsec initialization |
| |
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
| |