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: [email protected]
Signed-off-by: "Csókás, Bence" <[email protected]>
---
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
Replace hex values with BIT() and GENMASK() for readability
Cc: [email protected]
Signed-off-by: "Csókás, Bence" <[email protected]>
---
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
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
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: [email protected]
>
> Signed-off-by: "Cs?k?s, Bence" <[email protected]>
> ---
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.
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
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: [email protected]
>>
>> Signed-off-by: "Csókás, Bence" <[email protected]>
>> ---
>
> 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
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: [email protected]
> > >
> > > Signed-off-by: "Cs?k?s, Bence" <[email protected]>
> > > ---
> >
> > 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.
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: [email protected]
> > >
> > > Signed-off-by: "Cs?k?s, Bence" <[email protected]>
> > > ---
> >
> > 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