drivers/net/ethernet/xircom/xirc2ps_cs.c | 76 ++++++++++++++++-------- 1 file changed, 50 insertions(+), 26 deletions(-)
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.0Hi 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
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
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
> > 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
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
© 2016 - 2025 Red Hat, Inc.