netdev - Re: [PATCH net-next v2 2/3] net: phy: add Broadcom BCM54140 support

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]

Date:   Sun, 19 Apr 2020 17:49:43 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Michael Walle <michael@...le.cc>
Cc:     linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next v2 2/3] net: phy: add Broadcom BCM54140 support

On Sun, Apr 19, 2020 at 12:12:48PM +0200, Michael Walle wrote:

Hi Michael

> +static int bcm54140_b0_workaround(struct phy_device *phydev)
> +{
> +	int spare3;
> +	int ret;

Could you add a comment about what this is working around?

> +static int bcm54140_phy_probe(struct phy_device *phydev)
> +{
> +	struct bcm54140_phy_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	phydev->priv = priv;
> +
> +	ret = bcm54140_get_base_addr_and_port(phydev);
> +	if (ret)
> +		return ret;
> +
> +	dev_info(&phydev->mdio.dev,
> +		 "probed (port %d, base PHY address %d)\n",
> +		 priv->port, priv->base_addr);

phydev_dbg() ? Do we need to see this message four times?

> +
> +	return 0;
> +}
> +
> +static int bcm54140_config_init(struct phy_device *phydev)
> +{
> +	u16 reg = 0xffff;
> +	int ret;
> +
> +	/* Apply hardware errata */
> +	ret = bcm54140_b0_workaround(phydev);
> +	if (ret)
> +		return ret;
> +
> +	/* Unmask events we are interested in. */
> +	reg &= ~(BCM54140_RDB_INT_DUPLEX |
> +		 BCM54140_RDB_INT_SPEED |
> +		 BCM54140_RDB_INT_LINK);
> +	ret = bcm_phy_write_rdb(phydev, BCM54140_RDB_IMR, reg);
> +	if (ret)
> +		return ret;
> +
> +	/* LED1=LINKSPD[1], LED2=LINKSPD[2], LED3=ACTIVITY */
> +	ret = bcm_phy_modify_rdb(phydev, BCM54140_RDB_SPARE1,
> +				 0, BCM54140_RDB_SPARE1_LSLM);
> +	if (ret)
> +		return ret;

What are the reset default for LEDs? Can the LEDs be configured via
strapping pins? There is currently no good solution for this. Whatever
you pick will be wrong for somebody else. At minimum, strapping pins,
if they exist, should not be overridden.