Re: [PATCH net-next v2 8/9] net: phy: mscc: macsec initialization

[Andrew Lunn]

> 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