linux-kernel - Re: [PATCH] net: phy: broadcom: add support for BCM5221 phy

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: <ZNa0qlICwsUq6H2C@shell.armlinux.org.uk>
Date:   Fri, 11 Aug 2023 23:22:34 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Giulio Benetti <giulio.benetti@...ettiengineering.com>
Cc:     Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Florian Fainelli <florian.fainelli@...adcom.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jim Reinhart <jimr@...vox.com>,
        James Autry <jautry@...vox.com>,
        Matthew Maron <matthewm@...vox.com>
Subject: Re: [PATCH] net: phy: broadcom: add support for BCM5221 phy

On Fri, Aug 11, 2023 at 11:53:22PM +0200, Giulio Benetti wrote:
> +	reg = phy_read(phydev, MII_BRCM_FET_INTREG);
> +	if (reg < 0)
> +		return reg;
> +
> +	/* Unmask events we are interested in and mask interrupts globally. */
> +	reg = MII_BRCM_FET_IR_ENABLE |
> +	      MII_BRCM_FET_IR_MASK;
> +
> +	err = phy_write(phydev, MII_BRCM_FET_INTREG, reg);
> +	if (err < 0)
> +		return err;

Please explain why you read MII_BRCM_FET_INTREG, then discard its value
and write a replacement value.

> +
> +	/* Enable auto MDIX */
> +	err = phy_clear_bits(phydev, BCM5221_AEGSR, BCM5221_AEGSR_MDIX_DIS);
> +	if (err < 0)
> +		return err;
> +
> +	/* Enable shadow register access */
> +	brcmtest = phy_read(phydev, MII_BRCM_FET_BRCMTEST);
> +	if (brcmtest < 0)
> +		return brcmtest;
> +
> +	reg = brcmtest | MII_BRCM_FET_BT_SRE;
> +
> +	err = phy_write(phydev, MII_BRCM_FET_BRCMTEST, reg);
> +	if (err < 0)
> +		return err;

I think you should consider locking the MDIO bus while the device is
switched to the shadow register set, so that other accesses don't happen
that may interfere with this.

> +static int bcm5221_suspend(struct phy_device *phydev)
> +{
> +	int reg, err, err2, brcmtest;
> +
> +	/* Enable shadow register access */
> +	brcmtest = phy_read(phydev, MII_BRCM_FET_BRCMTEST);
> +	if (brcmtest < 0)
> +		return brcmtest;
> +
> +	reg = brcmtest | MII_BRCM_FET_BT_SRE;
> +
> +	err = phy_write(phydev, MII_BRCM_FET_BRCMTEST, reg);
> +	if (err < 0)
> +		return err;
> +
> +	/* Force Low Power Mode with clock enabled */
> +	err = phy_set_bits(phydev, MII_BRCM_FET_SHDW_AUXMODE4,
> +			   BCM5221_SHDW_AM4_EN_CLK_LPM |
> +			   BCM5221_SHDW_AM4_FORCE_LPM);
> +
> +	/* Disable shadow register access */
> +	err2 = phy_write(phydev, MII_BRCM_FET_BRCMTEST, brcmtest);
> +	if (!err)
> +		err = err2;

Same here.

> +
> +	return err;
> +}
> +
> +static int bcm5221_resume(struct phy_device *phydev)
> +{
> +	int reg, err, err2, brcmtest;
> +
> +	/* Enable shadow register access */
> +	brcmtest = phy_read(phydev, MII_BRCM_FET_BRCMTEST);
> +	if (brcmtest < 0)
> +		return brcmtest;
> +
> +	reg = brcmtest | MII_BRCM_FET_BT_SRE;
> +
> +	err = phy_write(phydev, MII_BRCM_FET_BRCMTEST, reg);
> +	if (err < 0)
> +		return err;
> +
> +	/* Exit Low Power Mode with clock enabled */
> +	err = phy_clear_bits(phydev, MII_BRCM_FET_SHDW_AUXMODE4,
> +			     BCM5221_SHDW_AM4_FORCE_LPM);
> +
> +	/* Disable shadow register access */
> +	err2 = phy_write(phydev, MII_BRCM_FET_BRCMTEST, brcmtest);
> +	if (!err)
> +		err = err2;

And, of course, same here.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!