netdev - Re: [PATCH net-next v2 3/4] net: phy: micrel: Replace hardcoded pages with defines

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aIRz_EvHUWSNAUVH@pengutronix.de>
Date: Sat, 26 Jul 2025 08:21:48 +0200
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Horatiu Vultur <horatiu.vultur@...rochip.com>
Cc: "Russell King (Oracle)" <linux@...linux.org.uk>, andrew@...n.ch,
	hkallweit1@...il.com, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, richardcochran@...il.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 3/4] net: phy: micrel: Replace hardcoded
 pages with defines

On Fri, Jul 25, 2025 at 08:48:39AM +0200, Horatiu Vultur wrote:
> The 07/24/2025 21:45, Russell King (Oracle) wrote:
> > 
> > On Thu, Jul 24, 2025 at 10:08:25PM +0200, Horatiu Vultur wrote:
> > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> > > index b04c471c11a4a..d20f028106b7d 100644
> > > --- a/drivers/net/phy/micrel.c
> > > +++ b/drivers/net/phy/micrel.c
> > > @@ -2788,6 +2788,13 @@ static int ksz886x_cable_test_get_status(struct phy_device *phydev,
> > >       return ret;
> e >  }
> > >
> > > +#define LAN_EXT_PAGE_0                                       0
> > > +#define LAN_EXT_PAGE_1                                       1
> > > +#define LAN_EXT_PAGE_2                                       2
> > > +#define LAN_EXT_PAGE_4                                       4
> > > +#define LAN_EXT_PAGE_5                                       5
> > > +#define LAN_EXT_PAGE_31                                      31
> 
> Hi Russell,
> 
> > 
> > I don't see the point of this change. This is almost as bad as:
> > 
> > #define ZERO 0
> > #define ONE 1
> > #define TWO 2
> > #define THREE 3
> > ...
> > #define ONE_HUNDRED_AND_FIFTY_FIVE 155
> > etc
> > 
> > It doesn't give us any new information, and just adds extra clutter,
> > making the code less readable.
> > 
> > The point of using register definitions is to describe the purpose
> > of the number, giving the number a meaning, not to just hide the
> > number because we don't want to see such things in C code.
> > 
> > I'm sorry if you were asked to do this in v1, but I think if you
> > were asked to do it, it would've been assuming that the definitions
> > could be more meaningful.
> 
> You are right, I have been ask to change this in version 1:
> https://lkml.org/lkml/2025/7/23/672
> 
> I have mentioned it that the extended pages don't have any meaningfull
> names also in the register description document. But Oleksij says he
> will be fine with xxxx_EXT_PAGE_0, so maybe I have missunderstood Oleksij

Hi,

I requested these defines because it's much easier to search for a specific
define than for a raw number - especially when debugging or comparing with
datasheets. Even if the names are generic, it helps track down usage when
documentation becomes available or evolves.

To improve the situation, I reviewed the LAN8814 documentation and observed how
the existing driver and patches (for LAN8842) use these extended pages.
Based on that, I suggest following names:

Documented Extended Pages:

These are described in the LAN8814 documentation:

/**
 * LAN8814_PAGE_COMMON_REGS - Selects Extended Page 4.
 *
 * This page contains device-common registers that affect the entire chip.
 * It includes controls for chip-level resets, strap status, GPIO,
 * QSGMII, the shared 1588 PTP block, and the PVT monitor.
 */
#define LAN8814_PAGE_COMMON_REGS 4

/**
 * LAN8814_PAGE_PORT_REGS - Selects Extended Page 5.
 *
 * This page contains port-specific registers that must be accessed
 * on a per-port basis. It includes controls for port LEDs, QSGMII PCS,
 * rate adaptation FIFOs, and the per-port 1588 TSU block.
 */
#define LAN8814_PAGE_PORT_REGS 5

Undocumented Pages (based on driver and patch analysis):

These pages are not officially documented, but their use is visible in the
driver and LAN8842 patch:

/**
 * LAN8814_PAGE_AFE_PMA - Selects Extended Page 1.
 *
 * This page appears to control the Analog Front-End (AFE) and Physical
 * Medium Attachment (PMA) layers. It is used to access registers like
 * LAN8814_PD_CONTROLS and LAN8814_LINK_QUALITY.
 */
#define LAN8814_PAGE_AFE_PMA 1

/**
 * LAN8814_PAGE_PCS_DIGITAL - Selects Extended Page 2.
 *
 * This page seems dedicated to the Physical Coding Sublayer (PCS) and other
 * digital logic. It is used for MDI-X alignment (LAN8814_ALIGN_SWAP) and EEE
 * state (LAN8814_EEE_STATE) in the LAN8814, and is repurposed for statistics
 * and self-test counters in the LAN8842.
 */
#define LAN8814_PAGE_PCS_DIGITAL 2

/**
 * LAN8814_PAGE_SYSTEM_CTRL - Selects Extended Page 31.
 *
 * This page appears to hold fundamental system or global controls. In the
 * driver, it is used by the related LAN8804 to access the
 * LAN8814_CLOCK_MANAGEMENT register.
 */
#define LAN8814_PAGE_SYSTEM_CTRL 31

While these names are not official, they still give useful hints and make the
code more readable. I doubt the LAN8842 has an identical layout, but it looks
similar enough to reuse these patterns for now.

Are there any plans to make the LAN8842 register documentation public? That
would help clarify this further and improve upstream support.

Best regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |