diff mbox series

[net-next,v3,1/9] net: sunhme: Just restart autonegotiation if we can't bring the link up

Message ID 20230314003613.3874089-2-seanga2@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: sunhme: Probe/IRQ cleanups | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 18 this patch: 18
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch warning CHECK: spaces preferred around that '/' (ctx:VxV) CHECK: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.rst WARNING: line length of 81 exceeds 80 columns WARNING: line length of 82 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Sean Anderson March 14, 2023, 12:36 a.m. UTC
If we've tried regular autonegotiation and forcing the link mode, just
restart autonegotiation instead of reinitializing the whole NIC.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v2)

Changes in v2:
- Move happy_meal_begin_auto_negotiation earlier and remove forward declaration

 drivers/net/ethernet/sun/sunhme.c | 245 +++++++++++++++---------------
 1 file changed, 119 insertions(+), 126 deletions(-)

Comments

Simon Horman March 15, 2023, 8:20 a.m. UTC | #1
On Mon, Mar 13, 2023 at 08:36:05PM -0400, Sean Anderson wrote:
> If we've tried regular autonegotiation and forcing the link mode, just
> restart autonegotiation instead of reinitializing the whole NIC.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>

Hi Sean,

This patch looks fine to me, as do patches 3 - 4, which is as far as I have
got with my review.

I do, however, have a general question regarding most of the patches in this
series: to what extent have they been tested on HW?

And my follow-up question is: to what extent should we consider removing
support for hardware that isn't being tested and therefore has/will likely
have become broken break at some point? Quattro, the subject of a latter
patch in this series, seems to be a case in point.
Sean Anderson March 15, 2023, 3:35 p.m. UTC | #2
On 3/15/23 04:20, Simon Horman wrote:
> On Mon, Mar 13, 2023 at 08:36:05PM -0400, Sean Anderson wrote:
>> If we've tried regular autonegotiation and forcing the link mode, just
>> restart autonegotiation instead of reinitializing the whole NIC.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> 
> Hi Sean,
> 
> This patch looks fine to me, as do patches 3 - 4, which is as far as I have
> got with my review.
> 
> I do, however, have a general question regarding most of the patches in this
> series: to what extent have they been tested on HW?

I have tested them with some PCI cards, mostly with the other end autonegotiating
100M. This series doesn't really touch the phy state machines, so I think it is
fine to just make sure the link comes up (and things work after bringing the
interface down and up).

> And my follow-up question is: to what extent should we consider removing
> support for hardware that isn't being tested and therefore has/will likely
> have become broken break at some point? Quattro, the subject of a latter
> patch in this series, seems to be a case in point.

Well, I ordered a quattro card (this hardware is quite cheap on ebay) so
hopefully I can test that. The real question is whether there's anyone using this
on sparc. I tried CCing some sparc users mailing lists in the cover letter, but no
luck so far.

--Sean
Simon Horman March 15, 2023, 3:57 p.m. UTC | #3
On Wed, Mar 15, 2023 at 11:35:19AM -0400, Sean Anderson wrote:
> On 3/15/23 04:20, Simon Horman wrote:
> > On Mon, Mar 13, 2023 at 08:36:05PM -0400, Sean Anderson wrote:
> > > If we've tried regular autonegotiation and forcing the link mode, just
> > > restart autonegotiation instead of reinitializing the whole NIC.
> > > 
> > > Signed-off-by: Sean Anderson <seanga2@gmail.com>
> > 
> > Hi Sean,
> > 
> > This patch looks fine to me, as do patches 3 - 4, which is as far as I have
> > got with my review.
> > 
> > I do, however, have a general question regarding most of the patches in this
> > series: to what extent have they been tested on HW?
> 
> I have tested them with some PCI cards, mostly with the other end
> autonegotiating 100M. This series doesn't really touch the phy state
> machines, so I think it is fine to just make sure the link comes up (and
> things work after bringing the interface down and up).

Understood, I'll proceed with my review on that basis.

> > And my follow-up question is: to what extent should we consider removing
> > support for hardware that isn't being tested and therefore has/will likely
> > have become broken break at some point? Quattro, the subject of a latter
> > patch in this series, seems to be a case in point.
> 
> Well, I ordered a quattro card (this hardware is quite cheap on ebay) so
> hopefully I can test that.

Yes, for some reason I was searching on eBay too :)

> The real question is whether there's anyone using this on sparc. I tried
> CCing some sparc users mailing lists in the cover letter, but no luck so
> far.

Yes, that is the question.
Simon Horman March 15, 2023, 3:58 p.m. UTC | #4
On Mon, Mar 13, 2023 at 08:36:05PM -0400, Sean Anderson wrote:
> If we've tried regular autonegotiation and forcing the link mode, just
> restart autonegotiation instead of reinitializing the whole NIC.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>
Simon Horman March 18, 2023, 8:37 a.m. UTC | #5
On Mon, Mar 13, 2023 at 08:36:05PM -0400, Sean Anderson wrote:
> If we've tried regular autonegotiation and forcing the link mode, just
> restart autonegotiation instead of reinitializing the whole NIC.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>

...

> @@ -606,6 +604,124 @@ static int is_lucent_phy(struct happy_meal *hp)
>  	return ret;
>  }
>  
> +/* hp->happy_lock must be held */
> +static void
> +happy_meal_begin_auto_negotiation(struct happy_meal *hp,
> +				  void __iomem *tregs,
> +				  const struct ethtool_link_ksettings *ep)
> +{
> +	int timeout;
> +
> +	/* Read all of the registers we are interested in now. */
> +	hp->sw_bmsr      = happy_meal_tcvr_read(hp, tregs, MII_BMSR);
> +	hp->sw_bmcr      = happy_meal_tcvr_read(hp, tregs, MII_BMCR);
> +	hp->sw_physid1   = happy_meal_tcvr_read(hp, tregs, MII_PHYSID1);
> +	hp->sw_physid2   = happy_meal_tcvr_read(hp, tregs, MII_PHYSID2);
> +
> +	/* XXX Check BMSR_ANEGCAPABLE, should not be necessary though. */
> +
> +	hp->sw_advertise = happy_meal_tcvr_read(hp, tregs, MII_ADVERTISE);
> +	if (!ep || ep->base.autoneg == AUTONEG_ENABLE) {
> +		/* Advertise everything we can support. */
> +		if (hp->sw_bmsr & BMSR_10HALF)
> +			hp->sw_advertise |= (ADVERTISE_10HALF);
> +		else
> +			hp->sw_advertise &= ~(ADVERTISE_10HALF);
> +
> +		if (hp->sw_bmsr & BMSR_10FULL)
> +			hp->sw_advertise |= (ADVERTISE_10FULL);
> +		else
> +			hp->sw_advertise &= ~(ADVERTISE_10FULL);
> +		if (hp->sw_bmsr & BMSR_100HALF)
> +			hp->sw_advertise |= (ADVERTISE_100HALF);
> +		else
> +			hp->sw_advertise &= ~(ADVERTISE_100HALF);
> +		if (hp->sw_bmsr & BMSR_100FULL)
> +			hp->sw_advertise |= (ADVERTISE_100FULL);
> +		else
> +			hp->sw_advertise &= ~(ADVERTISE_100FULL);
> +		happy_meal_tcvr_write(hp, tregs, MII_ADVERTISE, hp->sw_advertise);
> +
> +		/* XXX Currently no Happy Meal cards I know off support 100BaseT4,
> +		 * XXX and this is because the DP83840 does not support it, changes
> +		 * XXX would need to be made to the tx/rx logic in the driver as well
> +		 * XXX so I completely skip checking for it in the BMSR for now.
> +		 */
> +
> +		ASD("Advertising [ %s%s%s%s]\n",
> +		    hp->sw_advertise & ADVERTISE_10HALF ? "10H " : "",
> +		    hp->sw_advertise & ADVERTISE_10FULL ? "10F " : "",
> +		    hp->sw_advertise & ADVERTISE_100HALF ? "100H " : "",
> +		    hp->sw_advertise & ADVERTISE_100FULL ? "100F " : "");
> +
> +		/* Enable Auto-Negotiation, this is usually on already... */
> +		hp->sw_bmcr |= BMCR_ANENABLE;
> +		happy_meal_tcvr_write(hp, tregs, MII_BMCR, hp->sw_bmcr);
> +
> +		/* Restart it to make sure it is going. */
> +		hp->sw_bmcr |= BMCR_ANRESTART;
> +		happy_meal_tcvr_write(hp, tregs, MII_BMCR, hp->sw_bmcr);
> +
> +		/* BMCR_ANRESTART self clears when the process has begun. */
> +
> +		timeout = 64;  /* More than enough. */
> +		while (--timeout) {
> +			hp->sw_bmcr = happy_meal_tcvr_read(hp, tregs, MII_BMCR);
> +			if (!(hp->sw_bmcr & BMCR_ANRESTART))
> +				break; /* got it. */
> +			udelay(10);

nit: Checkpatch tells me that usleep_range() is preferred over udelay().
     Perhaps it would be worth looking into that for a follow-up patch.

> +		}
> +		if (!timeout) {
> +			netdev_err(hp->dev,
> +				   "Happy Meal would not start auto negotiation BMCR=0x%04x\n",
> +				   hp->sw_bmcr);
> +			netdev_notice(hp->dev,
> +				      "Performing force link detection.\n");
> +			goto force_link;
> +		} else {
> +			hp->timer_state = arbwait;
> +		}
> +	} else {
> +force_link:
> +		/* Force the link up, trying first a particular mode.
> +		 * Either we are here at the request of ethtool or
> +		 * because the Happy Meal would not start to autoneg.
> +		 */
> +
> +		/* Disable auto-negotiation in BMCR, enable the duplex and
> +		 * speed setting, init the timer state machine, and fire it off.
> +		 */
> +		if (!ep || ep->base.autoneg == AUTONEG_ENABLE) {
> +			hp->sw_bmcr = BMCR_SPEED100;
> +		} else {
> +			if (ep->base.speed == SPEED_100)
> +				hp->sw_bmcr = BMCR_SPEED100;
> +			else
> +				hp->sw_bmcr = 0;
> +			if (ep->base.duplex == DUPLEX_FULL)
> +				hp->sw_bmcr |= BMCR_FULLDPLX;
> +		}
> +		happy_meal_tcvr_write(hp, tregs, MII_BMCR, hp->sw_bmcr);
> +
> +		if (!is_lucent_phy(hp)) {
> +			/* OK, seems we need do disable the transceiver for the first
> +			 * tick to make sure we get an accurate link state at the
> +			 * second tick.
> +			 */
> +			hp->sw_csconfig = happy_meal_tcvr_read(hp, tregs,
> +							       DP83840_CSCONFIG);
> +			hp->sw_csconfig &= ~(CSCONFIG_TCVDISAB);
> +			happy_meal_tcvr_write(hp, tregs, DP83840_CSCONFIG,
> +					      hp->sw_csconfig);
> +		}
> +		hp->timer_state = ltrywait;
> +	}
> +
> +	hp->timer_ticks = 0;
> +	hp->happy_timer.expires = jiffies + (12 * HZ)/10;  /* 1.2 sec. */

nit: as a follow-up perhaps you could consider something like this.
     (* completely untested! * )

	hp->happy_timer.expires = jiffies + msecs_to_jiffies(1200);

> +	add_timer(&hp->happy_timer);
> +}
> +

...
Sean Anderson March 18, 2023, 2:31 p.m. UTC | #6
On 3/18/23 04:37, Simon Horman wrote:
> On Mon, Mar 13, 2023 at 08:36:05PM -0400, Sean Anderson wrote:
>> If we've tried regular autonegotiation and forcing the link mode, just
>> restart autonegotiation instead of reinitializing the whole NIC.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> 
> ...
> 
>> @@ -606,6 +604,124 @@ static int is_lucent_phy(struct happy_meal *hp)
>>   	return ret;
>>   }
>>   
>> +/* hp->happy_lock must be held */
>> +static void
>> +happy_meal_begin_auto_negotiation(struct happy_meal *hp,
>> +				  void __iomem *tregs,
>> +				  const struct ethtool_link_ksettings *ep)
>> +{
>> +	int timeout;
>> +
>> +	/* Read all of the registers we are interested in now. */
>> +	hp->sw_bmsr      = happy_meal_tcvr_read(hp, tregs, MII_BMSR);
>> +	hp->sw_bmcr      = happy_meal_tcvr_read(hp, tregs, MII_BMCR);
>> +	hp->sw_physid1   = happy_meal_tcvr_read(hp, tregs, MII_PHYSID1);
>> +	hp->sw_physid2   = happy_meal_tcvr_read(hp, tregs, MII_PHYSID2);
>> +
>> +	/* XXX Check BMSR_ANEGCAPABLE, should not be necessary though. */
>> +
>> +	hp->sw_advertise = happy_meal_tcvr_read(hp, tregs, MII_ADVERTISE);
>> +	if (!ep || ep->base.autoneg == AUTONEG_ENABLE) {
>> +		/* Advertise everything we can support. */
>> +		if (hp->sw_bmsr & BMSR_10HALF)
>> +			hp->sw_advertise |= (ADVERTISE_10HALF);
>> +		else
>> +			hp->sw_advertise &= ~(ADVERTISE_10HALF);
>> +
>> +		if (hp->sw_bmsr & BMSR_10FULL)
>> +			hp->sw_advertise |= (ADVERTISE_10FULL);
>> +		else
>> +			hp->sw_advertise &= ~(ADVERTISE_10FULL);
>> +		if (hp->sw_bmsr & BMSR_100HALF)
>> +			hp->sw_advertise |= (ADVERTISE_100HALF);
>> +		else
>> +			hp->sw_advertise &= ~(ADVERTISE_100HALF);
>> +		if (hp->sw_bmsr & BMSR_100FULL)
>> +			hp->sw_advertise |= (ADVERTISE_100FULL);
>> +		else
>> +			hp->sw_advertise &= ~(ADVERTISE_100FULL);
>> +		happy_meal_tcvr_write(hp, tregs, MII_ADVERTISE, hp->sw_advertise);
>> +
>> +		/* XXX Currently no Happy Meal cards I know off support 100BaseT4,
>> +		 * XXX and this is because the DP83840 does not support it, changes
>> +		 * XXX would need to be made to the tx/rx logic in the driver as well
>> +		 * XXX so I completely skip checking for it in the BMSR for now.
>> +		 */
>> +
>> +		ASD("Advertising [ %s%s%s%s]\n",
>> +		    hp->sw_advertise & ADVERTISE_10HALF ? "10H " : "",
>> +		    hp->sw_advertise & ADVERTISE_10FULL ? "10F " : "",
>> +		    hp->sw_advertise & ADVERTISE_100HALF ? "100H " : "",
>> +		    hp->sw_advertise & ADVERTISE_100FULL ? "100F " : "");
>> +
>> +		/* Enable Auto-Negotiation, this is usually on already... */
>> +		hp->sw_bmcr |= BMCR_ANENABLE;
>> +		happy_meal_tcvr_write(hp, tregs, MII_BMCR, hp->sw_bmcr);
>> +
>> +		/* Restart it to make sure it is going. */
>> +		hp->sw_bmcr |= BMCR_ANRESTART;
>> +		happy_meal_tcvr_write(hp, tregs, MII_BMCR, hp->sw_bmcr);
>> +
>> +		/* BMCR_ANRESTART self clears when the process has begun. */
>> +
>> +		timeout = 64;  /* More than enough. */
>> +		while (--timeout) {
>> +			hp->sw_bmcr = happy_meal_tcvr_read(hp, tregs, MII_BMCR);
>> +			if (!(hp->sw_bmcr & BMCR_ANRESTART))
>> +				break; /* got it. */
>> +			udelay(10);
> 
> nit: Checkpatch tells me that usleep_range() is preferred over udelay().
>       Perhaps it would be worth looking into that for a follow-up patch.

This will be fixed in another series.

>> +		}
>> +		if (!timeout) {
>> +			netdev_err(hp->dev,
>> +				   "Happy Meal would not start auto negotiation BMCR=0x%04x\n",
>> +				   hp->sw_bmcr);
>> +			netdev_notice(hp->dev,
>> +				      "Performing force link detection.\n");
>> +			goto force_link;
>> +		} else {
>> +			hp->timer_state = arbwait;
>> +		}
>> +	} else {
>> +force_link:
>> +		/* Force the link up, trying first a particular mode.
>> +		 * Either we are here at the request of ethtool or
>> +		 * because the Happy Meal would not start to autoneg.
>> +		 */
>> +
>> +		/* Disable auto-negotiation in BMCR, enable the duplex and
>> +		 * speed setting, init the timer state machine, and fire it off.
>> +		 */
>> +		if (!ep || ep->base.autoneg == AUTONEG_ENABLE) {
>> +			hp->sw_bmcr = BMCR_SPEED100;
>> +		} else {
>> +			if (ep->base.speed == SPEED_100)
>> +				hp->sw_bmcr = BMCR_SPEED100;
>> +			else
>> +				hp->sw_bmcr = 0;
>> +			if (ep->base.duplex == DUPLEX_FULL)
>> +				hp->sw_bmcr |= BMCR_FULLDPLX;
>> +		}
>> +		happy_meal_tcvr_write(hp, tregs, MII_BMCR, hp->sw_bmcr);
>> +
>> +		if (!is_lucent_phy(hp)) {
>> +			/* OK, seems we need do disable the transceiver for the first
>> +			 * tick to make sure we get an accurate link state at the
>> +			 * second tick.
>> +			 */
>> +			hp->sw_csconfig = happy_meal_tcvr_read(hp, tregs,
>> +							       DP83840_CSCONFIG);
>> +			hp->sw_csconfig &= ~(CSCONFIG_TCVDISAB);
>> +			happy_meal_tcvr_write(hp, tregs, DP83840_CSCONFIG,
>> +					      hp->sw_csconfig);
>> +		}
>> +		hp->timer_state = ltrywait;
>> +	}
>> +
>> +	hp->timer_ticks = 0;
>> +	hp->happy_timer.expires = jiffies + (12 * HZ)/10;  /* 1.2 sec. */
> 
> nit: as a follow-up perhaps you could consider something like this.
>       (* completely untested! * )
> 
> 	hp->happy_timer.expires = jiffies + msecs_to_jiffies(1200);

ditto.

>> +	add_timer(&hp->happy_timer);
>> +}
>> +
> 
> ...

--Sean
diff mbox series

Patch

diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
index b0c7ab74a82e..65733ee5ddd9 100644
--- a/drivers/net/ethernet/sun/sunhme.c
+++ b/drivers/net/ethernet/sun/sunhme.c
@@ -589,8 +589,6 @@  static int set_happy_link_modes(struct happy_meal *hp, void __iomem *tregs)
 	return 1;
 }
 
-static int happy_meal_init(struct happy_meal *hp);
-
 static int is_lucent_phy(struct happy_meal *hp)
 {
 	void __iomem *tregs = hp->tcvregs;
@@ -606,6 +604,124 @@  static int is_lucent_phy(struct happy_meal *hp)
 	return ret;
 }
 
+/* hp->happy_lock must be held */
+static void
+happy_meal_begin_auto_negotiation(struct happy_meal *hp,
+				  void __iomem *tregs,
+				  const struct ethtool_link_ksettings *ep)
+{
+	int timeout;
+
+	/* Read all of the registers we are interested in now. */
+	hp->sw_bmsr      = happy_meal_tcvr_read(hp, tregs, MII_BMSR);
+	hp->sw_bmcr      = happy_meal_tcvr_read(hp, tregs, MII_BMCR);
+	hp->sw_physid1   = happy_meal_tcvr_read(hp, tregs, MII_PHYSID1);
+	hp->sw_physid2   = happy_meal_tcvr_read(hp, tregs, MII_PHYSID2);
+
+	/* XXX Check BMSR_ANEGCAPABLE, should not be necessary though. */
+
+	hp->sw_advertise = happy_meal_tcvr_read(hp, tregs, MII_ADVERTISE);
+	if (!ep || ep->base.autoneg == AUTONEG_ENABLE) {
+		/* Advertise everything we can support. */
+		if (hp->sw_bmsr & BMSR_10HALF)
+			hp->sw_advertise |= (ADVERTISE_10HALF);
+		else
+			hp->sw_advertise &= ~(ADVERTISE_10HALF);
+
+		if (hp->sw_bmsr & BMSR_10FULL)
+			hp->sw_advertise |= (ADVERTISE_10FULL);
+		else
+			hp->sw_advertise &= ~(ADVERTISE_10FULL);
+		if (hp->sw_bmsr & BMSR_100HALF)
+			hp->sw_advertise |= (ADVERTISE_100HALF);
+		else
+			hp->sw_advertise &= ~(ADVERTISE_100HALF);
+		if (hp->sw_bmsr & BMSR_100FULL)
+			hp->sw_advertise |= (ADVERTISE_100FULL);
+		else
+			hp->sw_advertise &= ~(ADVERTISE_100FULL);
+		happy_meal_tcvr_write(hp, tregs, MII_ADVERTISE, hp->sw_advertise);
+
+		/* XXX Currently no Happy Meal cards I know off support 100BaseT4,
+		 * XXX and this is because the DP83840 does not support it, changes
+		 * XXX would need to be made to the tx/rx logic in the driver as well
+		 * XXX so I completely skip checking for it in the BMSR for now.
+		 */
+
+		ASD("Advertising [ %s%s%s%s]\n",
+		    hp->sw_advertise & ADVERTISE_10HALF ? "10H " : "",
+		    hp->sw_advertise & ADVERTISE_10FULL ? "10F " : "",
+		    hp->sw_advertise & ADVERTISE_100HALF ? "100H " : "",
+		    hp->sw_advertise & ADVERTISE_100FULL ? "100F " : "");
+
+		/* Enable Auto-Negotiation, this is usually on already... */
+		hp->sw_bmcr |= BMCR_ANENABLE;
+		happy_meal_tcvr_write(hp, tregs, MII_BMCR, hp->sw_bmcr);
+
+		/* Restart it to make sure it is going. */
+		hp->sw_bmcr |= BMCR_ANRESTART;
+		happy_meal_tcvr_write(hp, tregs, MII_BMCR, hp->sw_bmcr);
+
+		/* BMCR_ANRESTART self clears when the process has begun. */
+
+		timeout = 64;  /* More than enough. */
+		while (--timeout) {
+			hp->sw_bmcr = happy_meal_tcvr_read(hp, tregs, MII_BMCR);
+			if (!(hp->sw_bmcr & BMCR_ANRESTART))
+				break; /* got it. */
+			udelay(10);
+		}
+		if (!timeout) {
+			netdev_err(hp->dev,
+				   "Happy Meal would not start auto negotiation BMCR=0x%04x\n",
+				   hp->sw_bmcr);
+			netdev_notice(hp->dev,
+				      "Performing force link detection.\n");
+			goto force_link;
+		} else {
+			hp->timer_state = arbwait;
+		}
+	} else {
+force_link:
+		/* Force the link up, trying first a particular mode.
+		 * Either we are here at the request of ethtool or
+		 * because the Happy Meal would not start to autoneg.
+		 */
+
+		/* Disable auto-negotiation in BMCR, enable the duplex and
+		 * speed setting, init the timer state machine, and fire it off.
+		 */
+		if (!ep || ep->base.autoneg == AUTONEG_ENABLE) {
+			hp->sw_bmcr = BMCR_SPEED100;
+		} else {
+			if (ep->base.speed == SPEED_100)
+				hp->sw_bmcr = BMCR_SPEED100;
+			else
+				hp->sw_bmcr = 0;
+			if (ep->base.duplex == DUPLEX_FULL)
+				hp->sw_bmcr |= BMCR_FULLDPLX;
+		}
+		happy_meal_tcvr_write(hp, tregs, MII_BMCR, hp->sw_bmcr);
+
+		if (!is_lucent_phy(hp)) {
+			/* OK, seems we need do disable the transceiver for the first
+			 * tick to make sure we get an accurate link state at the
+			 * second tick.
+			 */
+			hp->sw_csconfig = happy_meal_tcvr_read(hp, tregs,
+							       DP83840_CSCONFIG);
+			hp->sw_csconfig &= ~(CSCONFIG_TCVDISAB);
+			happy_meal_tcvr_write(hp, tregs, DP83840_CSCONFIG,
+					      hp->sw_csconfig);
+		}
+		hp->timer_state = ltrywait;
+	}
+
+	hp->timer_ticks = 0;
+	hp->happy_timer.expires = jiffies + (12 * HZ)/10;  /* 1.2 sec. */
+	add_timer(&hp->happy_timer);
+}
+
 static void happy_meal_timer(struct timer_list *t)
 {
 	struct happy_meal *hp = from_timer(hp, t, happy_timer);
@@ -743,12 +859,7 @@  static void happy_meal_timer(struct timer_list *t)
 					netdev_notice(hp->dev,
 						      "Link down, cable problem?\n");
 
-					ret = happy_meal_init(hp);
-					if (ret) {
-						/* ho hum... */
-						netdev_err(hp->dev,
-							   "Error, cannot re-init the Happy Meal.\n");
-					}
+					happy_meal_begin_auto_negotiation(hp, tregs, NULL);
 					goto out;
 				}
 				if (!is_lucent_phy(hp)) {
@@ -1201,124 +1312,6 @@  static void happy_meal_init_rings(struct happy_meal *hp)
 	HMD("done\n");
 }
 
-/* hp->happy_lock must be held */
-static void
-happy_meal_begin_auto_negotiation(struct happy_meal *hp,
-				  void __iomem *tregs,
-				  const struct ethtool_link_ksettings *ep)
-{
-	int timeout;
-
-	/* Read all of the registers we are interested in now. */
-	hp->sw_bmsr      = happy_meal_tcvr_read(hp, tregs, MII_BMSR);
-	hp->sw_bmcr      = happy_meal_tcvr_read(hp, tregs, MII_BMCR);
-	hp->sw_physid1   = happy_meal_tcvr_read(hp, tregs, MII_PHYSID1);
-	hp->sw_physid2   = happy_meal_tcvr_read(hp, tregs, MII_PHYSID2);
-
-	/* XXX Check BMSR_ANEGCAPABLE, should not be necessary though. */
-
-	hp->sw_advertise = happy_meal_tcvr_read(hp, tregs, MII_ADVERTISE);
-	if (!ep || ep->base.autoneg == AUTONEG_ENABLE) {
-		/* Advertise everything we can support. */
-		if (hp->sw_bmsr & BMSR_10HALF)
-			hp->sw_advertise |= (ADVERTISE_10HALF);
-		else
-			hp->sw_advertise &= ~(ADVERTISE_10HALF);
-
-		if (hp->sw_bmsr & BMSR_10FULL)
-			hp->sw_advertise |= (ADVERTISE_10FULL);
-		else
-			hp->sw_advertise &= ~(ADVERTISE_10FULL);
-		if (hp->sw_bmsr & BMSR_100HALF)
-			hp->sw_advertise |= (ADVERTISE_100HALF);
-		else
-			hp->sw_advertise &= ~(ADVERTISE_100HALF);
-		if (hp->sw_bmsr & BMSR_100FULL)
-			hp->sw_advertise |= (ADVERTISE_100FULL);
-		else
-			hp->sw_advertise &= ~(ADVERTISE_100FULL);
-		happy_meal_tcvr_write(hp, tregs, MII_ADVERTISE, hp->sw_advertise);
-
-		/* XXX Currently no Happy Meal cards I know off support 100BaseT4,
-		 * XXX and this is because the DP83840 does not support it, changes
-		 * XXX would need to be made to the tx/rx logic in the driver as well
-		 * XXX so I completely skip checking for it in the BMSR for now.
-		 */
-
-		ASD("Advertising [ %s%s%s%s]\n",
-		    hp->sw_advertise & ADVERTISE_10HALF ? "10H " : "",
-		    hp->sw_advertise & ADVERTISE_10FULL ? "10F " : "",
-		    hp->sw_advertise & ADVERTISE_100HALF ? "100H " : "",
-		    hp->sw_advertise & ADVERTISE_100FULL ? "100F " : "");
-
-		/* Enable Auto-Negotiation, this is usually on already... */
-		hp->sw_bmcr |= BMCR_ANENABLE;
-		happy_meal_tcvr_write(hp, tregs, MII_BMCR, hp->sw_bmcr);
-
-		/* Restart it to make sure it is going. */
-		hp->sw_bmcr |= BMCR_ANRESTART;
-		happy_meal_tcvr_write(hp, tregs, MII_BMCR, hp->sw_bmcr);
-
-		/* BMCR_ANRESTART self clears when the process has begun. */
-
-		timeout = 64;  /* More than enough. */
-		while (--timeout) {
-			hp->sw_bmcr = happy_meal_tcvr_read(hp, tregs, MII_BMCR);
-			if (!(hp->sw_bmcr & BMCR_ANRESTART))
-				break; /* got it. */
-			udelay(10);
-		}
-		if (!timeout) {
-			netdev_err(hp->dev,
-				   "Happy Meal would not start auto negotiation BMCR=0x%04x\n",
-				   hp->sw_bmcr);
-			netdev_notice(hp->dev,
-				      "Performing force link detection.\n");
-			goto force_link;
-		} else {
-			hp->timer_state = arbwait;
-		}
-	} else {
-force_link:
-		/* Force the link up, trying first a particular mode.
-		 * Either we are here at the request of ethtool or
-		 * because the Happy Meal would not start to autoneg.
-		 */
-
-		/* Disable auto-negotiation in BMCR, enable the duplex and
-		 * speed setting, init the timer state machine, and fire it off.
-		 */
-		if (!ep || ep->base.autoneg == AUTONEG_ENABLE) {
-			hp->sw_bmcr = BMCR_SPEED100;
-		} else {
-			if (ep->base.speed == SPEED_100)
-				hp->sw_bmcr = BMCR_SPEED100;
-			else
-				hp->sw_bmcr = 0;
-			if (ep->base.duplex == DUPLEX_FULL)
-				hp->sw_bmcr |= BMCR_FULLDPLX;
-		}
-		happy_meal_tcvr_write(hp, tregs, MII_BMCR, hp->sw_bmcr);
-
-		if (!is_lucent_phy(hp)) {
-			/* OK, seems we need do disable the transceiver for the first
-			 * tick to make sure we get an accurate link state at the
-			 * second tick.
-			 */
-			hp->sw_csconfig = happy_meal_tcvr_read(hp, tregs,
-							       DP83840_CSCONFIG);
-			hp->sw_csconfig &= ~(CSCONFIG_TCVDISAB);
-			happy_meal_tcvr_write(hp, tregs, DP83840_CSCONFIG,
-					      hp->sw_csconfig);
-		}
-		hp->timer_state = ltrywait;
-	}
-
-	hp->timer_ticks = 0;
-	hp->happy_timer.expires = jiffies + (12 * HZ)/10;  /* 1.2 sec. */
-	add_timer(&hp->happy_timer);
-}
-
 /* hp->happy_lock must be held */
 static int happy_meal_init(struct happy_meal *hp)
 {