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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190810165317.GB30120@lunn.ch>
Date:   Sat, 10 Aug 2019 18:53:17 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Antoine Tenart <antoine.tenart@...tlin.com>
Cc:     davem@...emloft.net, sd@...asysnail.net, f.fainelli@...il.com,
        hkallweit1@...il.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com,
        alexandre.belloni@...tlin.com, allan.nielsen@...rochip.com,
        camelia.groza@....com, Simon.Edelhaus@...antia.com
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