linux-kernel - 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: <20190812081225.GC3698@kwain>
Date:   Mon, 12 Aug 2019 10:12:25 +0200
From:   Antoine Tenart <antoine.tenart@...tlin.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Antoine Tenart <antoine.tenart@...tlin.com>, 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

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