| Message ID | E1pex8Q-00Dvnr-5y@rmk-PC.armlinux.org.uk |
|---|---|
| State | New |
| Headers | show |
| Series | Another attempt at moving mv88e6xxx forward | expand |
On Wed, Mar 22, 2023 at 12:00:06PM +0000, Russell King (Oracle) wrote: > In preparation for supporting the use of software nodes to setup > phylink, switch DSA to use fwnode_get_phy_mode() to retrieve the > phy interface mode, rather than using of_get_phy_mode() which is > DT specific. ... > + struct fwnode_handle *fwnode; > + fwnode = of_fwnode_handle(dp->dn); const struct fwnode_handle *fwnode = of_fwnode_handle(dp->dn); ?
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? 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. ... > Why do you want it on one line? The code as written conforms to > netdev coding standards which as you well know are different from > the rest of the kernel. This is up to you. I don't care.
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? > Then there's phylink_create(). Same problem. So, fix that. Is it a problem? > 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. P.S. Please, move that phy thingy away from property.h, it doesn't belong there.
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. > > 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. > 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.) Thanks.
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. > > > 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.
On Thu, Mar 23, 2023 at 06:34:32PM +0200, Andy Shevchenko wrote: > On Thu, Mar 23, 2023 at 04:18:15PM +0000, Russell King (Oracle) 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: > > > > > > 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: > > It's not. But does it make difference? > > > $ 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. > > Correct. > > > 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 does. Now your time to be surprised. Oops, I put it into a wrong place. The above does not touch them, but... > > It seems > > to use fwnode_for_each_parent_node() from the child, which passes > > "child" to fwnode_get_parent(), ...this one touches. > > 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. > > All getters return _different_ fwnode which is not const due to modification > of the _returned_ fwnode. > > Do a bit of investigation, please. Thanks. > > > 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. > > Correct. > > > 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. > > OK. > > I started doing something about this as you may easily check with `git log`. > Now, instead of playing a good citizen of the community you are trying to > diminish the others' asks. > > I think the further continuation of this discussion doesn't make much sense. > But thank you for your opinion.
On Thu, Mar 23, 2023 at 05:53:46PM +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: > > > 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? > > > > > Then there's phylink_create(). Same problem. > > > > So, fix that. Is it a problem? > > To do both of these creates a five patch series, because there are so > many things that need to be constified: > > fwnode_get_phy_mode() is the trivial one. > > sfp_bus_find_fwnode(), and the sfp-bus internal fwnode uses. > > fwnode_get_phy_node(). > > phylink_create(), phylink_parse_fixedlink(), phylink_parse_mode(), > phylink_fwnode_phy_connect(). > > Hopefully nothing breaks as a result of changing all those - but that > can hardly be "tacked" on to the start of my series as a trivial > change - and clearly such a change should _not_ be part of this > series. Thank you for doing that! > Those five patches do not include moving fwnode_get_phy_mode(), whose > location remains undecided. No problem, we like iterative work.
diff --git a/net/dsa/port.c b/net/dsa/port.c index 67ad1adec2a2..07f9cb374a5d 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -1678,13 +1678,9 @@ static const struct phylink_mac_ops dsa_port_phylink_mac_ops = { int dsa_port_phylink_create(struct dsa_port *dp) { struct dsa_switch *ds = dp->ds; - phy_interface_t mode; + struct fwnode_handle *fwnode; struct phylink *pl; - int err; - - err = of_get_phy_mode(dp->dn, &mode); - if (err) - mode = PHY_INTERFACE_MODE_NA; + int mode; /* Presence of phylink_mac_link_state or phylink_mac_an_restart is * an indicator of a legacy phylink driver. @@ -1696,8 +1692,14 @@ int dsa_port_phylink_create(struct dsa_port *dp) if (ds->ops->phylink_get_caps) ds->ops->phylink_get_caps(ds, dp->index, &dp->pl_config); - pl = phylink_create(&dp->pl_config, of_fwnode_handle(dp->dn), - mode, &dsa_port_phylink_mac_ops); + fwnode = of_fwnode_handle(dp->dn); + + mode = fwnode_get_phy_mode(fwnode); + if (mode < 0) + mode = PHY_INTERFACE_MODE_NA; + + pl = phylink_create(&dp->pl_config, fwnode, mode, + &dsa_port_phylink_mac_ops); if (IS_ERR(pl)) { pr_err("error creating PHYLINK: %ld\n", PTR_ERR(pl)); return PTR_ERR(pl);
In preparation for supporting the use of software nodes to setup phylink, switch DSA to use fwnode_get_phy_mode() to retrieve the phy interface mode, rather than using of_get_phy_mode() which is DT specific. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- net/dsa/port.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)