[RFC PATCH 1/2] net: include: mii: Refactor: Define LPA_* in terms of ADVERTISE_*

[RFC PATCH 1/2] net: include: mii: Refactor: Define LPA_* in terms of ADVERTISE_*

[Csókás, Bence]

Ethernet specification mandates that these bits will be equal.
To reduce the amount of magix hex'es in the code, just define
them in terms of each other.

Cc: trivial@kernel.org

Signed-off-by: "Csókás, Bence" <csokas.bence@prolan.hu>
---
 include/uapi/linux/mii.h | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
index 39f7c44baf53..33e1b0c717e4 100644
--- a/include/uapi/linux/mii.h
+++ b/include/uapi/linux/mii.h
@@ -93,22 +93,22 @@
 				  ADVERTISE_100HALF | ADVERTISE_100FULL)
 
 /* Link partner ability register. */
-#define LPA_SLCT		0x001f	/* Same as advertise selector  */
-#define LPA_10HALF		0x0020	/* Can do 10mbps half-duplex   */
-#define LPA_1000XFULL		0x0020	/* Can do 1000BASE-X full-duplex */
-#define LPA_10FULL		0x0040	/* Can do 10mbps full-duplex   */
-#define LPA_1000XHALF		0x0040	/* Can do 1000BASE-X half-duplex */
-#define LPA_100HALF		0x0080	/* Can do 100mbps half-duplex  */
-#define LPA_1000XPAUSE		0x0080	/* Can do 1000BASE-X pause     */
-#define LPA_100FULL		0x0100	/* Can do 100mbps full-duplex  */
-#define LPA_1000XPAUSE_ASYM	0x0100	/* Can do 1000BASE-X pause asym*/
-#define LPA_100BASE4		0x0200	/* Can do 100mbps 4k packets   */
-#define LPA_PAUSE_CAP		0x0400	/* Can pause                   */
-#define LPA_PAUSE_ASYM		0x0800	/* Can pause asymetrically     */
-#define LPA_RESV		0x1000	/* Unused...                   */
-#define LPA_RFAULT		0x2000	/* Link partner faulted        */
-#define LPA_LPACK		0x4000	/* Link partner acked us       */
-#define LPA_NPAGE		0x8000	/* Next page bit               */
+#define LPA_SLCT		ADVERTISE_SLCT   /* Same as advertise selector */
+#define LPA_10HALF		ADVERTISE_10HALF
+#define LPA_1000XFULL		ADVERTISE_1000XFULL
+#define LPA_10FULL		ADVERTISE_10FULL
+#define LPA_1000XHALF		ADVERTISE_1000XHALF
+#define LPA_100HALF		ADVERTISE_100HALF
+#define LPA_1000XPAUSE		ADVERTISE_1000XPAUSE
+#define LPA_100FULL		ADVERTISE_100FULL
+#define LPA_1000XPAUSE_ASYM	ADVERTISE_1000XPSE_ASYM
+#define LPA_100BASE4		ADVERTISE_100BASE4
+#define LPA_PAUSE_CAP		ADVERTISE_PAUSE_CAP
+#define LPA_PAUSE_ASYM		ADVERTISE_PAUSE_ASYM
+#define LPA_RESV		ADVERTISE_RESV
+#define LPA_RFAULT		ADVERTISE_RFAULT /* Link partner faulted       */
+#define LPA_LPACK		ADVERTISE_LPACK  /* Link partner acked us      */
+#define LPA_NPAGE		ADVERTISE_NPAGE
 
 #define LPA_DUPLEX		(LPA_10FULL | LPA_100FULL)
 #define LPA_100			(LPA_100FULL | LPA_100HALF | LPA_100BASE4)
-- 
2.34.1

[RFC PATCH 2/2] net: include: mii: Refactor: Use BIT() for ADVERTISE_* bits

[Csókás, Bence]

Replace hex values with BIT() and GENMASK() for readability

Cc: trivial@kernel.org

Signed-off-by: "Csókás, Bence" <csokas.bence@prolan.hu>
---
 include/uapi/linux/mii.h | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
index 33e1b0c717e4..f03ac3b35850 100644
--- a/include/uapi/linux/mii.h
+++ b/include/uapi/linux/mii.h
@@ -69,23 +69,23 @@
 #define BMSR_100BASE4		0x8000	/* Can do 100mbps, 4k packets  */
 
 /* Advertisement control register. */
-#define ADVERTISE_SLCT		0x001f	/* Selector bits               */
-#define ADVERTISE_CSMA		0x0001	/* Only selector supported     */
-#define ADVERTISE_10HALF	0x0020	/* Try for 10mbps half-duplex  */
-#define ADVERTISE_1000XFULL	0x0020	/* Try for 1000BASE-X full-duplex */
-#define ADVERTISE_10FULL	0x0040	/* Try for 10mbps full-duplex  */
-#define ADVERTISE_1000XHALF	0x0040	/* Try for 1000BASE-X half-duplex */
-#define ADVERTISE_100HALF	0x0080	/* Try for 100mbps half-duplex */
-#define ADVERTISE_1000XPAUSE	0x0080	/* Try for 1000BASE-X pause    */
-#define ADVERTISE_100FULL	0x0100	/* Try for 100mbps full-duplex */
-#define ADVERTISE_1000XPSE_ASYM	0x0100	/* Try for 1000BASE-X asym pause */
-#define ADVERTISE_100BASE4	0x0200	/* Try for 100mbps 4k packets  */
-#define ADVERTISE_PAUSE_CAP	0x0400	/* Try for pause               */
-#define ADVERTISE_PAUSE_ASYM	0x0800	/* Try for asymetric pause     */
-#define ADVERTISE_RESV		0x1000	/* Unused...                   */
-#define ADVERTISE_RFAULT	0x2000	/* Say we can detect faults    */
-#define ADVERTISE_LPACK		0x4000	/* Ack link partners response  */
-#define ADVERTISE_NPAGE		0x8000	/* Next page bit               */
+#define ADVERTISE_SLCT		GENMASK(4, 0)	/* Selector bits               */
+#define ADVERTISE_CSMA		BIT(0)	/* Only selector supported     */
+#define ADVERTISE_10HALF	BIT(5)	/* Try for 10mbps half-duplex  */
+#define ADVERTISE_1000XFULL	BIT(5)	/* Try for 1000BASE-X full-duplex */
+#define ADVERTISE_10FULL	BIT(6)	/* Try for 10mbps full-duplex  */
+#define ADVERTISE_1000XHALF	BIT(6)	/* Try for 1000BASE-X half-duplex */
+#define ADVERTISE_100HALF	BIT(7)	/* Try for 100mbps half-duplex */
+#define ADVERTISE_1000XPAUSE	BIT(7)	/* Try for 1000BASE-X pause    */
+#define ADVERTISE_100FULL	BIT(8)	/* Try for 100mbps full-duplex */
+#define ADVERTISE_1000XPSE_ASYM	BIT(8)	/* Try for 1000BASE-X asym pause */
+#define ADVERTISE_100BASE4	BIT(9)	/* Try for 100mbps 4k packets  */
+#define ADVERTISE_PAUSE_CAP	BIT(10)	/* Try for pause               */
+#define ADVERTISE_PAUSE_ASYM	BIT(11)	/* Try for asymmetric pause     */
+#define ADVERTISE_RESV		BIT(12)	/* Unused...                   */
+#define ADVERTISE_RFAULT	BIT(13)	/* Say we can detect faults    */
+#define ADVERTISE_LPACK		BIT(14)	/* Ack link partners response  */
+#define ADVERTISE_NPAGE		BIT(15)	/* Next page bit               */
 
 #define ADVERTISE_FULL		(ADVERTISE_100FULL | ADVERTISE_10FULL | \
 				  ADVERTISE_CSMA)
-- 
2.34.1

Re: [RFC PATCH 1/2] net: include: mii: Refactor: Define LPA_* in terms of ADVERTISE_*

[Andrew Lunn]

On Wed, Jun 05, 2024 at 02:16:47PM +0200, Csókás, Bence wrote:
> Ethernet specification mandates that these bits will be equal.
> To reduce the amount of magix hex'es in the code, just define
> them in terms of each other.

Are magic hexes in this context actually bad? In .c files i would
agree. But what you have in effect done is force me into jump another
hoop to find the actual hex value so i can manually decode a register
value. And you have made the compile slightly slower.

These defines have been like this since the beginning of the git
history. Is there a good reason to change them after all that time?

	Andrew

Re: [RFC PATCH 1/2] net: include: mii: Refactor: Define LPA_* in terms of ADVERTISE_*

[Csókás Bence]

Hi!

On 6/5/24 14:51, Andrew Lunn wrote:
> On Wed, Jun 05, 2024 at 02:16:47PM +0200, Csókás, Bence wrote:
>> Ethernet specification mandates that these bits will be equal.
>> To reduce the amount of magix hex'es in the code, just define
>> them in terms of each other.
> 
> Are magic hexes in this context actually bad?

Yes, as if it ever needs to be changed (for instance in the 2/2 of the 
series, when I replaced them with BIT() macros), it needs to be changed 
twice in the file.

> In .c files i would
> agree. But what you have in effect done is force me into jump another
> hoop to find the actual hex value so i can manually decode a register
> value.

True. I expected this concern, hence why I tagged this as RFC. However, 
I believe that from a maintainability perspective, it's best to only 
have one definition, and since these #define's are right under one 
another, the "jumping around" is minimal anyways.

> And you have made the compile slightly slower.

C'mon, that's negligible. The time it takes to load the header file from 
disk will probably take longer than it does to resolve an extra layer of 
simple #define's.

> These defines have been like this since the beginning of the git
> history. Is there a good reason to change them after all that time?

Just because something was "always like this" doesn't mean that it 
cannot be changed. Especially since this patch is 100% 
backwards-compatible, just maybe slightly more future-proof.

> 	Andrew
> 

Bence

Re: [RFC PATCH 2/2] net: include: mii: Refactor: Use BIT() for ADVERTISE_* bits

[Vladimir Oltean]

On Wed, Jun 05, 2024 at 02:16:49PM +0200, Csókás, Bence wrote:
> Replace hex values with BIT() and GENMASK() for readability
> 
> Cc: trivial@kernel.org
> 
> Signed-off-by: "Csókás, Bence" <csokas.bence@prolan.hu>
> ---

You can't use BIT() and GENMASK() in headers exported to user space.

I mean you can, but the BIT() and GENMASK() macros themselves aren't
exported to user space, and you would break any application which used
values dependent on them.

Re: [RFC PATCH 2/2] net: include: mii: Refactor: Use BIT() for ADVERTISE_* bits

[Csókás Bence]

Hi!

On 6/5/24 16:13, Vladimir Oltean wrote:
> On Wed, Jun 05, 2024 at 02:16:49PM +0200, Csókás, Bence wrote:
>> Replace hex values with BIT() and GENMASK() for readability
>>
>> Cc: trivial@kernel.org
>>
>> Signed-off-by: "Csókás, Bence" <csokas.bence@prolan.hu>
>> ---
> 
> You can't use BIT() and GENMASK() in headers exported to user space.
> 
> I mean you can, but the BIT() and GENMASK() macros themselves aren't
> exported to user space, and you would break any application which used
> values dependent on them.
> 

I thought the vDSO headers (which currently hold the definition for 
`BIT()`) *are* exported. Though `GENMASK()`, and the headers which would 
normally include vdso/bits.h, might not be... But then again, is 
uapi/linux/mii.h itself even exported? And if so, why aren't these 
macros? Is there any reason _not_ to export the entire linux/bits.h?

Bence

Re: [RFC PATCH 2/2] net: include: mii: Refactor: Use BIT() for ADVERTISE_* bits

[Vladimir Oltean]

On Wed, Jun 05, 2024 at 04:47:27PM +0200, Csókás Bence wrote:
> Hi!
> 
> On 6/5/24 16:13, Vladimir Oltean wrote:
> > On Wed, Jun 05, 2024 at 02:16:49PM +0200, Csókás, Bence wrote:
> > > Replace hex values with BIT() and GENMASK() for readability
> > > 
> > > Cc: trivial@kernel.org
> > > 
> > > Signed-off-by: "Csókás, Bence" <csokas.bence@prolan.hu>
> > > ---
> > 
> > You can't use BIT() and GENMASK() in headers exported to user space.
> > 
> > I mean you can, but the BIT() and GENMASK() macros themselves aren't
> > exported to user space, and you would break any application which used
> > values dependent on them.
> > 
> 
> I thought the vDSO headers (which currently hold the definition for `BIT()`)
> *are* exported. Though `GENMASK()`, and the headers which would normally
> include vdso/bits.h, might not be... But then again, is uapi/linux/mii.h
> itself even exported?

grep through the output of "make -j 8 headers_install O=headers" is
a good place to start.

> And if so, why aren't these macros? Is there any reason _not_ to
> export the entire linux/bits.h?

Sorry, I'm not the person who can answer these questions.

Re: [RFC PATCH 2/2] net: include: mii: Refactor: Use BIT() for ADVERTISE_* bits

[Andrew Lunn]

On Wed, Jun 05, 2024 at 04:47:27PM +0200, Csókás Bence wrote:
> Hi!
> 
> On 6/5/24 16:13, Vladimir Oltean wrote:
> > On Wed, Jun 05, 2024 at 02:16:49PM +0200, Csókás, Bence wrote:
> > > Replace hex values with BIT() and GENMASK() for readability
> > > 
> > > Cc: trivial@kernel.org
> > > 
> > > Signed-off-by: "Csókás, Bence" <csokas.bence@prolan.hu>
> > > ---
> > 
> > You can't use BIT() and GENMASK() in headers exported to user space.
> > 
> > I mean you can, but the BIT() and GENMASK() macros themselves aren't
> > exported to user space, and you would break any application which used
> > values dependent on them.
> > 
> 
> I thought the vDSO headers (which currently hold the definition for `BIT()`)
> *are* exported. Though `GENMASK()`, and the headers which would normally
> include vdso/bits.h, might not be... But then again, is uapi/linux/mii.h
> itself even exported?

uapi .... I would expect everything below that is considered exported.

Take a look at the sources for mii-tool.c:

    if (bmcr & BMCR_ANENABLE) {
        if (bmsr & BMSR_ANEGCOMPLETE) {
            if (advert & lkpar) {
                strcat(buf, (lkpar & LPA_LPACK) ?
                       "negotiated" : "no autonegotiation,");
                strcat(buf, media_list(advert & lkpar, bmcr2 & lpa2>>2, 1));
                strcat(buf, ", ");
            } else {
                strcat(buf, "autonegotiation failed, ");
            }
        } else if (bmcr & BMCR_ANRESTART) {
            strcat(buf, "autonegotiation restarted, ");
        }
    } else {
        sprintf(buf+strlen(buf), "%s Mbit, %s duplex, ",
                ((bmcr2 & (ADVERTISE_1000HALF | ADVERTISE_1000FULL)) & lpa2 >> 2)
                ? "1000"
                : (bmcr & BMCR_SPEED100) ? "100" : "10",
                (bmcr & BMCR_FULLDPLX) ? "full" : "half");
    }
    strcat(buf, (bmsr & BMSR_LSTATUS) ? "link ok" : "no link");

So they are actually used as well. Try compiling this with your
changes made.

	Andrew