Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 19 May 2022 16:10:15 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Maxime Chevallier <maxime.chevallier@bootlin.com>
Cc:     davem@davemloft.net, Rob Herring <robh+dt@kernel.org>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, thomas.petazzoni@bootlin.com,
        Florian Fainelli <f.fainelli@gmail.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        linux-arm-kernel@lists.infradead.org,
        Richard Cochran <richardcochran@gmail.com>,
        Horatiu.Vultur@microchip.com, Allan.Nielsen@microchip.com,
        UNGLinuxDriver@microchip.com
Subject: Re: [PATCH net-next 4/6] net: phy: Add support for inband extensions
Message-ID: <YoZPx3s353hJcGnt@lunn.ch>
References: <20220519135647.465653-1-maxime.chevallier@bootlin.com>
 <20220519135647.465653-5-maxime.chevallier@bootlin.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220519135647.465653-5-maxime.chevallier@bootlin.com>
Precedence: bulk

> +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