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:   Mon, 5 Dec 2022 18:05:07 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, Oleksij Rempel <o.rempel@pengutronix.de>
Subject: Re: [PATCH v3 net-next 1/4] net/ethtool: add netlink interface for
 the PLCA RS
Message-ID: <Y44y05M+6NGod+4x@shell.armlinux.org.uk>
References: <d53ffecdde8d3950a24155228a3439f2c9b10b9b.1670259123.git.piergiorgio.beruto@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d53ffecdde8d3950a24155228a3439f2c9b10b9b.1670259123.git.piergiorgio.beruto@gmail.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Mon, Dec 05, 2022 at 06:17:37PM +0100, Piergiorgio Beruto wrote:
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index e5b6cb1a77f9..99e3497b6aa1 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -543,6 +543,40 @@ int phy_ethtool_get_stats(struct phy_device *phydev,
>  }
>  EXPORT_SYMBOL(phy_ethtool_get_stats);
>  
> +/**
> + *
> + */
> +int phy_ethtool_get_plca_cfg(struct phy_device *dev,
> +			     struct phy_plca_cfg *plca_cfg)
> +{
> +	// TODO
> +	return 0;
> +}
> +EXPORT_SYMBOL(phy_ethtool_get_plca_cfg);
> +
> +/**
> + *
> + */
> +int phy_ethtool_set_plca_cfg(struct phy_device *dev,
> +			     struct netlink_ext_ack *extack,
> +			     const struct phy_plca_cfg *plca_cfg)
> +{
> +	// TODO
> +	return 0;
> +}
> +EXPORT_SYMBOL(phy_ethtool_set_plca_cfg);
> +
> +/**
> + *
> + */
> +int phy_ethtool_get_plca_status(struct phy_device *dev,
> +				struct phy_plca_status *plca_st)
> +{
> +	// TODO
> +	return 0;
> +}
> +EXPORT_SYMBOL(phy_ethtool_get_plca_status);
> +
>  /**
>   * phy_start_cable_test - Start a cable test
>   *
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 716870a4499c..f248010c403d 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3262,6 +3262,9 @@ static const struct ethtool_phy_ops phy_ethtool_phy_ops = {
>  	.get_sset_count		= phy_ethtool_get_sset_count,
>  	.get_strings		= phy_ethtool_get_strings,
>  	.get_stats		= phy_ethtool_get_stats,
> +	.get_plca_cfg		= phy_ethtool_get_plca_cfg,
> +	.set_plca_cfg		= phy_ethtool_set_plca_cfg,
> +	.get_plca_status	= phy_ethtool_get_plca_status,
>  	.start_cable_test	= phy_start_cable_test,
>  	.start_cable_test_tdr	= phy_start_cable_test_tdr,
>  };

From what I can see, none of the above changes need to be part of
patch 1 - nothing in the rest of the patch requires these members to be
filled in, since you explicitly test to see whether they are before
calling them.

So, rather than introducing docbook stubs and stub functions that
do nothing, marked with "TODO" comments, just merge these changes
above with patch 3, where you actually populate these functions.

Also, why do they need to be exported to modules? From what I can see,
the only user of these functions is phy_device.c, which is part of
the same module as phy.c where the functions live, meaning that they
don't need to be exported.

It's also surely wrong to introduce stubs that return success but
do nothing.

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