netdev - Re: [RFC net-next] net: phy: introduce phy_reg

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:   Fri, 31 Mar 2023 13:46:07 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     "Radu Pirea (OSS)" <radu-nicolae.pirea@....nxp.com>
Cc:     andrew@...n.ch, hkallweit1@...il.com, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC net-next] net: phy: introduce phy_reg_field interface

On Fri, Mar 31, 2023 at 03:32:59PM +0300, Radu Pirea (OSS) wrote:
> Some PHYs can be heavily modified between revisions, and the addresses of
> the registers are changed and the register fields are moved from one
> register to another.
> 
> To integrate more PHYs in the same driver with the same register fields,
> but these register fields were located in different registers at
> different offsets, I introduced the phy_reg_fied structure.
> 
> phy_reg_fied structure abstracts the register fields differences.

Oh no, not more perliferation of different accessors...

> +int phy_read_reg_field(struct phy_device *phydev,
> +		       const struct phy_reg_field *reg_field)
> +{
> +	u16 mask;
> +	int ret;
> +
> +	if (reg_field->size == 0) {
> +		phydev_warn(phydev, "Trying to read a reg field of size 0.");
> +		return -EINVAL;
> +	}
> +
> +	phy_lock_mdio_bus(phydev);
> +	if (reg_field->mmd)
> +		ret = __phy_read_mmd(phydev, reg_field->devad,
> +				     reg_field->reg);
> +	else
> +		ret = __phy_read(phydev, reg_field->reg);
> +	phy_unlock_mdio_bus(phydev);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	mask = reg_field->size == 1 ? BIT(reg_field->offset) :
> +		GENMASK(reg_field->offset + reg_field->size - 1, reg_field->offset);
> +	ret &= mask;
> +	ret >>= reg_field->offset;
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_read_reg_field);

I guess next we'll eventually see that we need __phy_read_reg_field
which doesn't take the lock, so that several accesses can be done
together. E.g. to access some form of paging mechanism.

> +/**
> + * phy_write_reg_field - Convenience function for writing a register field
> + * on a given PHY.
> + * @phydev: the phy_device struct
> + * @reg_field: the phy_reg_field structure to be written
> + * @val: value to write to @reg_field
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +int phy_write_reg_field(struct phy_device *phydev,
> +			const struct phy_reg_field *reg_field, u16 val)
> +{
> +	u16 mask;
> +	u16 set;
> +	int ret;
> +
> +	if (reg_field->size == 0) {
> +		phydev_warn(phydev, "Trying to write a reg field of size 0.");
> +		return -EINVAL;
> +	}
> +
> +	mask = reg_field->size == 1 ? BIT(reg_field->offset) :
> +		GENMASK(reg_field->offset + reg_field->size - 1, reg_field->offset);
> +	set = val << reg_field->offset;
> +
> +	phy_lock_mdio_bus(phydev);
> +	if (reg_field->mmd)
> +		ret = __phy_modify_mmd_changed(phydev, reg_field->devad,
> +					       reg_field->reg, mask, set);
> +	else
> +		ret = __phy_modify_changed(phydev, reg_field->reg,
> +					   mask, set);
> +	phy_unlock_mdio_bus(phydev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_write_reg_field);

More or less the same for this too.

In order to properly review this, we need the patch which has the use
case for these new accessors.

Thanks.

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