netdev - Re: [PATCH net-next 5/5] net: phy: Add downshift get/set support in Microsemi PHYs driver

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: <20161104122746.GK13959@lunn.ch>
Date:   Fri, 4 Nov 2016 13:27:46 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     "Allan W. Nielsen" <allan.nielsen@...rosemi.com>
Cc:     netdev@...r.kernel.org, f.fainelli@...il.com,
        raju.lakkaraju@...rosemi.com, cphealy@...il.com, robh@...nel.org
Subject: Re: [PATCH net-next 5/5] net: phy: Add downshift get/set support in
 Microsemi PHYs driver

Hi Allan

> +static int vsc85xx_downshift_get(struct phy_device *phydev, u8 *count)
> +{
> +	int rc;
> +	u16 reg_val;
> +
> +	mutex_lock(&phydev->lock);
> +	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
> +	if (rc != 0)
> +		goto out_unlock;
> +
> +	reg_val = phy_read(phydev, MSCC_PHY_ACTIPHY_CNTL);
> +	reg_val &= DOWNSHIFT_CNTL_MASK;
> +	if (!(reg_val & DOWNSHIFT_EN))
> +		*count = 0;

DOWNSHIFT_DEV_DISABLE

> +	else
> +		*count = ((reg_val & ~DOWNSHIFT_EN) >> DOWNSHIFT_CNTL_POS) + 2;
>
> +	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
> +
> +out_unlock:
> +	mutex_unlock(&phydev->lock);
> +
> +	return rc;
> +}
> +
> +static int vsc85xx_downshift_set(struct phy_device *phydev, u8 count)
> +{
> +	int rc;
> +	u16 reg_val;
> +
> +	if (count == DOWNSHIFT_DEV_DEFAULT_COUNT) {
> +		/* Default downshift count 3 (i.e. Bit3:2 = 0b01) */
> +		count = ((1 << DOWNSHIFT_CNTL_POS) | DOWNSHIFT_EN);
> +	} else if (count > DOWNSHIFT_COUNT_MAX || count == 1) {
> +		phydev_err(phydev, "Invalid downshift count\n");

Maybe include a hint what is valid?

> +		return -EINVAL;

ERANGE? I don't know error codes too well, so this needs verifying.

> +	} else if (count) {
> +		/* Downshift count is either 2,3,4 or 5 */
> +		count = (((count - 2) << DOWNSHIFT_CNTL_POS) | DOWNSHIFT_EN);

Ah, now i see why + 2. But this means it never does what you ask it to
do. It would be better to round up < 2 to 2, and leave all the others
as is.

> +static int vsc85xx_get_tunable(struct phy_device *phydev,
> +			       struct ethtool_tunable *tuna, void *data)
> +{
> +	switch (tuna->id) {
> +	case ETHTOOL_PHY_DOWNSHIFT:
> +		return vsc85xx_downshift_get(phydev, (u8 *)data);
> +	default:
> +		phydev_err(phydev, "Unsupported PHY tunable id\n");
> +		return -EINVAL;

This is not really a error you should complain about. There could be
many tunables, and some your hardware cannot support. So return
-ENOSUPP and no phydev_err().

> +	}
> +}
> +
> +static int vsc85xx_set_tunable(struct phy_device *phydev,
> +			       struct ethtool_tunable *tuna,
> +			       const void *data)
> +{
> +	switch (tuna->id) {
> +	case ETHTOOL_PHY_DOWNSHIFT:
> +		return vsc85xx_downshift_set(phydev, *(u8 *)data);
> +	default:
> +		phydev_err(phydev, "Unsupported PHY tunable id\n");
> +		return -EINVAL;

Same as above.

     Andrew