[PATCH V2 0/2] net: phy: Make phy_interface_is_sgmii|rgmii a switch statement

[PATCH V2 0/2] net: phy: Make phy_interface_is_sgmii|rgmii a switch statement

[Nishanth Menon]

Originally reported by Tom[1], turned out to be that recent commit
75d28899e3e9 ("net: phy: Synchronize PHY interface modes with Linux")
reordered the enum definitions which in turn broke the range checks.

we are left with two options:
a) check against explicit values to help reuse as much as possible and
let compiler optimize where applicable
or
b) be very explicit in phy drivers and drop these helpers.

I have chosen to go with (a) approach.

Tested on am64x, though the dp83867 is used elsewhere as well.

Changes since v1:
* Change in switch handling (Thanks Marek for the suggestion)

V1: https://lore.kernel.org/all/20230413180713.2922524-1-nm@ti.com/

Nishanth Menon (2):
  net: phy: Make phy_interface_is_sgmii a switch statement
  net: phy: Make phy_interface_is_rgmii a switch statement

 include/phy.h | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

-- 
2.40.0

[PATCH V2 1/2] net: phy: Make phy_interface_is_sgmii a switch statement

[Nishanth Menon]

Recent commit 75d28899e3e9 ("net: phy: Synchronize PHY interface modes
with Linux") reordered the enum definitions. This caused the range of
enums that this api was checking to go bad.

While it is possible for the phy drivers to practically use the enum's
directly, drivers such as dp83867 use this helper to manage the
configuration of the phy correctly.

Reported-by: Tom Rini <trini@konsulko.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
Changes Since v1:
* Switch update based on feedback from Marek

V1: https://lore.kernel.org/r/20230413180713.2922524-2-nm@ti.com

 include/phy.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/phy.h b/include/phy.h
index a837fed72352..51dadcf14478 100644
--- a/include/phy.h
+++ b/include/phy.h
@@ -373,8 +373,15 @@ static inline bool phy_interface_is_rgmii(struct phy_device *phydev)
  */
 static inline bool phy_interface_is_sgmii(struct phy_device *phydev)
 {
-	return phydev->interface >= PHY_INTERFACE_MODE_SGMII &&
-		phydev->interface <= PHY_INTERFACE_MODE_QSGMII;
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QUSGMII:
+	case PHY_INTERFACE_MODE_USXGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+		return 1;
+	default:
+		return 0;
+	}
 }
 
 bool phy_interface_is_ncsi(void);
-- 
2.40.0

Re: [PATCH V2 1/2] net: phy: Make phy_interface_is_sgmii a switch statement

[Marek Behún]

On Thu, 13 Apr 2023 23:24:32 -0500
Nishanth Menon <nm@ti.com> wrote:

> Recent commit 75d28899e3e9 ("net: phy: Synchronize PHY interface modes
> with Linux") reordered the enum definitions. This caused the range of
> enums that this api was checking to go bad.
> 
> While it is possible for the phy drivers to practically use the enum's
> directly, drivers such as dp83867 use this helper to manage the
> configuration of the phy correctly.
> 
> Reported-by: Tom Rini <trini@konsulko.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
> Changes Since v1:
> * Switch update based on feedback from Marek
> 
> V1: https://lore.kernel.org/r/20230413180713.2922524-2-nm@ti.com
> 
>  include/phy.h | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/include/phy.h b/include/phy.h
> index a837fed72352..51dadcf14478 100644
> --- a/include/phy.h
> +++ b/include/phy.h
> @@ -373,8 +373,15 @@ static inline bool phy_interface_is_rgmii(struct phy_device *phydev)
>   */
>  static inline bool phy_interface_is_sgmii(struct phy_device *phydev)
>  {
> -	return phydev->interface >= PHY_INTERFACE_MODE_SGMII &&
> -		phydev->interface <= PHY_INTERFACE_MODE_QSGMII;
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_QUSGMII:
> +	case PHY_INTERFACE_MODE_USXGMII:
> +	case PHY_INTERFACE_MODE_QSGMII:
> +		return 1;
> +	default:
> +		return 0;
> +	}
>  }
>  
>  bool phy_interface_is_ncsi(void);

Reviewed-by: Marek Behún <kabel@kernel.org>

Please also consider dropping this function, since it is used only in
one driver
  drivers/net/phy/dp83867.c
and that device does not actually support QUSGMII, USXGMII nor QSGMII.
It only supports SGMII.

With rgmii, the corresponding phy_interface_is_rgmii() function makes
sense, but for SGMII the four modes are entirely too different for this
to make sense.

Marek

Re: [PATCH V2 1/2] net: phy: Make phy_interface_is_sgmii a switch statement

[Marek Vasut]

On 4/14/23 06:24, Nishanth Menon wrote:
> Recent commit 75d28899e3e9 ("net: phy: Synchronize PHY interface modes
> with Linux") reordered the enum definitions. This caused the range of
> enums that this api was checking to go bad.
> 
> While it is possible for the phy drivers to practically use the enum's
> directly, drivers such as dp83867 use this helper to manage the
> configuration of the phy correctly.
> 
> Reported-by: Tom Rini <trini@konsulko.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>

Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

It would be nice if you could do a follow-up patch as suggested by Marek 
Behun and inline this function into the TI PHY driver instead.

[PATCH V2 2/2] net: phy: Make phy_interface_is_rgmii a switch statement

[Nishanth Menon]

Recent commit 75d28899e3e9 ("net: phy: Synchronize PHY interface modes
with Linux") reordered the enum definitions. This exposed a problem in
range checking functions to identify the interface type. Though this
specific api was'nt impacted (all the RGMII definitions remained within
range), this experience should be used to never to have to face this
kind of challenge again.

While it is possible for the phy drivers to practically use the enum's
directly, drivers such as dp83867, dp83869, marvell, micrel_ksz90x1 etc
use the same.

Reported-by: Tom Rini <trini@konsulko.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
Changes since V1:
* Switch update based on feedback from Marek

V1: https://lore.kernel.org/r/20230413180713.2922524-3-nm@ti.com
 include/phy.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/phy.h b/include/phy.h
index 51dadcf14478..600966bc8b20 100644
--- a/include/phy.h
+++ b/include/phy.h
@@ -361,8 +361,15 @@ int get_phy_id(struct mii_dev *bus, int addr, int devad, u32 *phy_id);
  */
 static inline bool phy_interface_is_rgmii(struct phy_device *phydev)
 {
-	return phydev->interface >= PHY_INTERFACE_MODE_RGMII &&
-		phydev->interface <= PHY_INTERFACE_MODE_RGMII_TXID;
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		return 1;
+	default:
+		return 0;
+	}
 }
 
 /**
-- 
2.40.0

Re: [PATCH V2 2/2] net: phy: Make phy_interface_is_rgmii a switch statement

[Marek Behún]

On Thu, 13 Apr 2023 23:24:33 -0500
Nishanth Menon <nm@ti.com> wrote:

> Recent commit 75d28899e3e9 ("net: phy: Synchronize PHY interface modes
> with Linux") reordered the enum definitions. This exposed a problem in
> range checking functions to identify the interface type. Though this
> specific api was'nt impacted (all the RGMII definitions remained within
> range), this experience should be used to never to have to face this
> kind of challenge again.
> 
> While it is possible for the phy drivers to practically use the enum's
> directly, drivers such as dp83867, dp83869, marvell, micrel_ksz90x1 etc
> use the same.
> 
> Reported-by: Tom Rini <trini@konsulko.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
> Changes since V1:
> * Switch update based on feedback from Marek
> 
> V1: https://lore.kernel.org/r/20230413180713.2922524-3-nm@ti.com
>  include/phy.h | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/include/phy.h b/include/phy.h
> index 51dadcf14478..600966bc8b20 100644
> --- a/include/phy.h
> +++ b/include/phy.h
> @@ -361,8 +361,15 @@ int get_phy_id(struct mii_dev *bus, int addr, int devad, u32 *phy_id);
>   */
>  static inline bool phy_interface_is_rgmii(struct phy_device *phydev)
>  {
> -	return phydev->interface >= PHY_INTERFACE_MODE_RGMII &&
> -		phydev->interface <= PHY_INTERFACE_MODE_RGMII_TXID;
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		return 1;
> +	default:
> +		return 0;
> +	}
>  }
>  
>  /**

Reviewed-by: Marek Behún <kabel@kernel.org>

Re: [PATCH V2 2/2] net: phy: Make phy_interface_is_rgmii a switch statement

[Marek Vasut]

On 4/14/23 06:24, Nishanth Menon wrote:
> Recent commit 75d28899e3e9 ("net: phy: Synchronize PHY interface modes
> with Linux") reordered the enum definitions. This exposed a problem in
> range checking functions to identify the interface type. Though this
> specific api was'nt impacted (all the RGMII definitions remained within

Nit: "wasn't" , apostrophe in the wrong place .

> range), this experience should be used to never to have to face this
> kind of challenge again.
> 
> While it is possible for the phy drivers to practically use the enum's
> directly, drivers such as dp83867, dp83869, marvell, micrel_ksz90x1 etc
> use the same.
> 
> Reported-by: Tom Rini <trini@konsulko.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>

Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>