| Message ID | 20240605121648.69779-2-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | [RFC,1/2] net: include: mii: Refactor: Define LPA_* in terms of ADVERTISE_* | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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.
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
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.
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
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)
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(-)