| Message ID | tencent_9E44A7B2F489B91A12A11C2639C5A4B9F40A@qq.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phylink: Separating two unrelated definitions for improving code readability | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Wed, Nov 20, 2024 at 02:27:53PM +0800, Cong Yi wrote: > From: Cong Yi <yicong@kylinos.cn> > > After the support of PCS state machine, phylink and pcs two > unrelated state enum definitions are put together, which brings > some confusion to code reading. Hmm, so the definitions being prefixed with "PCS_STATE_" and the variable being called "pcs_state" isn't enough?
Hi, Russell King: Thank you for your reply! Yes, as you say, there is no problem with the definitions themselves being named. When I just read from Linux-5.4 to 6.6, I thought that PCS_STATE_ and PHYLINK_DISABLE- were associated in some way. After reading the code carefully, I found that there was no correlation。 In order to avoid similar confusion, I sent this patch.
On 11/20/24 07:27, Cong Yi wrote: > From: Cong Yi <yicong@kylinos.cn> > > After the support of PCS state machine, phylink and pcs two > unrelated state enum definitions are put together, which brings > some confusion to code reading. > > This patch defines the two separately. > > Signed-off-by: Cong Yi <yicong@kylinos.cn> ## Form letter - net-next-closed The merge window for v6.13 has begun and net-next is closed for new drivers, features, code refactoring and optimizations. We are currently accepting bug fixes only. Please repost when net-next reopens after Dec 2nd. RFC patches sent for review only are welcome at any time. See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle
On Wed, Nov 20, 2024 at 05:46:14PM +0800, Cong Yi wrote: > Hi, Russell King: > > Thank you for your reply! > Yes, as you say, there is no problem with the definitions themselves > being named. When I just read from Linux-5.4 to 6.6, I thought > that PCS_STATE_ and PHYLINK_DISABLE- were associated in some way. > After reading the code carefully, I found that there was no correlation。 > In order to avoid similar confusion, I sent this patch. For the record, I agree that tying together unrelated constants inside the same anonymous enum and resetting the counter is a confusing coding pattern, to which I don't see the benefit. Separating them and giving names to the enums also gives the opportunity for stronger typing, which was done here. I think the patch (or at least its idea) is ok.
On Thu, Nov 21, 2024 at 12:50:44PM +0200, Vladimir Oltean wrote: > On Wed, Nov 20, 2024 at 05:46:14PM +0800, Cong Yi wrote: > > Hi, Russell King: > > > > Thank you for your reply! > > Yes, as you say, there is no problem with the definitions themselves > > being named. When I just read from Linux-5.4 to 6.6, I thought > > that PCS_STATE_ and PHYLINK_DISABLE- were associated in some way. > > After reading the code carefully, I found that there was no correlation。 > > In order to avoid similar confusion, I sent this patch. > > For the record, I agree that tying together unrelated constants inside > the same anonymous enum and resetting the counter is a confusing coding > pattern, to which I don't see the benefit. Separating them and giving > names to the enums also gives the opportunity for stronger typing, which > was done here. I think the patch (or at least its idea) is ok. See include/linux/ata.h, and include/linux/libata.h. We also have many enums that either don't use the enum counter, or set the counter to a specific value. The typing argument is nonsense. This is a common misconception by C programmers. You don't get any extra typechecking with enums. If you define two enums, say fruit and colour, this produces no warning, even with -Wall -pedantic: enum fruit { APPLE, ORANGE }; enum colour { BLACK, WHITE }; enum fruit get_fruit(void); enum colour test(void) { return get_fruit(); } What one gets is more compiler specific variability in the type - some compiler architectures may use storage sufficient to store the range of values defined in the enum (e.g. it may select char vs int vs long) which makes laying out structs with no holes harder. Another thing one gets is checking in switch() statmeents that all values in the enumerated type have a "case" or "default". That's fine where we need to ensure that all values of an enum are checked, but if that's unnecessary, it becomes an unnecessary burden to remember to add a - sometimes - useless default case to each switch.
On Thu, Nov 21, 2024 at 11:21:33AM +0000, Russell King (Oracle) wrote: > On Thu, Nov 21, 2024 at 12:50:44PM +0200, Vladimir Oltean wrote: > > On Wed, Nov 20, 2024 at 05:46:14PM +0800, Cong Yi wrote: > > > Hi, Russell King: > > > > > > Thank you for your reply! > > > Yes, as you say, there is no problem with the definitions themselves > > > being named. When I just read from Linux-5.4 to 6.6, I thought > > > that PCS_STATE_ and PHYLINK_DISABLE- were associated in some way. > > > After reading the code carefully, I found that there was no correlation。 > > > In order to avoid similar confusion, I sent this patch. > > > > For the record, I agree that tying together unrelated constants inside > > the same anonymous enum and resetting the counter is a confusing coding > > pattern, to which I don't see the benefit. Separating them and giving > > names to the enums also gives the opportunity for stronger typing, which > > was done here. I think the patch (or at least its idea) is ok. > > See include/linux/ata.h, and include/linux/libata.h. > > We also have many enums that either don't use the enum counter, or set > the counter to a specific value. > > The typing argument is nonsense. This is a common misconception by C > programmers. You don't get any extra typechecking with enums. If you > define two enums, say fruit and colour, this produces no warning, > even with -Wall -pedantic: > > enum fruit { APPLE, ORANGE }; > enum colour { BLACK, WHITE }; > enum fruit get_fruit(void); > enum colour test(void) > { > return get_fruit(); > } > > What one gets is more compiler specific variability in the type - > some compiler architectures may use storage sufficient to store the > range of values defined in the enum (e.g. it may select char vs int > vs long) which makes laying out structs with no holes harder. Well, I mean... $ cat test_enum.c #include <stdio.h> enum fruit { APPLE, ORANGE }; enum colour { BLACK, WHITE }; enum fruit get_fruit(void) { return APPLE; } enum colour test(void) { return get_fruit(); } int main(void) { test(); } $ make CFLAGS="-Wall -Wextra" test_enum cc -Wall -Wextra test_enum.c -o test_enum test_enum.c: In function ‘test’: test_enum.c:13:16: warning: implicit conversion from ‘enum fruit’ to ‘enum colour’ [-Wenum-conversion] 13 | return get_fruit(); | ^~~~~~~~~~~ I don't understand what's to defend about this, really.
On Thu, Nov 21, 2024 at 01:52:30PM +0200, Vladimir Oltean wrote: > On Thu, Nov 21, 2024 at 11:21:33AM +0000, Russell King (Oracle) wrote: > > On Thu, Nov 21, 2024 at 12:50:44PM +0200, Vladimir Oltean wrote: > > > On Wed, Nov 20, 2024 at 05:46:14PM +0800, Cong Yi wrote: > > > > Hi, Russell King: > > > > > > > > Thank you for your reply! > > > > Yes, as you say, there is no problem with the definitions themselves > > > > being named. When I just read from Linux-5.4 to 6.6, I thought > > > > that PCS_STATE_ and PHYLINK_DISABLE- were associated in some way. > > > > After reading the code carefully, I found that there was no correlation。 > > > > In order to avoid similar confusion, I sent this patch. > > > > > > For the record, I agree that tying together unrelated constants inside > > > the same anonymous enum and resetting the counter is a confusing coding > > > pattern, to which I don't see the benefit. Separating them and giving > > > names to the enums also gives the opportunity for stronger typing, which > > > was done here. I think the patch (or at least its idea) is ok. > > > > See include/linux/ata.h, and include/linux/libata.h. > > > > We also have many enums that either don't use the enum counter, or set > > the counter to a specific value. > > > > The typing argument is nonsense. This is a common misconception by C > > programmers. You don't get any extra typechecking with enums. If you > > define two enums, say fruit and colour, this produces no warning, > > even with -Wall -pedantic: > > > > enum fruit { APPLE, ORANGE }; > > enum colour { BLACK, WHITE }; > > enum fruit get_fruit(void); > > enum colour test(void) > > { > > return get_fruit(); > > } > > > > What one gets is more compiler specific variability in the type - > > some compiler architectures may use storage sufficient to store the > > range of values defined in the enum (e.g. it may select char vs int > > vs long) which makes laying out structs with no holes harder. > > Well, I mean... > > $ cat test_enum.c > #include <stdio.h> > > enum fruit { APPLE, ORANGE }; > enum colour { BLACK, WHITE }; > > enum fruit get_fruit(void) > { > return APPLE; > } > > enum colour test(void) > { > return get_fruit(); > } > > int main(void) > { > test(); > } > $ make CFLAGS="-Wall -Wextra" test_enum > cc -Wall -Wextra test_enum.c -o test_enum > test_enum.c: In function ‘test’: > test_enum.c:13:16: warning: implicit conversion from ‘enum fruit’ to ‘enum colour’ [-Wenum-conversion] > 13 | return get_fruit(); > | ^~~~~~~~~~~ > > I don't understand what's to defend about this, really. It's not something I want to entertain right now. I have enough on my plate without having patches like this to deal with. Maybe next year I'll look at it, but not right now.
On Thu, Nov 21, 2024 at 12:11:02PM +0000, Russell King (Oracle) wrote: > On Thu, Nov 21, 2024 at 01:52:30PM +0200, Vladimir Oltean wrote: > > I don't understand what's to defend about this, really. > > It's not something I want to entertain right now. I have enough on my > plate without having patches like this to deal with. Maybe next year > I'll look at it, but not right now. I can definitely understand the lack of time to deal with trivial matters, but I mean, it isn't as if ./scripts/get_maintainer.pl drivers/net/phy/phylink.c lists a single person...
On Thu, Nov 21, 2024 at 02:15:48PM +0200, Vladimir Oltean wrote: > On Thu, Nov 21, 2024 at 12:11:02PM +0000, Russell King (Oracle) wrote: > > On Thu, Nov 21, 2024 at 01:52:30PM +0200, Vladimir Oltean wrote: > > > I don't understand what's to defend about this, really. > > > > It's not something I want to entertain right now. I have enough on my > > plate without having patches like this to deal with. Maybe next year > > I'll look at it, but not right now. > > I can definitely understand the lack of time to deal with trivial > matters, but I mean, it isn't as if ./scripts/get_maintainer.pl > drivers/net/phy/phylink.c lists a single person... Trivial patches have an impact beyond just reviewing the patch. They can cause conflicts, causing work that's in progress to need extra re-work. I have the problems of in-band that I've been trying to address since April. I have phylink EEE support that I've also been trying to move forward. However, with everything that has happened this year (first, a high priority work item, followed by holiday, followed by my eye operations) I've only _just_ been able to get back to looking at these issues... meanwhile I see that I'm now being asked for stuff about stacked PHYs which is also going to impact phylink. Oh, and to top it off, I've discovered that mainline is broken on my test platform (IRQ crap) which I'm currently trying to debug what has been broken. Meaning I'm not working on any phylink stuff right now because of other people's breakage. It's just been bit of crap after another after another. Give me a sodding break.
On Thu, Nov 21, 2024 at 12:26:44PM +0000, Russell King (Oracle) wrote: > On Thu, Nov 21, 2024 at 02:15:48PM +0200, Vladimir Oltean wrote: > > On Thu, Nov 21, 2024 at 12:11:02PM +0000, Russell King (Oracle) wrote: > > > On Thu, Nov 21, 2024 at 01:52:30PM +0200, Vladimir Oltean wrote: > > > > I don't understand what's to defend about this, really. > > > > > > It's not something I want to entertain right now. I have enough on my > > > plate without having patches like this to deal with. Maybe next year > > > I'll look at it, but not right now. > > > > I can definitely understand the lack of time to deal with trivial > > matters, but I mean, it isn't as if ./scripts/get_maintainer.pl > > drivers/net/phy/phylink.c lists a single person... > > Trivial patches have an impact beyond just reviewing the patch. They > can cause conflicts, causing work that's in progress to need extra > re-work. > > I have the problems of in-band that I've been trying to address since > April. I have phylink EEE support that I've also been trying to move > forward. However, with everything that has happened this year (first, > a high priority work item, followed by holiday, followed by my eye > operations) I've only _just_ been able to get back to looking at these > issues... meanwhile I see that I'm now being asked for stuff about > stacked PHYs which is also going to impact phylink. Oh, and to top it > off, I've discovered that mainline is broken on my test platform > (IRQ crap) which I'm currently trying to debug what has been broken. > Meaning I'm not working on any phylink stuff right now because of > other people's breakage. > > It's just been bit of crap after another after another. > > Give me a sodding break. I just believe that any patch submitter has the right for their proposal to be evaluated based solely on its own merits (even if time has to be stretched in order for that to happen), not based on unrelated context. At the end of the day, somebody other than you should be able to come in and say "I think this looks fine" and that should be sufficient, no? Especially since the dispute you've raised of this change's technical merit seems to be out of the way now.
On Thu, Nov 21, 2024 at 02:47:18PM +0200, Vladimir Oltean wrote: > On Thu, Nov 21, 2024 at 12:26:44PM +0000, Russell King (Oracle) wrote: > > On Thu, Nov 21, 2024 at 02:15:48PM +0200, Vladimir Oltean wrote: > > > On Thu, Nov 21, 2024 at 12:11:02PM +0000, Russell King (Oracle) wrote: > > > > On Thu, Nov 21, 2024 at 01:52:30PM +0200, Vladimir Oltean wrote: > > > > > I don't understand what's to defend about this, really. > > > > > > > > It's not something I want to entertain right now. I have enough on my > > > > plate without having patches like this to deal with. Maybe next year > > > > I'll look at it, but not right now. > > > > > > I can definitely understand the lack of time to deal with trivial > > > matters, but I mean, it isn't as if ./scripts/get_maintainer.pl > > > drivers/net/phy/phylink.c lists a single person... > > > > Trivial patches have an impact beyond just reviewing the patch. They > > can cause conflicts, causing work that's in progress to need extra > > re-work. > > > > I have the problems of in-band that I've been trying to address since > > April. I have phylink EEE support that I've also been trying to move > > forward. However, with everything that has happened this year (first, > > a high priority work item, followed by holiday, followed by my eye > > operations) I've only _just_ been able to get back to looking at these > > issues... meanwhile I see that I'm now being asked for stuff about > > stacked PHYs which is also going to impact phylink. Oh, and to top it > > off, I've discovered that mainline is broken on my test platform > > (IRQ crap) which I'm currently trying to debug what has been broken. > > Meaning I'm not working on any phylink stuff right now because of > > other people's breakage. > > > > It's just been bit of crap after another after another. > > > > Give me a sodding break. > > I just believe that any patch submitter has the right for their proposal > to be evaluated based solely on its own merits (even if time has to be > stretched in order for that to happen), not based on unrelated context. Right, and my coding preference is as I've written the code. If my coding preference, as author and maintainer of this file, were different then I would've written it differently. Am I not entitled to make my own choices for code I maintain?
On Thu, Nov 21, 2024 at 12:49:43PM +0000, Russell King (Oracle) wrote: > On Thu, Nov 21, 2024 at 02:47:18PM +0200, Vladimir Oltean wrote: > > On Thu, Nov 21, 2024 at 12:26:44PM +0000, Russell King (Oracle) wrote: > > > On Thu, Nov 21, 2024 at 02:15:48PM +0200, Vladimir Oltean wrote: > > > > On Thu, Nov 21, 2024 at 12:11:02PM +0000, Russell King (Oracle) wrote: > > > > > On Thu, Nov 21, 2024 at 01:52:30PM +0200, Vladimir Oltean wrote: > > > > > > I don't understand what's to defend about this, really. > > > > > > > > > > It's not something I want to entertain right now. I have enough on my > > > > > plate without having patches like this to deal with. Maybe next year > > > > > I'll look at it, but not right now. > > > > > > > > I can definitely understand the lack of time to deal with trivial > > > > matters, but I mean, it isn't as if ./scripts/get_maintainer.pl > > > > drivers/net/phy/phylink.c lists a single person... > > > > > > Trivial patches have an impact beyond just reviewing the patch. They > > > can cause conflicts, causing work that's in progress to need extra > > > re-work. > > > > > > I have the problems of in-band that I've been trying to address since > > > April. I have phylink EEE support that I've also been trying to move > > > forward. However, with everything that has happened this year (first, > > > a high priority work item, followed by holiday, followed by my eye > > > operations) I've only _just_ been able to get back to looking at these > > > issues... meanwhile I see that I'm now being asked for stuff about > > > stacked PHYs which is also going to impact phylink. Oh, and to top it > > > off, I've discovered that mainline is broken on my test platform > > > (IRQ crap) which I'm currently trying to debug what has been broken. > > > Meaning I'm not working on any phylink stuff right now because of > > > other people's breakage. > > > > > > It's just been bit of crap after another after another. > > > > > > Give me a sodding break. > > > > I just believe that any patch submitter has the right for their proposal > > to be evaluated based solely on its own merits (even if time has to be > > stretched in order for that to happen), not based on unrelated context. > > Right, and my coding preference is as I've written the code. If my > coding preference, as author and maintainer of this file, were > different then I would've written it differently. > > Am I not entitled to make my own choices for code I maintain? Well, yes, I do believe that's the essence of our disagreement. I would very much like phylink to not be the personal island of Russell King the individual, but open to common patterns adopted across the entire networking subsystem, and especially to other individuals. You've quoted ata headers as an example of a similar pattern, but "git blame" says that comes from pre-history, so I'm not sure it's exactly the best example of what to do today.
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 3e9957b6aa14..1c65fd29538d 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -30,11 +30,13 @@ (ADVERTISED_TP | ADVERTISED_MII | ADVERTISED_FIBRE | \ ADVERTISED_BNC | ADVERTISED_AUI | ADVERTISED_Backplane) -enum { +enum phylink_disable_state { PHYLINK_DISABLE_STOPPED, PHYLINK_DISABLE_LINK, PHYLINK_DISABLE_MAC_WOL, +}; +enum phylink_pcs_state { PCS_STATE_DOWN = 0, PCS_STATE_STARTING, PCS_STATE_STARTED, @@ -76,7 +78,7 @@ struct phylink { struct phylink_link_state phy_state; struct work_struct resolve; unsigned int pcs_neg_mode; - unsigned int pcs_state; + enum phylink_pcs_state pcs_state; bool link_failed; bool using_mac_select_pcs;