[PATCH] net: phylink: Separating two unrelated definitions for improving code readability

[PATCH] net: phylink: Separating two unrelated definitions for improving code readability

[Cong Yi]

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>
---
 drivers/net/phy/phylink.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

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;
-- 
2.25.1

Re: [PATCH] net: phylink: Separating two unrelated definitions for improving code readability

[Russell King (Oracle)]

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?

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

Re: [PATCH] net: phylink: Separating two unrelated definitions for improving code readability

[Cong Yi]

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.

Re: [PATCH] net: phylink: Separating two unrelated definitions for improving code readability

[Vladimir Oltean]

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.

Re: [PATCH] net: phylink: Separating two unrelated definitions for improving code readability

[Russell King (Oracle)]

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.

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

Re: [PATCH] net: phylink: Separating two unrelated definitions for improving code readability

[Vladimir Oltean]

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.

Re: [PATCH] net: phylink: Separating two unrelated definitions for improving code readability

[Russell King (Oracle)]

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.

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

Re: [PATCH] net: phylink: Separating two unrelated definitions for improving code readability

[Vladimir Oltean]

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

Re: [PATCH] net: phylink: Separating two unrelated definitions for improving code readability

[Russell King (Oracle)]

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.

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

Re: [PATCH] net: phylink: Separating two unrelated definitions for improving code readability

[Vladimir Oltean]

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.

Re: [PATCH] net: phylink: Separating two unrelated definitions for improving code readability

[Russell King (Oracle)]

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?

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

Re: [PATCH] net: phylink: Separating two unrelated definitions for improving code readability

[Vladimir Oltean]

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.

Re: [PATCH] net: phylink: Separating two unrelated definitions for improving code readability

[Paolo Abeni]

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