netdev - Re: [PATCH net-next 4/6] net: phy: Add support for inband extensions

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:   Thu, 19 May 2022 16:10:15 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc:     davem@...emloft.net, Rob Herring <robh+dt@...nel.org>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, thomas.petazzoni@...tlin.com,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        linux-arm-kernel@...ts.infradead.org,
        Richard Cochran <richardcochran@...il.com>,
        Horatiu.Vultur@...rochip.com, Allan.Nielsen@...rochip.com,
        UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next 4/6] net: phy: Add support for inband extensions

> +static int phy_set_inband_ext(struct phy_device *phydev, u32 mask, u32 ext)
> +{
> +	int ret;
> +
> +	if (!phy_interface_has_inband_ext(phydev->interface))
> +		return -EOPNOTSUPP;
> +
> +	if (!phydev->drv->inband_ext_config)
> +		return -EOPNOTSUPP;
> +
> +	ret = phydev->drv->inband_ext_config(phydev, mask, ext);
> +	if (ret)
> +		return ret;
> +
> +	phydev->inband_ext.enabled &= ~mask;
> +	phydev->inband_ext.enabled |= (mask & ext);

You appear to be missing locking in this patchset.

> +int phy_inband_ext_enable(struct phy_device *phydev, u32 ext)
> +{
> +	return phy_set_inband_ext(phydev, ext, ext);

There should be an -EOPNOTSUPP here is requested to enable an
extension which is not available.

> +}
> +EXPORT_SYMBOL(phy_inband_ext_enable);
> +
> +int phy_inband_ext_disable(struct phy_device *phydev, u32 ext)
> +{
> +	return phy_set_inband_ext(phydev, ext, 0);

And the same here.

> +}
> +EXPORT_SYMBOL(phy_inband_ext_disable);
> +
> +int phy_inband_ext_set_available(struct phy_device *phydev, u32 mask, u32 ext)
> +{
> +	if (!(mask & phydev->drv->inband_ext))
> +		return -EOPNOTSUPP;
> +
> +	phydev->inband_ext.available &= ~mask;
> +	phydev->inband_ext.available |= (mask & ext);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(phy_inband_ext_set_available);
> +
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 4a2731c78590..6b08f49bce5b 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -190,6 +190,21 @@ static inline void phy_interface_set_rgmii(unsigned long *intf)
>  	__set_bit(PHY_INTERFACE_MODE_RGMII_TXID, intf);
>  }
>  
> +/*
> + * TODO : Doc
> + */
> +enum {
> +	__PHY_INBAND_EXT_PCH = 0,
> +};
> +
> +#define PHY_INBAND_EXT_PCH	BIT(__PHY_INBAND_EXT_PCH)

the documentation is important here, since it makes it clear if these
are values directly taken from the specification, or if these are
linux specific, and the driver needs to map from linux to whatever the
spec calls them.

     Andrew