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

[PATCH v2 resub 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

[PATCH v2 resub 2/2] net: include: mii: Refactor: Use bit ops for ADVERTISE_* bits

[Csókás, Bence]

Replace hex values with bit shift and __GENMASK() for readability

Cc: trivial@kernel.org

Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
---

Notes:
    Changes since v2:
    * Replace BIT() with bit shift, as the macro is not exported to userspace
    * Use __GENMASK(), exported into userspace in 3c7a8e190bc5
    
    (yesterday I accidentally sent v1 again, this is the correct v2)

 include/uapi/linux/mii.h | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
index 33e1b0c717e4..3fbc113a0f70 100644
--- a/include/uapi/linux/mii.h
+++ b/include/uapi/linux/mii.h
@@ -9,6 +9,7 @@
 #ifndef _UAPI__LINUX_MII_H__
 #define _UAPI__LINUX_MII_H__
 
+#include <linux/bits.h>
 #include <linux/types.h>
 #include <linux/ethtool.h>
 
@@ -69,23 +70,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		(1 << 0)	/* Only selector supported     */
+#define ADVERTISE_10HALF	(1 << 5)	/* Try for 10mbps half-duplex  */
+#define ADVERTISE_1000XFULL	(1 << 5)	/* Try for 1000BASE-X full-duplex */
+#define ADVERTISE_10FULL	(1 << 6)	/* Try for 10mbps full-duplex  */
+#define ADVERTISE_1000XHALF	(1 << 6)	/* Try for 1000BASE-X half-duplex */
+#define ADVERTISE_100HALF	(1 << 7)	/* Try for 100mbps half-duplex */
+#define ADVERTISE_1000XPAUSE	(1 << 7)	/* Try for 1000BASE-X pause    */
+#define ADVERTISE_100FULL	(1 << 8)	/* Try for 100mbps full-duplex */
+#define ADVERTISE_1000XPSE_ASYM	(1 << 8)	/* Try for 1000BASE-X asym pause */
+#define ADVERTISE_100BASE4	(1 << 9)	/* Try for 100mbps 4k packets  */
+#define ADVERTISE_PAUSE_CAP	(1 << 10)	/* Try for pause               */
+#define ADVERTISE_PAUSE_ASYM	(1 << 11)	/* Try for asymmetric pause     */
+#define ADVERTISE_RESV		(1 << 12)	/* Unused...                   */
+#define ADVERTISE_RFAULT	(1 << 13)	/* Say we can detect faults    */
+#define ADVERTISE_LPACK		(1 << 14)	/* Ack link partners response  */
+#define ADVERTISE_NPAGE		(1 << 15)	/* Next page bit               */
 
 #define ADVERTISE_FULL		(ADVERTISE_100FULL | ADVERTISE_10FULL | \
 				  ADVERTISE_CSMA)
-- 
2.34.1

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

[Andrew Lunn]

On Wed, Jun 19, 2024 at 02:46:22PM +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.

I have a quick email exchange with other PHY maintainers, and we
agree. We will reject these changes, they are just churn and bring no
real benefit.

NACK

    Andrew

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

[Csókás Bence]

Hi

On 6/20/24 21:07, Andrew Lunn wrote:
> On Wed, Jun 19, 2024 at 02:46:22PM +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.
> 
> I have a quick email exchange with other PHY maintainers, and we
> agree. We will reject these changes, they are just churn and bring no
> real benefit.
> 
> NACK
> 
>      Andrew
> 

The benefit is that I don't have to constantly convert between "n-th bit 
set" (which is how virtually all datasheets, specifications, 
documentation etc. represent MII bits) and these hex values. In most 
places in the kernel, register bits are already represented with BIT() 
et al., so why not here?

Bence

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

[Russell King (Oracle)]

On Fri, Jun 21, 2024 at 09:48:23AM +0200, Csókás Bence wrote:
> Hi
> 
> On 6/20/24 21:07, Andrew Lunn wrote:
> > On Wed, Jun 19, 2024 at 02:46:22PM +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.
> > 
> > I have a quick email exchange with other PHY maintainers, and we
> > agree. We will reject these changes, they are just churn and bring no
> > real benefit.
> > 
> > NACK
> > 
> >      Andrew
> > 
> 
> The benefit is that I don't have to constantly convert between "n-th bit
> set" (which is how virtually all datasheets, specifications, documentation
> etc. represent MII bits) and these hex values. In most places in the kernel,
> register bits are already represented with BIT() et al., so why not here?

These are user API files, you can't use BIT() here (BIT() isn't defined
for userspace header files.) Next, these are 'int's not 'longs' so
using BIT() or _BITUL() could cause warnings - plus it changes the
type of these definitions not only for kernel space but also user space.
Thus, it's an API change.

So no, we're not making changes to make this "more readable" at the
expense of breaking the kernel's UAPI.

Just get used to working with hex numbers like most of us had to do
before BIT() was added to the kernel... it's not difficult, each hex
digit is after all four binary bits. It's not like it's decimal.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!