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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ