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 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:
> > > > 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.
> 
> > > 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.
> 
> 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.

To illustrate how rediculous this is:

$ git grep 'struct fwnode_handle \*.*='

gives 134 instances. Of those, only five are const, which means 129
aren't. So I question - why are you singling mine out for what appears
to be special treatment.


Let's look at other parts of the fwnode API.

void __iomem *fwnode_iomap(struct fwnode_handle *fwnode, int index);

Does that modify the fwnode it was passed? It calls:

        void __iomem *(*iomap)(struct fwnode_handle *fwnode, int index);

in struct fwnode_operations, so that would need to be made const as well.
The only implementation of that which I can find is of_fwnode_iomap()
which uses to_of_node() on that, which casts away the const-ness. So
this would be a candidate to making const.


bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child);

I'd be surprised if that modifies either of those fwnodes. It seems
to use fwnode_for_each_parent_node() from the child, which passes
"child" to fwnode_get_parent(), which itself is const. Therefore, it
seems there's no reason not to make "child" const. "ancestor" can
also be made const since it's only being used for pointer-compares.


unsigned int fwnode_graph_get_endpoint_count(struct fwnode_handle *fwnode,
                                             unsigned long flags);

Similar story with this, although it uses
fwnode_graph_for_each_endpoint(), which seems to mean that "fwnode"
can also be const.


My point is that there are several things in the fwnode API that
should be made const but that aren't, but which should likely be
fixed before requiring const-ness of those fwnode_handle
declarations in people's code.

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