Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 1 Sep 2012 11:35:31 +0200 (CEST)
From:      Dan Lukes <dan@obluda.cz>
To:        FreeBSD-gnats-submit@FreeBSD.org
Subject:   kern/171228: [ patch ] if_re - eeprom write issues
Message-ID:  <201209010935.q819ZVie084355@kgw.obluda.cz>
Resent-Message-ID: <201209011000.q81A0LBl080455@freefall.freebsd.org>

next in thread | raw e-mail | index | archive | help

>Number:         171228
>Category:       kern
>Synopsis:       [ patch ] if_re - eeprom write issues
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Sat Sep 01 10:00:20 UTC 2012
>Closed-Date:
>Last-Modified:
>Originator:     Dan Lukes
>Release:        FreeBSD 9.1-RC1 i386
>Organization:
Obludarium
>Environment:

Originally found on:
System: FreeBSD 9.1-RC1 i386
sys/dev/re/if_re.c,v 1.198.2.14 2012/07/02 19:56:31

verified to apply to HEAD & if_re.c 1.224 as well

>Description:

Realtek network card driver, if_re.c:

1. re_clrwol()
   CFG5 register writen after config register write done claimed. Yes, some documentation show that it's not necesarry to be in such mode to write CFG5, but it's unclear it apply to all chip versions.
   See SVN rev 232145 comment also. 
   It's good to be consistent in all parts of code. All other writes to CFG5 register in module are done in "EEPROM write enable" mode but this one in re_clrwol() (see re_setwol and re_attach routines)

2. unecesarry writes into EEPROM (e.g. write of data althougth unchanged)
  There are several places, where config register is read, examined, sometime) modified, then written back into EEPROM. 
  Unfortunately, they are written back unconditionally, whenever they has been modified or not. 
  As EEPROM have limited number of write cycles, such behavior may short lifetime of network card

>How-To-Repeat:
	see description
>Fix:
[1] - move CFG5 write before "EEPROM write mode" close. It's better to be on safe side and it doesn't harm anything even it's not necesarry for particular chip
[2] - store original value of configuration register read, write back only if changed


--- patch-re begins here ---
--- sys/dev/re/if_re.c.orig	2012-07-02 21:56:31.000000000 +0200
+++ sys/dev/re/if_re.c	2012-09-01 10:01:33.000000000 +0200
@@ -1194,7 +1194,7 @@
 	u_int16_t		devid, re_did = 0;
 	int			error = 0, i, phy, rid;
 	int			msic, msixc, reg;
-	uint8_t			cfg;
+	uint8_t			ocfg, cfg;
 
 	sc = device_get_softc(dev);
 	sc->rl_dev = dev;
@@ -1294,9 +1294,10 @@
 				sc->rl_flags |= RL_FLAG_MSI;
 				/* Explicitly set MSI enable bit. */
 				CSR_WRITE_1(sc, RL_EECMD, RL_EE_MODE);
-				cfg = CSR_READ_1(sc, RL_CFG2);
+				ocfg = cfg = CSR_READ_1(sc, RL_CFG2);
 				cfg |= RL_CFG2_MSI;
-				CSR_WRITE_1(sc, RL_CFG2, cfg);
+				if (cfg != ocfg)
+					CSR_WRITE_1(sc, RL_CFG2, cfg);
 				CSR_WRITE_1(sc, RL_EECMD, RL_EEMODE_OFF);
 			} else
 				pci_release_msi(dev);
@@ -1500,12 +1501,14 @@
 
 	/* Enable PME. */
 	CSR_WRITE_1(sc, RL_EECMD, RL_EE_MODE);
-	cfg = CSR_READ_1(sc, sc->rl_cfg1);
+	ocfg = cfg = CSR_READ_1(sc, sc->rl_cfg1);
 	cfg |= RL_CFG1_PME;
-	CSR_WRITE_1(sc, sc->rl_cfg1, cfg);
-	cfg = CSR_READ_1(sc, sc->rl_cfg5);
+	if (ocfg != cfg)
+		CSR_WRITE_1(sc, sc->rl_cfg1, cfg);
+	ocfg = cfg = CSR_READ_1(sc, sc->rl_cfg5);
 	cfg &= RL_CFG5_PME_STS;
-	CSR_WRITE_1(sc, sc->rl_cfg5, cfg);
+	if (ocfg != cfg)
+		CSR_WRITE_1(sc, sc->rl_cfg5, cfg);
 	CSR_WRITE_1(sc, RL_EECMD, RL_EEMODE_OFF);
 
 	if ((sc->rl_flags & RL_FLAG_PAR) != 0) {
@@ -3780,7 +3783,7 @@
 	struct ifnet		*ifp;
 	int			pmc;
 	uint16_t		pmstat;
-	uint8_t			v;
+	uint8_t			ov, v;
 
 	RL_LOCK_ASSERT(sc);
 
@@ -3805,19 +3808,21 @@
 	CSR_WRITE_1(sc, RL_EECMD, RL_EE_MODE);
 
 	/* Enable PME. */
-	v = CSR_READ_1(sc, sc->rl_cfg1);
+	ov = v = CSR_READ_1(sc, sc->rl_cfg1);
 	v &= ~RL_CFG1_PME;
 	if ((ifp->if_capenable & IFCAP_WOL) != 0)
 		v |= RL_CFG1_PME;
-	CSR_WRITE_1(sc, sc->rl_cfg1, v);
+	if (ov != v)
+		CSR_WRITE_1(sc, sc->rl_cfg1, v);
 
-	v = CSR_READ_1(sc, sc->rl_cfg3);
+	ov = v = CSR_READ_1(sc, sc->rl_cfg3);
 	v &= ~(RL_CFG3_WOL_LINK | RL_CFG3_WOL_MAGIC);
 	if ((ifp->if_capenable & IFCAP_WOL_MAGIC) != 0)
 		v |= RL_CFG3_WOL_MAGIC;
-	CSR_WRITE_1(sc, sc->rl_cfg3, v);
+	if (ov != v)
+		CSR_WRITE_1(sc, sc->rl_cfg3, v);
 
-	v = CSR_READ_1(sc, sc->rl_cfg5);
+	ov = v = CSR_READ_1(sc, sc->rl_cfg5);
 	v &= ~(RL_CFG5_WOL_BCAST | RL_CFG5_WOL_MCAST | RL_CFG5_WOL_UCAST |
 	    RL_CFG5_WOL_LANWAKE);
 	if ((ifp->if_capenable & IFCAP_WOL_UCAST) != 0)
@@ -3826,7 +3831,8 @@
 		v |= RL_CFG5_WOL_MCAST | RL_CFG5_WOL_BCAST;
 	if ((ifp->if_capenable & IFCAP_WOL) != 0)
 		v |= RL_CFG5_WOL_LANWAKE;
-	CSR_WRITE_1(sc, sc->rl_cfg5, v);
+	if (ov != v)
+		CSR_WRITE_1(sc, sc->rl_cfg5, v);
 
 	/* Config register write done. */
 	CSR_WRITE_1(sc, RL_EECMD, RL_EEMODE_OFF);
@@ -3852,7 +3858,7 @@
 re_clrwol(struct rl_softc *sc)
 {
 	int			pmc;
-	uint8_t			v;
+	uint8_t			ov, v;
 
 	RL_LOCK_ASSERT(sc);
 
@@ -3862,17 +3868,19 @@
 	/* Enable config register write. */
 	CSR_WRITE_1(sc, RL_EECMD, RL_EE_MODE);
 
-	v = CSR_READ_1(sc, sc->rl_cfg3);
+	ov = v = CSR_READ_1(sc, sc->rl_cfg3);
 	v &= ~(RL_CFG3_WOL_LINK | RL_CFG3_WOL_MAGIC);
-	CSR_WRITE_1(sc, sc->rl_cfg3, v);
+	if (ov != v)
+		CSR_WRITE_1(sc, sc->rl_cfg3, v);
 
-	/* Config register write done. */
-	CSR_WRITE_1(sc, RL_EECMD, RL_EEMODE_OFF);
-
-	v = CSR_READ_1(sc, sc->rl_cfg5);
+	ov = v = CSR_READ_1(sc, sc->rl_cfg5);
 	v &= ~(RL_CFG5_WOL_BCAST | RL_CFG5_WOL_MCAST | RL_CFG5_WOL_UCAST);
 	v &= ~RL_CFG5_WOL_LANWAKE;
-	CSR_WRITE_1(sc, sc->rl_cfg5, v);
+	if (ov != v)
+		CSR_WRITE_1(sc, sc->rl_cfg5, v);
+
+	/* Config register write done. */
+	CSR_WRITE_1(sc, RL_EECMD, RL_EEMODE_OFF);
 }
 
 static void
--- patch-re ends here ---
>Release-Note:
>Audit-Trail:
>Unformatted:



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201209010935.q819ZVie084355>