[PATCH 0/2] Pull request for 'r8169-fixes' branch

[PATCH 0/2] Pull request for 'r8169-fixes' branch

[Francois Romieu]

Please pull from branch 'r8169-fixes' in repository

git://git.kernel.org/pub/scm/linux/kernel/git/romieu/netdev-2.6.git r8169-fixes

to get the changes below.

Distance from 'davem-fixes' (8fa0b315fc0c1a414da1371f1fc39523a657c192)
----------------------------------------------------------------------

4f2e025bf9881b3dfcddbebd30c3ad65a748c296
18eb1b8ec3970595a0081770f6b5e134aaa07ba3

Diffstat
--------

 drivers/net/r8169.c |   38 ++++++++++++++++++++++++++++++--------
 1 files changed, 30 insertions(+), 8 deletions(-)

Shortlog
--------

Francois Romieu (2):
      r8169: verbose mac address init
      r8169: checks against wrong mac addresse init

Patch
-----

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index c821da2..2b4e975 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -81,6 +81,10 @@ static const int multicast_filter_limit = 32;
 #define RTL8169_TX_TIMEOUT	(6*HZ)
 #define RTL8169_PHY_TIMEOUT	(10*HZ)
 
+#define RTL_EEPROM_SIG		cpu_to_le32(0x8129)
+#define RTL_EEPROM_SIG_MASK	cpu_to_le32(0xffff)
+#define RTL_EEPROM_SIG_ADDR	0x0000
+
 /* write/read MMIO register */
 #define RTL_W8(reg, val8)	writeb ((val8), ioaddr + (reg))
 #define RTL_W16(reg, val16)	writew ((val16), ioaddr + (reg))
@@ -1944,14 +1948,15 @@ static void rtl_init_mac_address(struct rtl8169_private *tp,
 				 void __iomem *ioaddr)
 {
 	struct pci_dev *pdev = tp->pci_dev;
-	u8 cfg1;
 	int vpd_cap;
+	__le32 sig;
 	u8 mac[8];
-	DECLARE_MAC_BUF(buf);
+	u8 cfg1;
 
 	cfg1 = RTL_R8(Config1);
 	if (!(cfg1  & VPD)) {
-		dprintk("VPD access not enabled, enabling\n");
+		if (netif_msg_probe(tp))
+			dev_info(&pdev->dev, "VPD access disabled, enabling\n");
 		RTL_W8(Cfg9346, Cfg9346_Unlock);
 		RTL_W8(Config1, cfg1 | VPD);
 		RTL_W8(Cfg9346, Cfg9346_Lock);
@@ -1961,7 +1966,16 @@ static void rtl_init_mac_address(struct rtl8169_private *tp,
 	if (!vpd_cap)
 		return;
 
-	/* MAC address is stored in EEPROM at offset 0x0e
+	if (rtl_eeprom_read(pdev, vpd_cap, RTL_EEPROM_SIG_ADDR, &sig) < 0)
+		return;
+
+	if ((sig & RTL_EEPROM_SIG_MASK) != RTL_EEPROM_SIG) {
+		dev_info(&pdev->dev, "Missing EEPROM signature: %08x\n", sig);
+		return;
+	}
+
+	/*
+	 * MAC address is stored in EEPROM at offset 0x0e
 	 * Realtek says: "The VPD address does not have to be a DWORD-aligned
 	 * address as defined in the PCI 2.2 Specifications, but the VPD data
 	 * is always consecutive 4-byte data starting from the VPD address
@@ -1969,14 +1983,22 @@ static void rtl_init_mac_address(struct rtl8169_private *tp,
 	 */
 	if (rtl_eeprom_read(pdev, vpd_cap, 0x000e, (__le32*)&mac[0]) < 0 ||
 	    rtl_eeprom_read(pdev, vpd_cap, 0x0012, (__le32*)&mac[4]) < 0) {
-		dprintk("Reading MAC address from EEPROM failed\n");
+		if (netif_msg_probe(tp)) {
+			dev_warn(&pdev->dev,
+				 "reading MAC address from EEPROM failed\n");
+		}
 		return;
 	}
 
-	dprintk("MAC address found in EEPROM: %s\n", print_mac(buf, mac));
+	if (netif_msg_probe(tp)) {
+		DECLARE_MAC_BUF(buf);
+
+		dev_info(&pdev->dev, "MAC address found in EEPROM: %s\n",
+			 print_mac(buf, mac));
+	}
 
-	/* Write MAC address */
-	rtl_rar_set(tp, mac);
+	if (is_valid_ether_addr(mac))
+		rtl_rar_set(tp, mac);
 }
 
 static int __devinit
-- 
Ueimor

[PATCH 1/2] r8169: verbose mac address init

[Francois Romieu]

I prefer the debug information to be displayed until
the issue is properly handled.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
Cc: Edward Hsu <edward_hsu@realtek.com.tw>
---
 drivers/net/r8169.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index c821da2..cd9a215 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -1947,11 +1947,11 @@ static void rtl_init_mac_address(struct rtl8169_private *tp,
 	u8 cfg1;
 	int vpd_cap;
 	u8 mac[8];
-	DECLARE_MAC_BUF(buf);
 
 	cfg1 = RTL_R8(Config1);
 	if (!(cfg1  & VPD)) {
-		dprintk("VPD access not enabled, enabling\n");
+		if (netif_msg_probe(tp))
+			dev_info(&pdev->dev, "VPD access disabled, enabling\n");
 		RTL_W8(Cfg9346, Cfg9346_Unlock);
 		RTL_W8(Config1, cfg1 | VPD);
 		RTL_W8(Cfg9346, Cfg9346_Lock);
@@ -1969,11 +1969,19 @@ static void rtl_init_mac_address(struct rtl8169_private *tp,
 	 */
 	if (rtl_eeprom_read(pdev, vpd_cap, 0x000e, (__le32*)&mac[0]) < 0 ||
 	    rtl_eeprom_read(pdev, vpd_cap, 0x0012, (__le32*)&mac[4]) < 0) {
-		dprintk("Reading MAC address from EEPROM failed\n");
+		if (netif_msg_probe(tp)) {
+			dev_warn(&pdev->dev,
+				 "reading MAC address from EEPROM failed\n");
+		}
 		return;
 	}
 
-	dprintk("MAC address found in EEPROM: %s\n", print_mac(buf, mac));
+	if (netif_msg_probe(tp)) {
+		DECLARE_MAC_BUF(buf);
+
+		dev_info(&pdev->dev, "MAC address found in EEPROM: %s\n",
+			 print_mac(buf, mac));
+	}
 
 	/* Write MAC address */
 	rtl_rar_set(tp, mac);
-- 
1.5.3.3

[PATCH 2/2] r8169: checks against wrong mac addresse init

[Francois Romieu]

Checking the signature of the eeprom and the validity of the
MAC address should be enough to filter out the bad addresses
observed so far.

Contributed by Ivan Vecera and Martin Capitanio.

Tested on 8102el, 8168b and 8169 for a start.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
Cc: Edward Hsu <edward_hsu@realtek.com.tw>
---
 drivers/net/r8169.c |   22 ++++++++++++++++++----
 1 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index cd9a215..2b4e975 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -81,6 +81,10 @@ static const int multicast_filter_limit = 32;
 #define RTL8169_TX_TIMEOUT	(6*HZ)
 #define RTL8169_PHY_TIMEOUT	(10*HZ)
 
+#define RTL_EEPROM_SIG		cpu_to_le32(0x8129)
+#define RTL_EEPROM_SIG_MASK	cpu_to_le32(0xffff)
+#define RTL_EEPROM_SIG_ADDR	0x0000
+
 /* write/read MMIO register */
 #define RTL_W8(reg, val8)	writeb ((val8), ioaddr + (reg))
 #define RTL_W16(reg, val16)	writew ((val16), ioaddr + (reg))
@@ -1944,9 +1948,10 @@ static void rtl_init_mac_address(struct rtl8169_private *tp,
 				 void __iomem *ioaddr)
 {
 	struct pci_dev *pdev = tp->pci_dev;
-	u8 cfg1;
 	int vpd_cap;
+	__le32 sig;
 	u8 mac[8];
+	u8 cfg1;
 
 	cfg1 = RTL_R8(Config1);
 	if (!(cfg1  & VPD)) {
@@ -1961,7 +1966,16 @@ static void rtl_init_mac_address(struct rtl8169_private *tp,
 	if (!vpd_cap)
 		return;
 
-	/* MAC address is stored in EEPROM at offset 0x0e
+	if (rtl_eeprom_read(pdev, vpd_cap, RTL_EEPROM_SIG_ADDR, &sig) < 0)
+		return;
+
+	if ((sig & RTL_EEPROM_SIG_MASK) != RTL_EEPROM_SIG) {
+		dev_info(&pdev->dev, "Missing EEPROM signature: %08x\n", sig);
+		return;
+	}
+
+	/*
+	 * MAC address is stored in EEPROM at offset 0x0e
 	 * Realtek says: "The VPD address does not have to be a DWORD-aligned
 	 * address as defined in the PCI 2.2 Specifications, but the VPD data
 	 * is always consecutive 4-byte data starting from the VPD address
@@ -1983,8 +1997,8 @@ static void rtl_init_mac_address(struct rtl8169_private *tp,
 			 print_mac(buf, mac));
 	}
 
-	/* Write MAC address */
-	rtl_rar_set(tp, mac);
+	if (is_valid_ether_addr(mac))
+		rtl_rar_set(tp, mac);
 }
 
 static int __devinit
-- 
1.5.3.3

Re: [PATCH 2/2] r8169: checks against wrong mac addresse init

[Martin Capitanio]

On Thu, 2008-10-16 at 23:48 +0200, Francois Romieu wrote:
> Checking the signature of the eeprom and the validity of the
> MAC address should be enough to filter out the bad addresses
> observed so far.
> 
> Contributed by Ivan Vecera and Martin Capitanio.

-Tested on 8102el, 8168b and 8169 for a start.
+Tested on 8102e, 8168b and 8169 for a start.
XID 34a00000

Please take a look at the realtek r8101_n aka RealTek RTL8101E,
RTL8102E(L) code. Only CFG_METHOD_1, CFG_METHOD_2
and #(ioaddr, 0x00) == 0x8128 are here allowed to EEPROM access.

Thanks,
Martin


static void 
rtl8101_get_mac_version(struct rtl8101_private *tp, 
			void __iomem *ioaddr)
{
	u32 reg,val32;
	u32 ICVerID;
		
	val32 = RTL_R32(TxConfig);	
	reg = val32 & 0x7c800000;
	ICVerID = val32 & 0x00700000;

	switch (reg) {
		case 0x34000000:
			if (ICVerID == 0x00000000)
				tp->mcfg = CFG_METHOD_1;
			else if (ICVerID == 0x00200000)
				tp->mcfg = CFG_METHOD_2;
			else if (ICVerID == 0x00300000)
				tp->mcfg = CFG_METHOD_6;
			else
				tp->mcfg = CFG_METHOD_6;
			break;
		case 0x34800000: //RTL8102E
		case 0x24800000: //RTL8102EL
			if (ICVerID == 0x00000000)
				tp->mcfg = CFG_METHOD_3;
			else if (ICVerID == 0x00100000)
				tp->mcfg = CFG_METHOD_4;
			else if (ICVerID == 0x00200000)
				tp->mcfg = CFG_METHOD_5;
			else
				tp->mcfg = CFG_METHOD_5;
			break;
		default:
			tp->mcfg = 0;
			printk("unknown chip version (%x)\n",reg);
			break;
	}

}

static void
rtl8101_check_eeprom(struct rtl8101_private *tp)
{
	void __iomem *ioaddr = tp->mmio_addr;
	u32 mac_addr_1 = 0;
	u32 mac_addr_2 = 0;

	if ((tp->mcfg != CFG_METHOD_1) && (tp->mcfg != CFG_METHOD_2))
		return;

	if (rtl_eeprom_read_sc(ioaddr, 0x00) != 0x8128)
		return;

	RTL_W8(Cfg9346, Cfg9346_Unlock);
	RTL_W8(Config5, RTL_R8(Config5) & ~UWF);
	RTL_W8(Cfg9346, Cfg9346_Lock);

	rtl_eeprom_write_sc(ioaddr, 0x00, 0x8129);

	RTL_W8(Cfg9346, Cfg9346_EEM0);
	mdelay(15);
	rtl_eeprom_write_sc(ioaddr, 0x00, 0x8128);

	mac_addr_1 = rtl_eeprom_read_sc(ioaddr, 0x08);
	mac_addr_1 = mac_addr_1 << 16;

	mac_addr_2 = rtl_eeprom_read_sc(ioaddr, 0x07);
	mac_addr_1 |= mac_addr_2;

	RTL_W8(Cfg9346, Cfg9346_Unlock);
	RTL_W32(MAC0, mac_addr_1);
	RTL_W8(Cfg9346, Cfg9346_Lock);

	mac_addr_1 = 0;
	mac_addr_1 = rtl_eeprom_read_sc(ioaddr, 0x09);

	RTL_W8(Cfg9346, Cfg9346_Unlock);
	RTL_W32(MAC4, mac_addr_1);
	RTL_W8(Cfg9346, Cfg9346_Lock);
}



> 
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
> Cc: Edward Hsu <edward_hsu@realtek.com.tw>
> ---
>  drivers/net/r8169.c |   22 ++++++++++++++++++----
>  1 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> index cd9a215..2b4e975 100644
> --- a/drivers/net/r8169.c
> +++ b/drivers/net/r8169.c
> @@ -81,6 +81,10 @@ static const int multicast_filter_limit = 32;
>  #define RTL8169_TX_TIMEOUT	(6*HZ)
>  #define RTL8169_PHY_TIMEOUT	(10*HZ)
>  
> +#define RTL_EEPROM_SIG		cpu_to_le32(0x8129)
> +#define RTL_EEPROM_SIG_MASK	cpu_to_le32(0xffff)
> +#define RTL_EEPROM_SIG_ADDR	0x0000
> +
>  /* write/read MMIO register */
>  #define RTL_W8(reg, val8)	writeb ((val8), ioaddr + (reg))
>  #define RTL_W16(reg, val16)	writew ((val16), ioaddr + (reg))
> @@ -1944,9 +1948,10 @@ static void rtl_init_mac_address(struct rtl8169_private *tp,
>  				 void __iomem *ioaddr)
>  {
>  	struct pci_dev *pdev = tp->pci_dev;
> -	u8 cfg1;
>  	int vpd_cap;
> +	__le32 sig;
>  	u8 mac[8];
> +	u8 cfg1;
>  
>  	cfg1 = RTL_R8(Config1);
>  	if (!(cfg1  & VPD)) {
> @@ -1961,7 +1966,16 @@ static void rtl_init_mac_address(struct rtl8169_private *tp,
>  	if (!vpd_cap)
>  		return;
>  
> -	/* MAC address is stored in EEPROM at offset 0x0e
> +	if (rtl_eeprom_read(pdev, vpd_cap, RTL_EEPROM_SIG_ADDR, &sig) < 0)
> +		return;
> +
> +	if ((sig & RTL_EEPROM_SIG_MASK) != RTL_EEPROM_SIG) {
> +		dev_info(&pdev->dev, "Missing EEPROM signature: %08x\n", sig);
> +		return;
> +	}
> +
> +	/*
> +	 * MAC address is stored in EEPROM at offset 0x0e
>  	 * Realtek says: "The VPD address does not have to be a DWORD-aligned
>  	 * address as defined in the PCI 2.2 Specifications, but the VPD data
>  	 * is always consecutive 4-byte data starting from the VPD address
> @@ -1983,8 +1997,8 @@ static void rtl_init_mac_address(struct rtl8169_private *tp,
>  			 print_mac(buf, mac));
>  	}
>  
> -	/* Write MAC address */
> -	rtl_rar_set(tp, mac);
> +	if (is_valid_ether_addr(mac))
> +		rtl_rar_set(tp, mac);
>  }
>  
>  static int __devinit

Re: [PATCH 2/2] r8169: checks against wrong mac addresse init

[Francois Romieu]

Martin Capitanio <c4p7n@capitanio.org> :
[...]
> Please take a look at the realtek r8101_n aka RealTek RTL8101E,
> RTL8102E(L) code. Only CFG_METHOD_1, CFG_METHOD_2
> and #(ioaddr, 0x00) == 0x8128 are here allowed to EEPROM access.

So Realtek's 810x driver eeprom code targets the 8101Eb and the
8101Ec only. It does not include Ivan's 8102el.

[...]
> static void
> rtl8101_check_eeprom(struct rtl8101_private *tp)
> {
[...]
> 	mac_addr_1 = rtl_eeprom_read_sc(ioaddr, 0x08);
> 	mac_addr_1 = mac_addr_1 << 16;
> 
> 	mac_addr_2 = rtl_eeprom_read_sc(ioaddr, 0x07);
> 	mac_addr_1 |= mac_addr_2;

The mac address is at the usual place.

-- 
Ueimor

Re: [PATCH 2/2] r8169: checks against wrong mac addresse init

[Martin Capitanio]

On Fri, 2008-10-17 at 22:01 +0200, Francois Romieu wrote:
> Martin Capitanio <c4p7n@capitanio.org> :
> [...]
> > Please take a look at the realtek r8101_n aka RealTek RTL8101E,
> > RTL8102E(L) code. Only CFG_METHOD_1, CFG_METHOD_2
> > and #(ioaddr, 0x00) == 0x8128 are here allowed to EEPROM access.
> 
> So Realtek's 810x driver eeprom code targets the 8101Eb and the
> 8101Ec only. It does not include Ivan's 8102el.

So it seems ;)
RTL_W8(Cfg9346, Cfg9346_AMMO);
        mdelay(15);

and here they arm/unarm vpd:

static int
rtl8169_eeprom_read(struct net_device *dev, u32 *eeprom_cont, int eeprom_size)
{
	struct rtl8169_private *tp = netdev_priv(dev);
	void __iomem *ioaddr = tp->mmio_addr;
	int i;
	unsigned int read_addr;

	RTL_W8(Cfg9346, Cfg9346_Unlock);
	RTL_W8(Config1, RTL_R8(Config1) | VPDEnable);
	RTL_W8(Cfg9346, Cfg9346_Lock);

	for (i = 0, read_addr = 0; i < eeprom_size / 4; i++)
		*(eeprom_cont + i) = rtl8169_vpd_read(dev, read_addr + i * 4);

	RTL_W8(Cfg9346, Cfg9346_Unlock);
	RTL_W8(Config1, RTL_R8(Config1) & ~VPDEnable);
	RTL_W8(Cfg9346, Cfg9346_Lock);

	return 0;
}

Re: [PATCH 2/2] r8169: checks against wrong mac addresse init

[Ivan Vecera]

Martin Capitanio wrote:
> Please take a look at the realtek r8101_n aka RealTek RTL8101E,
> RTL8102E(L) code. Only CFG_METHOD_1, CFG_METHOD_2
> and #(ioaddr, 0x00) == 0x8128 are here allowed to EEPROM access.
> 
> ...
> 	rtl_eeprom_write_sc(ioaddr, 0x00, 0x8129);
> 
> 	RTL_W8(Cfg9346, Cfg9346_EEM0);
> 	mdelay(15);
> 	rtl_eeprom_write_sc(ioaddr, 0x00, 0x8128);

1) According specification when EEM0 is set to 1 and EEM1 is set to 0
then adapter enters "auto load" mode. Entering this mode will make the
adapter load the contents of the 93C46 (93C56) as when the PCI RSTB
signal is asserted. This auto-load operation will take about 2 ms.
Upon completion, the it automatically returns to normal mode
(EEM1 = EEM0 = 0) and all of the other registers are reset to default
values.
2) First 2 bytes of the EEPROM contain ID code words for the adapter.
It will load the contents of the EEPROM into the corresponding location
if the ID word (0x8129) is correct.

So the Realtek's driver at first sets these bytes to value 0x8129 to ensure
that auto-load will be a success. Then it initiates auto-load (EMM0 = 1 and
EMM1 = 0) then wait some time and finally writes back original value (0x8128).

Ivan

Re: [PATCH 2/2] r8169: checks against wrong mac addresse init

[Martin Capitanio]

On Tue, 2008-10-21 at 19:10 +0200, Ivan Vecera wrote:
Martin Capitanio wrote:
> > Please take a look at the realtek r8101_n aka RealTek RTL8101E,
> > RTL8102E(L) code. Only CFG_METHOD_1, CFG_METHOD_2
> > and #(ioaddr, 0x00) == 0x8128 are here allowed to EEPROM access.
> > 
> > ...
> > 	rtl_eeprom_write_sc(ioaddr, 0x00, 0x8129);
> > 
> > 	RTL_W8(Cfg9346, Cfg9346_EEM0);
> > 	mdelay(15);
> > 	rtl_eeprom_write_sc(ioaddr, 0x00, 0x8128);
> 
> 1) According specification when EEM0 is set to 1 and EEM1 is set to 0
> then adapter enters "auto load" mode. Entering this mode will make the
> adapter load the contents of the 93C46 (93C56) as when the PCI RSTB
> signal is asserted. This auto-load operation will take about 2 ms.
> Upon completion, the it automatically returns to normal mode
> (EEM1 = EEM0 = 0) and all of the other registers are reset to default
> values.
> 2) First 2 bytes of the EEPROM contain ID code words for the adapter.
> It will load the contents of the EEPROM into the corresponding location
> if the ID word (0x8129) is correct.
> 
> So the Realtek's driver at first sets these bytes to value 0x8129 to ensure
> that auto-load will be a success. Then it initiates auto-load (EMM0 = 1 and
> EMM1 = 0) then wait some time and finally writes back original value (0x8128).
>

My current working hypothesis is, that some of the devices due a hw bug
doesn't like a auto-load on the 'pci-hardwired-reset'. So they have the
value (0x8128).

Second thing is, that thru vpd you have full r/w
access to the EEPROM and the difference between read and write
is just flipping 1 bit. Although my brain refuses to parse the related
linux's pci core code, I think the access should be hw disabled
at the driver start and in the read case as soon as possible
to keep the window for the devil tinier ( http://lwn.net/Articles/303390/ ),
i.e. the realtek's code:

	RTL_W8(Cfg9346, Cfg9346_Unlock);
        RTL_W8(Config1, RTL_R8(Config1) | VPDEnable);
        RTL_W8(Cfg9346, Cfg9346_Lock);

        for (i = 0, read_addr = 0; i < eeprom_size / 4; i++)
                *(eeprom_cont + i) = rtl8169_vpd_read(dev, read_addr + i * 4);

        RTL_W8(Cfg9346, Cfg9346_Unlock);
        RTL_W8(Config1, RTL_R8(Config1) & ~VPDEnable);
        RTL_W8(Cfg9346, Cfg9346_Lock);

Martin

Re: [PATCH 0/2] Pull request for 'r8169-fixes' branch

[Jeff Garzik]

Francois Romieu wrote:
> Please pull from branch 'r8169-fixes' in repository
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/romieu/netdev-2.6.git r8169-fixes

pulled