Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Thu, Mar 23, 2023 at 05:33:17PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 23, 2023 at 03:23:12PM +0000, Russell King (Oracle) wrote:
> > On Thu, Mar 23, 2023 at 05:00:08PM +0200, Andy Shevchenko wrote:
> > > On Thu, Mar 23, 2023 at 02:49:01PM +0000, Russell King (Oracle) wrote:
> > > > On Thu, Mar 23, 2023 at 04:38:29PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Mar 23, 2023 at 02:31:04PM +0000, Russell King (Oracle) wrote:
> > > > > > On Thu, Mar 23, 2023 at 04:03:05PM +0200, Andy Shevchenko wrote:
> > > > > > > On Wed, Mar 22, 2023 at 12:00:06PM +0000, Russell King (Oracle) wrote:
> 
> ...
> 
> > > > > > > > +	struct fwnode_handle *fwnode;
> > > > > > > 
> > > > > > > > +	fwnode = of_fwnode_handle(dp->dn);
> > > > > > > 
> > > > > > > 	const struct fwnode_handle *fwnode = of_fwnode_handle(dp->dn);
> > > > > > > 
> > > > > > > ?
> > > > > > 
> > > > > > Why const?
> > > > > 
> > > > > Do you modify its content on the fly?
> > > > 
> > > > Do you want to litter code with casts to get rid of the const?
> > > > 
> > > > > For fwnode as a basic object type we want to reduce the scope of the possible
> > > > > modifications. If you don't modify and APIs you call do not require non-const
> > > > > object, use const for fwnode.
> > > > 
> > > > Let's start here. We pass this fwnode to fwnode_get_phy_mode():
> > > > 
> > > > include/linux/property.h:int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
> > > > 
> > > > Does fwnode_get_phy_mode() alter the contents of the fwnode? Probably
> > > > not, but it doesn't take a const pointer. Therefore, to declare my
> > > > fwnode as const, I'd need to cast the const-ness away before calling
> > > > this.
> > > 
> > > So, fix the fwnode_get_phy_mode(). Is it a problem?
> > 
> > No, I refuse. That's for a different patch set.
> 
> I don't disagree, but it can be done as a precursor to your RFC.

And you want that merged through net-next?

> > > > Then there's phylink_create(). Same problem.
> > > 
> > > So, fix that. Is it a problem?
> > 
> > No for the same reason.
> > 
> > > > So NAK to this const - until such time that we have a concerted effort
> > > > to making functions we call which do not modify the "fwnode" argument
> > > > constify that argument. Otherwise it's just rediculously crazy to
> > > > declare a variable const only to then litter the code with casts to get
> > > > rid of it at every call site.
> > > > 
> > > > Please do a bit of research before making suggestions. Thanks.
> > > 
> > > So, MAK to your patch. You can fix that, and you know that.
> > 
> > Sorry, I don't accept your NAK. While you have a valid point about
> > these things being const, that is not the fault of this patch series,
> > and is something that should be addressed separately.
> 
> Yes, and since it's not a big deal it can be done as a precursor work.
> 
> > The lack of const-ness that has been there for quite some time is no
> > reason to NAK a patch that has nothing to do with this.
> 
> Instead of saying politely that you didn't agree of the necessity of the asked
> changes, you shoowed your confrontational manner with a strong NAK. Let's not
> escalate it further, it won't play well with a nervous system.
> 
> > > P.S. Please, move that phy thingy away from property.h, it doesn't belong
> > > there.
> > 
> > Again, that's a subject for a separate patch.
> > 
> > I will re-post this in due course and ignore your NAK (due to your
> > lack of research, and confrontational nature.)
> 
> Don't make a drama out of it. Many maintainers are asking for a small cleanups
> before applying a feature.
> 
> Nevertheless, since I'm neither a net nor a DSA maintainer, I have only thing
> to push is to move the PHY APIs out from the property.h. The rest is up to you.

Really? In your previous message, you were NAKing the patch based on the
lack of "const"ness. So you've changed your tune to something that was
a request in a post-script (PS).

If you had done due diligence, you would have realised that its
implementation is in property.c, so presumably if you had known that
either (a) you wouldn't be making the request or (b) you would be
asking for that to be moved as well.

Now, where do you expect it to be moved to? There is nowhere convenient
in net/ nor drivers/net/ for it today. It's corresponding DT equivalent
is in net/core/of_net.c, but that is only built if CONFIG_OF is enabled
which is unsuitable for the fwnode version. I guess we could have a
net/core/fwnode.c just for this single function... then where do we put
the prototype? include/linux/fwnode_net.h (which would be a new header
just for this), and updating the five drivers for this change.

In any case, due to the way netdev works, this should *not* be part
of this patch series, because if there's a reason to revert this
series, we wouldn't want the move of fwnode_get_phy_mode() to also
get reverted - that would potentially cause chaos.

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