| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c6c354ec-e4fe-4b80-b2e5-9f6c8350b504@gmail.com>
Date: Mon, 25 Aug 2025 08:44:18 +0200
From: Heiner Kallweit <hkallweit1@...il.com>
To: Alex Tran <alex.t.tran@...il.com>, Andrew Lunn <andrew+netdev@...n.ch>
Cc: "David S . Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v1] Fixes: xircom auto-negoation timer
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@...il.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
Powered by blists - more mailing lists