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