[PATCH v2 0/2] sungem: PHY initialization fixes and style cleanup

[PATCH v2 0/2] sungem: PHY initialization fixes and style cleanup

[Joel]

This series splits the previous monolithic patch per feedback from Andrew Lunn.

Patch 1 cleans up pointer formatting and indentation to align with kernel coding standards across the file.

Patch 2 implements hardware validation for the BCM5411 PHY initialization and adds error reporting to the core gem_init_phy logic

Joel (2):
  sungem: fix pointer formatting and indentation
  sungem: validate BCM5411 PHY register reads

 drivers/net/ethernet/sun/sungem.c |  8 +++--
 drivers/net/sungem_phy.c          | 60 +++++++++++++++++--------------
 2 files changed, 39 insertions(+), 29 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] sungem: fix pointer formatting and indentation

[Joel]

Align with Linux Kernel Coding Style by placing the asterisk with the variable name and correcting indentation in bcm54xx_read_link.
---
 drivers/net/sungem_phy.c | 53 ++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

diff --git a/drivers/net/sungem_phy.c b/drivers/net/sungem_phy.c
index c10198d44..44cbe1938 100644
--- a/drivers/net/sungem_phy.c
+++ b/drivers/net/sungem_phy.c
@@ -44,27 +44,27 @@ static const int phy_BCM5400_link_table[8][3] = {
 	{ 1, 0, 1 },	/* 1000BT */
 };
 
-static inline int __sungem_phy_read(struct mii_phy* phy, int id, int reg)
+static inline int __sungem_phy_read(struct mii_phy *phy, int id, int reg)
 {
 	return phy->mdio_read(phy->dev, id, reg);
 }
 
-static inline void __sungem_phy_write(struct mii_phy* phy, int id, int reg, int val)
+static inline void __sungem_phy_write(struct mii_phy *phy, int id, int reg, int val)
 {
 	phy->mdio_write(phy->dev, id, reg, val);
 }
 
-static inline int sungem_phy_read(struct mii_phy* phy, int reg)
+static inline int sungem_phy_read(struct mii_phy *phy, int reg)
 {
 	return phy->mdio_read(phy->dev, phy->mii_id, reg);
 }
 
-static inline void sungem_phy_write(struct mii_phy* phy, int reg, int val)
+static inline void sungem_phy_write(struct mii_phy *phy, int reg, int val)
 {
 	phy->mdio_write(phy->dev, phy->mii_id, reg, val);
 }
 
-static int reset_one_mii_phy(struct mii_phy* phy, int phy_id)
+static int reset_one_mii_phy(struct mii_phy *phy, int phy_id)
 {
 	u16 val;
 	int limit = 10000;
@@ -88,7 +88,7 @@ static int reset_one_mii_phy(struct mii_phy* phy, int phy_id)
 	return limit <= 0;
 }
 
-static int bcm5201_init(struct mii_phy* phy)
+static int bcm5201_init(struct mii_phy *phy)
 {
 	u16 data;
 
@@ -101,7 +101,7 @@ static int bcm5201_init(struct mii_phy* phy)
 	return 0;
 }
 
-static int bcm5201_suspend(struct mii_phy* phy)
+static int bcm5201_suspend(struct mii_phy *phy)
 {
 	sungem_phy_write(phy, MII_BCM5201_INTERRUPT, 0);
 	sungem_phy_write(phy, MII_BCM5201_MULTIPHY, MII_BCM5201_MULTIPHY_SUPERISOLATE);
@@ -109,7 +109,7 @@ static int bcm5201_suspend(struct mii_phy* phy)
 	return 0;
 }
 
-static int bcm5221_init(struct mii_phy* phy)
+static int bcm5221_init(struct mii_phy *phy)
 {
 	u16 data;
 
@@ -132,7 +132,7 @@ static int bcm5221_init(struct mii_phy* phy)
 	return 0;
 }
 
-static int bcm5221_suspend(struct mii_phy* phy)
+static int bcm5221_suspend(struct mii_phy *phy)
 {
 	u16 data;
 
@@ -147,7 +147,7 @@ static int bcm5221_suspend(struct mii_phy* phy)
 	return 0;
 }
 
-static int bcm5241_init(struct mii_phy* phy)
+static int bcm5241_init(struct mii_phy *phy)
 {
 	u16 data;
 
@@ -170,7 +170,7 @@ static int bcm5241_init(struct mii_phy* phy)
 	return 0;
 }
 
-static int bcm5241_suspend(struct mii_phy* phy)
+static int bcm5241_suspend(struct mii_phy *phy)
 {
 	u16 data;
 
@@ -185,7 +185,7 @@ static int bcm5241_suspend(struct mii_phy* phy)
 	return 0;
 }
 
-static int bcm5400_init(struct mii_phy* phy)
+static int bcm5400_init(struct mii_phy *phy)
 {
 	u16 data;
 
@@ -214,7 +214,7 @@ static int bcm5400_init(struct mii_phy* phy)
 	return 0;
 }
 
-static int bcm5400_suspend(struct mii_phy* phy)
+static int bcm5400_suspend(struct mii_phy *phy)
 {
 #if 0 /* Commented out in Darwin... someone has those dawn docs ? */
 	sungem_phy_write(phy, MII_BMCR, BMCR_PDOWN);
@@ -222,7 +222,7 @@ static int bcm5400_suspend(struct mii_phy* phy)
 	return 0;
 }
 
-static int bcm5401_init(struct mii_phy* phy)
+static int bcm5401_init(struct mii_phy *phy)
 {
 	u16 data;
 	int rev;
@@ -270,7 +270,7 @@ static int bcm5401_init(struct mii_phy* phy)
 	return 0;
 }
 
-static int bcm5401_suspend(struct mii_phy* phy)
+static int bcm5401_suspend(struct mii_phy *phy)
 {
 #if 0 /* Commented out in Darwin... someone has those dawn docs ? */
 	sungem_phy_write(phy, MII_BMCR, BMCR_PDOWN);
@@ -278,8 +278,9 @@ static int bcm5401_suspend(struct mii_phy* phy)
 	return 0;
 }
 
-static int bcm5411_init(struct mii_phy* phy)
+static int bcm5411_init(struct mii_phy *phy)
 {
+	int val;
 	u16 data;
 
 	/* Here's some more Apple black magic to setup
@@ -408,14 +409,14 @@ static int genmii_read_link(struct mii_phy *phy)
 	return 0;
 }
 
-static int generic_suspend(struct mii_phy* phy)
+static int generic_suspend(struct mii_phy *phy)
 {
 	sungem_phy_write(phy, MII_BMCR, BMCR_PDOWN);
 
 	return 0;
 }
 
-static int bcm5421_init(struct mii_phy* phy)
+static int bcm5421_init(struct mii_phy *phy)
 {
 	u16 data;
 	unsigned int id;
@@ -548,7 +549,7 @@ static int bcm54xx_read_link(struct mii_phy *phy)
 	u16 val;
 
 	if (phy->autoneg) {
-	    	val = sungem_phy_read(phy, MII_BCM5400_AUXSTATUS);
+		val = sungem_phy_read(phy, MII_BCM5400_AUXSTATUS);
 		link_mode = ((val & MII_BCM5400_AUXSTATUS_LINKMODE_MASK) >>
 			     MII_BCM5400_AUXSTATUS_LINKMODE_SHIFT);
 		phy->duplex = phy_BCM5400_link_table[link_mode][0] ?
@@ -568,7 +569,7 @@ static int bcm54xx_read_link(struct mii_phy *phy)
 	return 0;
 }
 
-static int marvell88e1111_init(struct mii_phy* phy)
+static int marvell88e1111_init(struct mii_phy *phy)
 {
 	u16 rev;
 
@@ -592,7 +593,7 @@ static int marvell88e1111_init(struct mii_phy* phy)
 
 #define BCM5421_MODE_MASK	(1 << 5)
 
-static int bcm5421_poll_link(struct mii_phy* phy)
+static int bcm5421_poll_link(struct mii_phy *phy)
 {
 	u32 phy_reg;
 	int mode;
@@ -616,7 +617,7 @@ static int bcm5421_poll_link(struct mii_phy* phy)
 		return 1;
 }
 
-static int bcm5421_read_link(struct mii_phy* phy)
+static int bcm5421_read_link(struct mii_phy *phy)
 {
 	u32 phy_reg;
 	int mode;
@@ -644,7 +645,7 @@ static int bcm5421_read_link(struct mii_phy* phy)
 	return 0;
 }
 
-static int bcm5421_enable_fiber(struct mii_phy* phy, int autoneg)
+static int bcm5421_enable_fiber(struct mii_phy *phy, int autoneg)
 {
 	/* enable fiber mode */
 	sungem_phy_write(phy, MII_NCONFIG, 0x9020);
@@ -665,7 +666,7 @@ static int bcm5421_enable_fiber(struct mii_phy* phy, int autoneg)
 #define BCM5461_FIBER_LINK	(1 << 2)
 #define BCM5461_MODE_MASK	(3 << 1)
 
-static int bcm5461_poll_link(struct mii_phy* phy)
+static int bcm5461_poll_link(struct mii_phy *phy)
 {
 	u32 phy_reg;
 	int mode;
@@ -691,7 +692,7 @@ static int bcm5461_poll_link(struct mii_phy* phy)
 
 #define BCM5461_FIBER_DUPLEX	(1 << 3)
 
-static int bcm5461_read_link(struct mii_phy* phy)
+static int bcm5461_read_link(struct mii_phy *phy)
 {
 	u32 phy_reg;
 	int mode;
@@ -720,7 +721,7 @@ static int bcm5461_read_link(struct mii_phy* phy)
 	return 0;
 }
 
-static int bcm5461_enable_fiber(struct mii_phy* phy, int autoneg)
+static int bcm5461_enable_fiber(struct mii_phy *phy, int autoneg)
 {
 	/* select fiber mode, enable 1000 base-X registers */
 	sungem_phy_write(phy, MII_NCONFIG, 0xfc0b);
-- 
2.43.0

[PATCH v2 2/2] sungem: validate BCM5411 PHY register reads

[Joel]

Check for -EIO and 0xffff during initialization to prevent silent failures. Add error logging to gem_init_phy to report failures.
---
 drivers/net/ethernet/sun/sungem.c | 8 ++++++--
 drivers/net/sungem_phy.c          | 7 ++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/sun/sungem.c b/drivers/net/ethernet/sun/sungem.c
index 8e69d917d..525aba527 100644
--- a/drivers/net/ethernet/sun/sungem.c
+++ b/drivers/net/ethernet/sun/sungem.c
@@ -1707,8 +1707,12 @@ static void gem_init_phy(struct gem *gp)
 		sungem_phy_probe(&gp->phy_mii, gp->mii_phy_addr);
 
 		/* Init PHY */
-		if (gp->phy_mii.def && gp->phy_mii.def->ops->init)
-			gp->phy_mii.def->ops->init(&gp->phy_mii);
+		if (gp->phy_mii.def && gp->phy_mii.def->ops->init) {
+			int err = gp->phy_mii.def->ops->init(&gp->phy_mii);
+
+			if (err)
+				netdev_err(gp->dev, "PHY init failed: %d\n", err);
+		}
 	} else {
 		gem_pcs_reset(gp);
 		gem_pcs_reinit_adv(gp);
diff --git a/drivers/net/sungem_phy.c b/drivers/net/sungem_phy.c
index 44cbe1938..ec8800c93 100644
--- a/drivers/net/sungem_phy.c
+++ b/drivers/net/sungem_phy.c
@@ -296,7 +296,12 @@ static int bcm5411_init(struct mii_phy *phy)
 	sungem_phy_write(phy, MII_BMCR, BMCR_RESET);
 	sungem_phy_write(phy, MII_BMCR, 0x1340);
 
-	data = sungem_phy_read(phy, MII_BCM5400_GB_CONTROL);
+	val = sungem_phy_read(phy, MII_BCM5400_GB_CONTROL);
+
+	if (val < 0 || val == 0xffff)
+		return -EIO;
+
+	data = (u16)val;
 	data |= MII_BCM5400_GB_CONTROL_FULLDUPLEXCAP;
 	sungem_phy_write(phy, MII_BCM5400_GB_CONTROL, data);
 
-- 
2.43.0

Re: [PATCH v2 2/2] sungem: validate BCM5411 PHY register reads

[Andrew Lunn]

On Fri, Apr 24, 2026 at 03:33:48AM +0000, Joel wrote:
> Check for -EIO and 0xffff during initialization to prevent silent failures. Add error logging to gem_init_phy to report failures.

I assume you have the hardware?

Looking at the git history, there have not been any fixes to this
driver since 2012. Why has this problem suddenly appeared? Is you
hardware getting flaky?

       Andrew