| Message ID | E1pex8Q-00Dvnr-5y@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | RFC, archived |
| 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. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
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 04:03:05PM +0200, Andy Shevchenko wrote: > 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); > > ? Why const? 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. Thanks.
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 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. Then there's phylink_create(). Same 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.
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 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.
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.
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. > 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. 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 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 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. You failed to answer this. I can only assume you don't have an answer for why you are singling out my use compared to all the 129 other uses in the kernel that follow this pattern. Given the number of them, that is no basis to NAK this patch. > > 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. I believe you are mistaken. > > 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. > > All getters return _different_ fwnode which is not const due to modification > of the _returned_ fwnode. > > Do a bit of investigation, please. Thanks. I did exactly that, and I included the research in my email. How is fwnode_is_ancestor_of() a "getter" when it returns a _boolean_? Did you read my fully researched explanation? "child" is *not* modified, it is merely passed to another function that accepts a *const* pointer. Here's teaching you to suck eggs, because that's clearly what it's going to take to make you see sense: bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child) { struct fwnode_handle *parent; if (IS_ERR_OR_NULL(ancestor)) return false; if (child == ancestor) return true; fwnode_for_each_parent_node(child, parent) { if (parent == ancestor) { fwnode_handle_put(parent); return true; } } return false; } "child" is only used by the if() "child == ancestor" and fwnode_for_each_parent_node(). fwnode_for_each_parent_node() is: #define fwnode_for_each_parent_node(fwnode, parent) \ for (parent = fwnode_get_parent(fwnode); parent; \ parent = fwnode_get_next_parent(parent)) so child is passed in as fwnode there, and gets passed to fwnode_get_parent(). fwnode_get_parent() is declared as: struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode); So "child" ends up passed into this function, which takes a const pointer. At no point is "child" assigned to in fwnode_is_ancestor_of(). It is only used in situations where it can be a const pointer. Now, if we look at "ancestor" then there are three locations it is used: if (IS_ERR_OR_NULL(ancestor)) if (child == ancestor) if (parent == ancestor) { All of these can cope with ancestor being const. I will grant you that "parent" can't be const, but then I *never* said it would be as I was only taking about the functions arguments. So, I think you need to revise your comment on this, because clearly it was incorrect. > > 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. No, I'm finding your requests to be rather inconsistent, lacking in useful value, and lacking an appreciation of how other parts of the kernel's development process works.
On Thu, Mar 23, 2023 at 05:06:49PM +0000, Russell King (Oracle) wrote: OK, you are right as always, I'm wrong. Case closed. Thank you.
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. Those five patches do not include moving fwnode_get_phy_mode(), whose location remains undecided.
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.
On Thu, Mar 23, 2023 at 08:04:45PM +0200, Andy Shevchenko wrote: > 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. Oh, and what a waste of time that was. You request that the fwnode should be declared const, but now I realise that you never looked at patch 4, where we call fwnode_remove_software_node() on this fwnode (so that the swnodes returned by ->port_get_fwnode are released). Since fwnode_remove_software_node() modifies the swnode containing the fwnode, and therefore does not take a const fwnode pointer, we also can't make _this_ fwnode pointer const - even with all those changes. So, I feel like I've been on a wild goose chase and what a needless effort it has been to concoct patches that I don't even need for this. So again, rmk was right! Sigh.
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(-)