[PATCH net v1] Fixes: xircom auto-negoation timer

[PATCH net v1] Fixes: xircom auto-negoation timer

[Alex Tran]

Auto negoation for DP83840A takes ~3.5 seconds.
Removed sleeping in loop and replaced with timer based completion.

Ignored the CHECK from checkpatch.pl:
CHECK: Avoid CamelCase: <MediaSelect>
GetByte(XIRCREG_ESR) & MediaSelect) ? 1 : 2;

This can be addressed in a separate refactoring patch.

Signed-off-by: Alex Tran <alex.t.tran@gmail.com>
---
 drivers/net/ethernet/xircom/xirc2ps_cs.c | 76 ++++++++++++++++--------
 1 file changed, 50 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/xircom/xirc2ps_cs.c b/drivers/net/ethernet/xircom/xirc2ps_cs.c
index a31d5d5e6..6e552f79b 100644
--- a/drivers/net/ethernet/xircom/xirc2ps_cs.c
+++ b/drivers/net/ethernet/xircom/xirc2ps_cs.c
@@ -100,6 +100,11 @@
 /* Time in jiffies before concluding Tx hung */
 #define TX_TIMEOUT	((400*HZ)/1000)
 
+/* Time in jiffies before autoneg interval ends*/
+#define AUTONEG_TIMEOUT ((100 * HZ) / 1000)
+
+#define RUN_AT(x) (jiffies + (x))
+
 /****************
  * Some constants used to access the hardware
  */
@@ -281,6 +286,9 @@ struct local_info {
     unsigned last_ptr_value; /* last packets transmitted value */
     const char *manf_str;
     struct work_struct tx_timeout_task;
+	struct timer_list timer; /* auto negotiation timer*/
+	int autoneg_attempts;
+	struct completion autoneg_done;
 };
 
 /****************
@@ -300,6 +308,7 @@ static const struct ethtool_ops netdev_ethtool_ops;
 static void hardreset(struct net_device *dev);
 static void do_reset(struct net_device *dev, int full);
 static int init_mii(struct net_device *dev);
+static void autoneg_timer(struct timer_list *t);
 static void do_powerdown(struct net_device *dev);
 static int do_stop(struct net_device *dev);
 
@@ -1561,6 +1570,8 @@ do_reset(struct net_device *dev, int full)
     PutByte(XIRCREG40_TXST1,  0x00); /* TEN, rsv, PTD, EXT, retry_counter:4  */
 
     if (full && local->mohawk && init_mii(dev)) {
+	if (local->probe_port)
+		wait_for_completion(&local->autoneg_done);
 	if (dev->if_port == 4 || local->dingo || local->new_mii) {
 	    netdev_info(dev, "MII selected\n");
 	    SelectPage(2);
@@ -1629,8 +1640,7 @@ init_mii(struct net_device *dev)
 {
     struct local_info *local = netdev_priv(dev);
     unsigned int ioaddr = dev->base_addr;
-    unsigned control, status, linkpartner;
-    int i;
+	unsigned int control, status;
 
     if (if_port == 4 || if_port == 1) { /* force 100BaseT or 10BaseT */
 	dev->if_port = if_port;
@@ -1663,35 +1673,49 @@ init_mii(struct net_device *dev)
     if (local->probe_port) {
 	/* according to the DP83840A specs the auto negotiation process
 	 * may take up to 3.5 sec, so we use this also for our ML6692
-	 * Fixme: Better to use a timer here!
 	 */
-	for (i=0; i < 35; i++) {
-	    msleep(100);	 /* wait 100 msec */
-	    status = mii_rd(ioaddr,  0, 1);
-	    if ((status & 0x0020) && (status & 0x0004))
-		break;
+	local->dev = dev;
+	local->autoneg_attempts = 0;
+	init_completion(&local->autoneg_done);
+	timer_setup(&local->timer, autoneg_timer, 0);
+	local->timer.expires = RUN_AT(AUTONEG_TIMEOUT); /* 100msec intervals*/
+	add_timer(&local->timer);
 	}
 
-	if (!(status & 0x0020)) {
-	    netdev_info(dev, "autonegotiation failed; using 10mbs\n");
-	    if (!local->new_mii) {
-		control = 0x0000;
-		mii_wr(ioaddr,  0, 0, control, 16);
-		udelay(100);
-		SelectPage(0);
-		dev->if_port = (GetByte(XIRCREG_ESR) & MediaSelect) ? 1 : 2;
-	    }
+	return 1;
+}
+
+static void autoneg_timer(struct timer_list *t)
+{
+	struct local_info *local = timer_container_of(local, t, timer);
+	unsigned int ioaddr = local->dev->base_addr;
+	unsigned int status, linkpartner, control;
+
+	status = mii_rd(ioaddr, 0, 1);
+	if ((status & 0x0020) && (status & 0x0004)) {
+		linkpartner = mii_rd(ioaddr, 0, 5);
+		netdev_info(local->dev, "MII link partner: %04x\n",
+			    linkpartner);
+		if (linkpartner & 0x0080)
+			local->dev->if_port = 4;
+		else
+			local->dev->if_port = 1;
+		complete(&local->autoneg_done);
+	} else if (local->autoneg_attempts++ < 35) {
+		mod_timer(&local->timer, RUN_AT(AUTONEG_TIMEOUT));
 	} else {
-	    linkpartner = mii_rd(ioaddr, 0, 5);
-	    netdev_info(dev, "MII link partner: %04x\n", linkpartner);
-	    if (linkpartner & 0x0080) {
-		dev->if_port = 4;
-	    } else
-		dev->if_port = 1;
+		netdev_info(local->dev,
+			    "autonegotiation failed; using 10mbs\n");
+		if (!local->new_mii) {
+			control = 0x0000;
+			mii_wr(ioaddr, 0, 0, control, 16);
+			usleep_range(100, 150);
+			SelectPage(0);
+			local->dev->if_port =
+				(GetByte(XIRCREG_ESR) & MediaSelect) ? 1 : 2;
+		}
+		complete(&local->autoneg_done);
 	}
-    }
-
-    return 1;
 }
 
 static void
-- 
2.51.0

Re: [PATCH net v1] Fixes: xircom auto-negoation timer

[Heiner Kallweit]

On 8/25/2025 3:28 AM, Alex Tran wrote:
> Auto negoation for DP83840A takes ~3.5 seconds.
> Removed sleeping in loop and replaced with timer based completion.
> 
You state this is a fix. Which problem does it fix?

IMO touching such legacy code makes only sense if you:
- fix an actual bug
- reduce complexity
- avoid using deprecated API's

Do you have this hardware for testing your patches?

You might consider migrating this driver to use phylib.
Provided this contributes to reducing complexity.


> Ignored the CHECK from checkpatch.pl:
> CHECK: Avoid CamelCase: <MediaSelect>
> GetByte(XIRCREG_ESR) & MediaSelect) ? 1 : 2;
> 
> This can be addressed in a separate refactoring patch.
> 
> Signed-off-by: Alex Tran <alex.t.tran@gmail.com>
> ---
>  drivers/net/ethernet/xircom/xirc2ps_cs.c | 76 ++++++++++++++++--------
>  1 file changed, 50 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xircom/xirc2ps_cs.c b/drivers/net/ethernet/xircom/xirc2ps_cs.c
> index a31d5d5e6..6e552f79b 100644
> --- a/drivers/net/ethernet/xircom/xirc2ps_cs.c
> +++ b/drivers/net/ethernet/xircom/xirc2ps_cs.c
> @@ -100,6 +100,11 @@
>  /* Time in jiffies before concluding Tx hung */
>  #define TX_TIMEOUT	((400*HZ)/1000)
>  
> +/* Time in jiffies before autoneg interval ends*/
> +#define AUTONEG_TIMEOUT ((100 * HZ) / 1000)
> +
> +#define RUN_AT(x) (jiffies + (x))
> +
>  /****************
>   * Some constants used to access the hardware
>   */
> @@ -281,6 +286,9 @@ struct local_info {
>      unsigned last_ptr_value; /* last packets transmitted value */
>      const char *manf_str;
>      struct work_struct tx_timeout_task;
> +	struct timer_list timer; /* auto negotiation timer*/
> +	int autoneg_attempts;
> +	struct completion autoneg_done;
>  };
>  
>  /****************
> @@ -300,6 +308,7 @@ static const struct ethtool_ops netdev_ethtool_ops;
>  static void hardreset(struct net_device *dev);
>  static void do_reset(struct net_device *dev, int full);
>  static int init_mii(struct net_device *dev);
> +static void autoneg_timer(struct timer_list *t);
>  static void do_powerdown(struct net_device *dev);
>  static int do_stop(struct net_device *dev);
>  
> @@ -1561,6 +1570,8 @@ do_reset(struct net_device *dev, int full)
>      PutByte(XIRCREG40_TXST1,  0x00); /* TEN, rsv, PTD, EXT, retry_counter:4  */
>  
>      if (full && local->mohawk && init_mii(dev)) {
> +	if (local->probe_port)
> +		wait_for_completion(&local->autoneg_done);
>  	if (dev->if_port == 4 || local->dingo || local->new_mii) {
>  	    netdev_info(dev, "MII selected\n");
>  	    SelectPage(2);
> @@ -1629,8 +1640,7 @@ init_mii(struct net_device *dev)
>  {
>      struct local_info *local = netdev_priv(dev);
>      unsigned int ioaddr = dev->base_addr;
> -    unsigned control, status, linkpartner;
> -    int i;
> +	unsigned int control, status;
>  
>      if (if_port == 4 || if_port == 1) { /* force 100BaseT or 10BaseT */
>  	dev->if_port = if_port;
> @@ -1663,35 +1673,49 @@ init_mii(struct net_device *dev)
>      if (local->probe_port) {
>  	/* according to the DP83840A specs the auto negotiation process
>  	 * may take up to 3.5 sec, so we use this also for our ML6692
> -	 * Fixme: Better to use a timer here!
>  	 */
> -	for (i=0; i < 35; i++) {
> -	    msleep(100);	 /* wait 100 msec */
> -	    status = mii_rd(ioaddr,  0, 1);
> -	    if ((status & 0x0020) && (status & 0x0004))
> -		break;
> +	local->dev = dev;
> +	local->autoneg_attempts = 0;
> +	init_completion(&local->autoneg_done);
> +	timer_setup(&local->timer, autoneg_timer, 0);
> +	local->timer.expires = RUN_AT(AUTONEG_TIMEOUT); /* 100msec intervals*/
> +	add_timer(&local->timer);
>  	}
>  
> -	if (!(status & 0x0020)) {
> -	    netdev_info(dev, "autonegotiation failed; using 10mbs\n");
> -	    if (!local->new_mii) {
> -		control = 0x0000;
> -		mii_wr(ioaddr,  0, 0, control, 16);
> -		udelay(100);
> -		SelectPage(0);
> -		dev->if_port = (GetByte(XIRCREG_ESR) & MediaSelect) ? 1 : 2;
> -	    }
> +	return 1;
> +}
> +
> +static void autoneg_timer(struct timer_list *t)
> +{
> +	struct local_info *local = timer_container_of(local, t, timer);
> +	unsigned int ioaddr = local->dev->base_addr;
> +	unsigned int status, linkpartner, control;
> +
> +	status = mii_rd(ioaddr, 0, 1);
> +	if ((status & 0x0020) && (status & 0x0004)) {

These are standard C22 PHY register bits BMSR_LSTATUS and
BMSR_ANEGCOMPLETE.

> +		linkpartner = mii_rd(ioaddr, 0, 5);
> +		netdev_info(local->dev, "MII link partner: %04x\n",
> +			    linkpartner);
> +		if (linkpartner & 0x0080)
> +			local->dev->if_port = 4;
> +		else
> +			local->dev->if_port = 1;
> +		complete(&local->autoneg_done);
> +	} else if (local->autoneg_attempts++ < 35) {
> +		mod_timer(&local->timer, RUN_AT(AUTONEG_TIMEOUT));
>  	} else {
> -	    linkpartner = mii_rd(ioaddr, 0, 5);
> -	    netdev_info(dev, "MII link partner: %04x\n", linkpartner);
> -	    if (linkpartner & 0x0080) {
> -		dev->if_port = 4;
> -	    } else
> -		dev->if_port = 1;
> +		netdev_info(local->dev,
> +			    "autonegotiation failed; using 10mbs\n");
> +		if (!local->new_mii) {
> +			control = 0x0000;
> +			mii_wr(ioaddr, 0, 0, control, 16);
> +			usleep_range(100, 150);
> +			SelectPage(0);
> +			local->dev->if_port =
> +				(GetByte(XIRCREG_ESR) & MediaSelect) ? 1 : 2;
> +		}
> +		complete(&local->autoneg_done);
>  	}
> -    }
> -
> -    return 1;
>  }
>  
>  static void

Re: [PATCH net v1] Fixes: xircom auto-negoation timer

[Andrew Lunn]

On Mon, Aug 25, 2025 at 08:44:18AM +0200, Heiner Kallweit wrote:
> On 8/25/2025 3:28 AM, Alex Tran wrote:
> > Auto negoation for DP83840A takes ~3.5 seconds.
> > Removed sleeping in loop and replaced with timer based completion.
> > 
> You state this is a fix. Which problem does it fix?
> 
> IMO touching such legacy code makes only sense if you:
> - fix an actual bug
> - reduce complexity
> - avoid using deprecated API's
> 
> Do you have this hardware for testing your patches?
> 
> You might consider migrating this driver to use phylib.
> Provided this contributes to reducing complexity.

There is plenty to reduce. There is a full bit-banging MDIO
implementation which could be replaced with the core implementation.

The harder part for converting to phylib will be the ML6692 and
DP83840A. There are no Linux driver for these, but given the age,
there is a good chance genphy will work.

	Andrew

Re: [PATCH net v1] Fixes: xircom auto-negoation timer

[Alex Tran]

> > You state this is a fix. Which problem does it fix?
The change was originally suggested under a FIXME comment but it seems
like more of a cleanup.

> > Do you have this hardware for testing your patches?
I do not have the physical hardware. The change was made based on code
analysis and successful kselftests and kernel compilation on all yes
config.

> > You might consider migrating this driver to use phylib.
> > Provided this contributes to reducing complexity.
>
> There is plenty to reduce. There is a full bit-banging MDIO
> implementation which could be replaced with the core implementation.
>
> The harder part for converting to phylib will be the ML6692 and
> DP83840A. There are no Linux driver for these, but given the age,
> there is a good chance genphy will work.

Migrating the driver to use phylib sounds like a good idea. I can
rework this patch and send in a v2 with the changes if you all are
fine with moving forward with this.



-- 
Alex Tran

Re: [PATCH net v1] Fixes: xircom auto-negoation timer

[Andrew Lunn]

On Mon, Aug 25, 2025 at 10:40:45AM -0700, Alex Tran wrote:
> > > You state this is a fix. Which problem does it fix?
> The change was originally suggested under a FIXME comment but it seems
> like more of a cleanup.
> 
> > > Do you have this hardware for testing your patches?
> I do not have the physical hardware. The change was made based on code
> analysis and successful kselftests and kernel compilation on all yes
> config.

Sorry, without the hardware: NACK.

For old hardware like this, please only work on them if you have the
hardware. Otherwise, you are likely to break it. There is no gain for
work like this, and in effect you just waste reviewer time.

    Andrew

---
pw-bot: cr
	

Re: [PATCH net v1] Fixes: xircom auto-negoation timer

[kernel test robot]

Hi Alex,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Alex-Tran/Fixes-xircom-auto-negoation-timer/20250825-093026
base:   net/main
patch link:    https://lore.kernel.org/r/20250825012821.492355-1-alex.t.tran%40gmail.com
patch subject: [PATCH net v1] Fixes: xircom auto-negoation timer
config: i386-randconfig-r073-20250831 (https://download.01.org/0day-ci/archive/20250831/202508312115.rbF0CO44-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202508312115.rbF0CO44-lkp@intel.com/

New smatch warnings:
drivers/net/ethernet/xircom/xirc2ps_cs.c:1643 init_mii() warn: inconsistent indenting

Old smatch warnings:
drivers/net/ethernet/xircom/xirc2ps_cs.c:1208 xirc2ps_tx_timeout_task() warn: inconsistent indenting
drivers/net/ethernet/xircom/xirc2ps_cs.c:1685 init_mii() warn: inconsistent indenting

vim +1643 drivers/net/ethernet/xircom/xirc2ps_cs.c

  1633	
  1634	/****************
  1635	 * Initialize the Media-Independent-Interface
  1636	 * Returns: True if we have a good MII
  1637	 */
  1638	static int
  1639	init_mii(struct net_device *dev)
  1640	{
  1641	    struct local_info *local = netdev_priv(dev);
  1642	    unsigned int ioaddr = dev->base_addr;
> 1643		unsigned int control, status;
  1644	
  1645	    if (if_port == 4 || if_port == 1) { /* force 100BaseT or 10BaseT */
  1646		dev->if_port = if_port;
  1647		local->probe_port = 0;
  1648		return 1;
  1649	    }
  1650	
  1651	    status = mii_rd(ioaddr,  0, 1);
  1652	    if ((status & 0xff00) != 0x7800)
  1653		return 0; /* No MII */
  1654	
  1655	    local->new_mii = (mii_rd(ioaddr, 0, 2) != 0xffff);
  1656	    
  1657	    if (local->probe_port)
  1658		control = 0x1000; /* auto neg */
  1659	    else if (dev->if_port == 4)
  1660		control = 0x2000; /* no auto neg, 100mbs mode */
  1661	    else
  1662		control = 0x0000; /* no auto neg, 10mbs mode */
  1663	    mii_wr(ioaddr,  0, 0, control, 16);
  1664	    udelay(100);
  1665	    control = mii_rd(ioaddr, 0, 0);
  1666	
  1667	    if (control & 0x0400) {
  1668		netdev_notice(dev, "can't take PHY out of isolation mode\n");
  1669		local->probe_port = 0;
  1670		return 0;
  1671	    }
  1672	
  1673	    if (local->probe_port) {
  1674		/* according to the DP83840A specs the auto negotiation process
  1675		 * may take up to 3.5 sec, so we use this also for our ML6692
  1676		 */
  1677		local->dev = dev;
  1678		local->autoneg_attempts = 0;
  1679		init_completion(&local->autoneg_done);
  1680		timer_setup(&local->timer, autoneg_timer, 0);
  1681		local->timer.expires = RUN_AT(AUTONEG_TIMEOUT); /* 100msec intervals*/
  1682		add_timer(&local->timer);
  1683		}
  1684	
  1685		return 1;
  1686	}
  1687	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki