[PATCH 1/1] r8169: revert "read MAC address from EEPROM on init"

[PATCH 1/1] r8169: revert "read MAC address from EEPROM on init"

[Francois Romieu]

This reverts commit 7bf6bf4803df1adc927f585168d2135fb019c698.

The code has both a short existence and an increasing track
of failures despite some work to amend it for -rc1. It is not
just a matter of reading the eeprom: sometimes the eeprom is
read correctly, then the mac address is not written correctly
back into the mac registers.

Some chipsets seem to work reliably but it is not clear at
this point if the code can simply be made to work on a
per-chipset basis and post -rc1 is not the place where I want
to experiment these things.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/r8169.c |   88 ---------------------------------------------------
 1 files changed, 0 insertions(+), 88 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 2b4e975..4b7cb38 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -1915,92 +1915,6 @@ static void rtl_disable_msi(struct pci_dev *pdev, struct rtl8169_private *tp)
 	}
 }
 
-static int rtl_eeprom_read(struct pci_dev *pdev, int cap, int addr, __le32 *val)
-{
-	int ret, count = 100;
-	u16 status = 0;
-	u32 value;
-
-	ret = pci_write_config_word(pdev, cap + PCI_VPD_ADDR, addr);
-	if (ret < 0)
-		return ret;
-
-	do {
-		udelay(10);
-		ret = pci_read_config_word(pdev, cap + PCI_VPD_ADDR, &status);
-		if (ret < 0)
-			return ret;
-	} while (!(status & PCI_VPD_ADDR_F) && --count);
-
-	if (!(status & PCI_VPD_ADDR_F))
-		return -ETIMEDOUT;
-
-	ret = pci_read_config_dword(pdev, cap + PCI_VPD_DATA, &value);
-	if (ret < 0)
-		return ret;
-
-	*val = cpu_to_le32(value);
-
-	return 0;
-}
-
-static void rtl_init_mac_address(struct rtl8169_private *tp,
-				 void __iomem *ioaddr)
-{
-	struct pci_dev *pdev = tp->pci_dev;
-	int vpd_cap;
-	__le32 sig;
-	u8 mac[8];
-	u8 cfg1;
-
-	cfg1 = RTL_R8(Config1);
-	if (!(cfg1  & VPD)) {
-		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);
-	}
-
-	vpd_cap = pci_find_capability(pdev, PCI_CAP_ID_VPD);
-	if (!vpd_cap)
-		return;
-
-	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
-	 * specified."
-	 */
-	if (rtl_eeprom_read(pdev, vpd_cap, 0x000e, (__le32*)&mac[0]) < 0 ||
-	    rtl_eeprom_read(pdev, vpd_cap, 0x0012, (__le32*)&mac[4]) < 0) {
-		if (netif_msg_probe(tp)) {
-			dev_warn(&pdev->dev,
-				 "reading MAC address from EEPROM failed\n");
-		}
-		return;
-	}
-
-	if (netif_msg_probe(tp)) {
-		DECLARE_MAC_BUF(buf);
-
-		dev_info(&pdev->dev, "MAC address found in EEPROM: %s\n",
-			 print_mac(buf, mac));
-	}
-
-	if (is_valid_ether_addr(mac))
-		rtl_rar_set(tp, mac);
-}
-
 static int __devinit
 rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
@@ -2178,8 +2092,6 @@ rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	tp->mmio_addr = ioaddr;
 
-	rtl_init_mac_address(tp, ioaddr);
-
 	/* Get MAC address */
 	for (i = 0; i < MAC_ADDR_LEN; i++)
 		dev->dev_addr[i] = RTL_R8(MAC0 + i);
-- 
1.5.3.3

Re: [PATCH 1/1] r8169: revert "read MAC address from EEPROM on init"

[Jeff Garzik]

Francois Romieu wrote:
> This reverts commit 7bf6bf4803df1adc927f585168d2135fb019c698.
> 
> The code has both a short existence and an increasing track
> of failures despite some work to amend it for -rc1. It is not
> just a matter of reading the eeprom: sometimes the eeprom is
> read correctly, then the mac address is not written correctly
> back into the mac registers.
> 
> Some chipsets seem to work reliably but it is not clear at
> this point if the code can simply be made to work on a
> per-chipset basis and post -rc1 is not the place where I want
> to experiment these things.
> 
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>

Acked-by: Jeff Garzik <jgarzik@redhat.com>

I'll send upstream if Linus doesn't pick it up shortly...

Re: [PATCH 1/1] r8169: revert "read MAC address from EEPROM on init"

[Francois Romieu]

Krzysztof Halasa <khc@pm.waw.pl> :
[...]
> Ahh, just PCI VPD.
> 
> IMHO the above shouldn't hurt anyone, in theory. You only write to
> VPD_ADDR, and to change actual VPD data (i.e., EEPROM), you need to
> write to VPD_DATA.
> 
> Anyone should be allowed to do that even for unknown PCI devices
> without a problem.

Theorically, yes.

In practice I noticed rather different behaviors between a 8168b and
a 8110s when I tried accessing the eeprom through plain old bitbang
a few months ago (i.e. ok on the 8110, lots of zero on the 8168b).

> > -static void rtl_init_mac_address(struct rtl8169_private *tp,
> 
> > -	if (!(cfg1  & VPD)) {
> > -		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);
> 
> Now I wonder what do they say the above does exactly.
> Unfortunately the docs don't seem to be public.

Realtek's legal department has changed his mind :o(

>From what I had received beforehand:

RTL8169:  R/W Set to enable Vital Product Data. The VPD data is stored in
          93C46 or 93c56 from within offset 40h-7fh.

RTL8168C: R (huh ?) Set to enable Vital Product Data. Always 1.

[...]
> > -	if (netif_msg_probe(tp)) {
> > -		DECLARE_MAC_BUF(buf);
> > -
> > -		dev_info(&pdev->dev, "MAC address found in EEPROM: %s\n",
> > -			 print_mac(buf, mac));
> > -	}
> > -
> > -	if (is_valid_ether_addr(mac))
> > -		rtl_rar_set(tp, mac);
> > -}
> 
> No RTL_W8(Config1, cfg1 | ~VPD) to disable (perhaps R/W) access?

Apparently not. See above.

> Though obviously first reading a good address from the EEPROM and then
> erasing first 32 bits is a different story.

Yes.

> I wonder if changing the MAC address by hand works (using ifconfig, to
> arbitrary address).

It does. It can be quite sticky. That was the whole point of the
patch.

-- 
Ueimor

Re: [PATCH 1/1] r8169: revert "read MAC address from EEPROM on init"

[Krzysztof Halasa]

Francois Romieu <romieu@fr.zoreil.com> writes:

> In practice I noticed rather different behaviors between a 8168b and
> a 8110s when I tried accessing the eeprom through plain old bitbang
> a few months ago (i.e. ok on the 8110, lots of zero on the 8168b).

You mean "manually" controlling EEPROM clock and data lines, using
a different register?
A speed issue by chance?

If they have a different way to write to the EEPROM (using direct
access to EEPROM signals), then perhaps there is no EEPROM write mode
using VPD? VPD write mode (not writing to the address/F register, but
writing to VPD "storage" itself) is quite simple and permanent, thus
kind of dangerous. If it's R/O, then it can't erase the EEPROM (parts).

>> I wonder if changing the MAC address by hand works (using ifconfig, to
>> arbitrary address).
>
> It does. It can be quite sticky. That was the whole point of the
> patch.

I see.

I wouldn't drop this patch permanently, though.
-- 
Krzysztof Halasa

Re: [PATCH 1/1] r8169: revert "read MAC address from EEPROM on init"

[Krzysztof Halasa]

[Reposting to lkml only, Gnus likes to eat quoting from email addresses
under specific circumstances and then vger doesn't pass them through]

Francois Romieu <romieu@fr.zoreil.com> writes:

> -static int rtl_eeprom_read(struct pci_dev *pdev, int cap, int addr, __le32 *val)
> -{
> -	int ret, count = 100;
> -	u16 status = 0;
> -	u32 value;
> -
> -	ret = pci_write_config_word(pdev, cap + PCI_VPD_ADDR, addr);
> -	if (ret < 0)
> -		return ret;
> -
> -	do {
> -		udelay(10);
> -		ret = pci_read_config_word(pdev, cap + PCI_VPD_ADDR, &status);
> -		if (ret < 0)
> -			return ret;
> -	} while (!(status & PCI_VPD_ADDR_F) && --count);
> -
> -	if (!(status & PCI_VPD_ADDR_F))
> -		return -ETIMEDOUT;
> -
> -	ret = pci_read_config_dword(pdev, cap + PCI_VPD_DATA, &value);
> -	if (ret < 0)
> -		return ret;
> -
> -	*val = cpu_to_le32(value);
> -
> -	return 0;

Ahh, just PCI VPD.

IMHO the above shouldn't hurt anyone, in theory. You only write to
VPD_ADDR, and to change actual VPD data (i.e., EEPROM), you need to
write to VPD_DATA.

Anyone should be allowed to do that even for unknown PCI devices
without a problem.

> -static void rtl_init_mac_address(struct rtl8169_private *tp,

> -	if (!(cfg1  & VPD)) {
> -		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);

Now I wonder what do they say the above does exactly.
Unfortunately the docs don't seem to be public.

> -	 * 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
> -	 * specified."

Nice.

> -	if (netif_msg_probe(tp)) {
> -		DECLARE_MAC_BUF(buf);
> -
> -		dev_info(&pdev->dev, "MAC address found in EEPROM: %s\n",
> -			 print_mac(buf, mac));
> -	}
> -
> -	if (is_valid_ether_addr(mac))
> -		rtl_rar_set(tp, mac);
> -}

No RTL_W8(Config1, cfg1 | ~VPD) to disable (perhaps R/W) access?

Though obviously first reading a good address from the EEPROM and then
erasing first 32 bits is a different story. I wonder if changing the
MAC address by hand works (using ifconfig, to arbitrary address).

-- 
Krzysztof Halasa