Re: [PATCH v3 net-next 1/4] net/ethtool: add netlink interface for the PLCA RS

[Piergiorgio Beruto]

On Mon, Dec 05, 2022 at 06:05:07PM +0000, Russell King (Oracle) wrote:
> 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.
My apologies, in my understanding of this process (which is new to me)
the idea of dividing the patches into smaller parts is to facilitate the
review. It was not clear to me that the single patches had to be
self-consistent also from a formal perspective. I was assuming that a
patchset can be accepted or rejected as a whole. Is this the case, or
can you accept only a subset of patches in a set?

In short, I did this because I -thought- it would help the reader
understanding what the intention of the change would be. If this is not
the case, fair enough, I'll not do this in the future.

> 
> 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.
Clear. Do you want me to regenerate the patches into a v4 or do you
think we can move forward with v3 anyway?

> 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.
I did this because similar functions in the same file are all exported
to modules (e.g. phy_config_aneg, phy_speed_down, phy_start_cable_test).
Therefore, I assumed the intention was to let modules (maybe out-of-tree
modules?) make use of these functions. If you think we should not do
this, would kindly explain why for example the phy_start_cable_test is
exported? I'm really asking because I'm trying to learn the rationales
behind the architectural choices that I see here.

> 
> It's also surely wrong to introduce stubs that return success but
> do nothing.
No doubt it would be wrong if the patch can be integrated regardless of
the other patches in the same set.

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