[PATCH 0/9] PCI vpd patches (rev4)

[PATCH 0/9] PCI vpd patches (rev4)

[Stephen Hemminger]

This is update to VPD access patches.  These fix the VPD routines
to have a sufficient timeout, and modify the skge/sky2 drivers to
not use their own VPD access code.

Since only network drivers are impacted, these should go through
the net-next tree for 2.6.28.

-- 

[PATCH 1/9] PCI: vpd handle longer delays in access (rev3)

[Stephen Hemminger]

[-- Attachment #1: vpd-delay.patch --]
[-- Type: text/plain, Size: 4040 bytes --]

Accessing the VPD area can take a long time.  The existing 
VPD access code fails consistently on my hardware. There are comments
in the Marvell vendor driver that it can take up to 13ms per word. 

Change the access routines to:
  * use a mutex rather than spinning with IRQ's disabled and lock held
  * have a much longer timeout
  * call cond_resched while spinning

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/pci/access.c	2008-09-09 13:56:10.000000000 -0700
+++ b/drivers/pci/access.c	2008-09-09 13:56:56.000000000 -0700
@@ -133,39 +133,46 @@ PCI_USER_WRITE_CONFIG(dword, u32)
 
 struct pci_vpd_pci22 {
 	struct pci_vpd base;
-	spinlock_t lock; /* controls access to hardware and the flags */
-	u8	cap;
+	struct mutex lock;
+	u16	flag;
 	bool	busy;
-	bool	flag; /* value of F bit to wait for */
+	u8	cap;
 };
 
-/* Wait for last operation to complete */
+/*
+ * Wait for last operation to complete.
+ * This code has to spin since there is no other notification from the PCI
+ * hardware. Since the VPD is often implemented by serial attachment to an
+ * EEPROM, it may take many milliseconds to complete.
+ */
 static int pci_vpd_pci22_wait(struct pci_dev *dev)
 {
 	struct pci_vpd_pci22 *vpd =
 		container_of(dev->vpd, struct pci_vpd_pci22, base);
-	u16 flag, status;
-	int wait;
+	unsigned long timeout = jiffies + HZ/20 + 2;
+	u16 status;
 	int ret;
 
 	if (!vpd->busy)
 		return 0;
 
-	flag = vpd->flag ? PCI_VPD_ADDR_F : 0;
-	wait = vpd->flag ? 10 : 1000; /* read: 100 us; write: 10 ms */
 	for (;;) {
-		ret = pci_user_read_config_word(dev,
-						vpd->cap + PCI_VPD_ADDR,
+		ret = pci_user_read_config_word(dev, vpd->cap + PCI_VPD_ADDR,
 						&status);
-		if (ret < 0)
+		if (ret)
 			return ret;
-		if ((status & PCI_VPD_ADDR_F) == flag) {
+
+		if ((status & PCI_VPD_ADDR_F) == vpd->flag) {
 			vpd->busy = false;
 			return 0;
 		}
-		if (wait-- == 0)
+
+		if (time_after(jiffies, timeout))
 			return -ETIMEDOUT;
-		udelay(10);
+		if (fatal_signal_pending(current))
+			return -EINTR;
+		if(!cond_resched())
+			udelay(10);
 	}
 }
 
@@ -175,7 +182,7 @@ static int pci_vpd_pci22_read(struct pci
 	struct pci_vpd_pci22 *vpd =
 		container_of(dev->vpd, struct pci_vpd_pci22, base);
 	u32 val;
-	int ret;
+	int ret = 0;
 	int begin, end, i;
 
 	if (pos < 0 || pos > vpd->base.len || size > vpd->base.len  - pos)
@@ -183,7 +190,9 @@ static int pci_vpd_pci22_read(struct pci
 	if (size == 0)
 		return 0;
 
-	spin_lock_irq(&vpd->lock);
+	if (mutex_lock_killable(&vpd->lock))
+		return -EINTR;
+
 	ret = pci_vpd_pci22_wait(dev);
 	if (ret < 0)
 		goto out;
@@ -191,15 +200,16 @@ static int pci_vpd_pci22_read(struct pci
 					 pos & ~3);
 	if (ret < 0)
 		goto out;
+
 	vpd->busy = true;
-	vpd->flag = 1;
+	vpd->flag = PCI_VPD_ADDR_F;
 	ret = pci_vpd_pci22_wait(dev);
 	if (ret < 0)
 		goto out;
 	ret = pci_user_read_config_dword(dev, vpd->cap + PCI_VPD_DATA,
 					 &val);
 out:
-	spin_unlock_irq(&vpd->lock);
+	mutex_unlock(&vpd->lock);
 	if (ret < 0)
 		return ret;
 
@@ -220,7 +230,7 @@ static int pci_vpd_pci22_write(struct pc
 	struct pci_vpd_pci22 *vpd =
 		container_of(dev->vpd, struct pci_vpd_pci22, base);
 	u32 val;
-	int ret;
+	int ret = 0;
 
 	if (pos < 0 || pos > vpd->base.len || pos & 3 ||
 	    size > vpd->base.len - pos || size < 4)
@@ -231,7 +241,8 @@ static int pci_vpd_pci22_write(struct pc
 	val |= ((u8) *buf++) << 16;
 	val |= ((u32)(u8) *buf++) << 24;
 
-	spin_lock_irq(&vpd->lock);
+	if (mutex_lock_killable(&vpd->lock))
+		return -EINTR;
 	ret = pci_vpd_pci22_wait(dev);
 	if (ret < 0)
 		goto out;
@@ -247,7 +258,7 @@ static int pci_vpd_pci22_write(struct pc
 	vpd->flag = 0;
 	ret = pci_vpd_pci22_wait(dev);
 out:
-	spin_unlock_irq(&vpd->lock);
+	mutex_unlock(&vpd->lock);
 	if (ret < 0)
 		return ret;
 
@@ -279,7 +290,7 @@ int pci_vpd_pci22_init(struct pci_dev *d
 
 	vpd->base.len = PCI_VPD_PCI22_SIZE;
 	vpd->base.ops = &pci_vpd_pci22_ops;
-	spin_lock_init(&vpd->lock);
+	mutex_init(&vpd->lock);
 	vpd->cap = cap;
 	vpd->busy = false;
 	dev->vpd = &vpd->base;

-- 

Re: [PATCH 1/9] PCI: vpd handle longer delays in access (rev3)

[Matthew Wilcox]

On Thu, Sep 25, 2008 at 09:48:52AM -0700, Stephen Hemminger wrote:
> +		if(!cond_resched())

Style nit (did you run checkpatch?)

I think I've commented on the previous versions of this patch enough for
it to be worth adding:

Reviwed-by: Matthew Wilcox <willy@linux.intel.com>

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

[PATCH 2/9] PCI: revise VPD access interface (rev3)

[Stephen Hemminger]

[-- Attachment #1: vpd-ops.patch --]
[-- Type: text/plain, Size: 9321 bytes --]

Change PCI VPD API which was only used by sysfs to something usable
in drivers. 
   * move iteration over multiple words to the low level
   * use conventional types for arguments
   * add exportable wrapper

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
 drivers/pci/access.c    |  157 ++++++++++++++++++++++++++++++------------------
 drivers/pci/pci-sysfs.c |   38 ++---------
 drivers/pci/pci.h       |    6 -
 include/linux/pci.h     |    4 +
 4 files changed, 115 insertions(+), 90 deletions(-)

--- a/drivers/pci/access.c	2008-09-09 13:56:56.000000000 -0700
+++ b/drivers/pci/access.c	2008-09-09 13:57:05.000000000 -0700
@@ -66,6 +66,39 @@ EXPORT_SYMBOL(pci_bus_write_config_byte)
 EXPORT_SYMBOL(pci_bus_write_config_word);
 EXPORT_SYMBOL(pci_bus_write_config_dword);
 
+
+/**
+ * pci_read_vpd - Read one entry from Vital Product Data
+ * @dev:	pci device struct
+ * @pos:	offset in vpd space
+ * @count:	number of bytes to read
+ * @buf:	pointer to where to store result
+ *
+ */
+ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf)
+{
+	if (!dev->vpd || !dev->vpd->ops)
+		return -ENODEV;
+	return dev->vpd->ops->read(dev, pos, count, buf);
+}
+EXPORT_SYMBOL(pci_read_vpd);
+
+/**
+ * pci_write_vpd - Write entry to Vital Product Data
+ * @dev:	pci device struct
+ * @pos:	offset in vpd space
+ * @count:	number of bytes to read
+ * @val:	value to write
+ *
+ */
+ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf)
+{
+	if (!dev->vpd || !dev->vpd->ops)
+		return -ENODEV;
+	return dev->vpd->ops->write(dev, pos, count, buf);
+}
+EXPORT_SYMBOL(pci_write_vpd);
+
 /*
  * The following routines are to prevent the user from accessing PCI config
  * space when it's unsafe to do so.  Some devices require this during BIST and
@@ -176,19 +209,17 @@ static int pci_vpd_pci22_wait(struct pci
 	}
 }
 
-static int pci_vpd_pci22_read(struct pci_dev *dev, int pos, int size,
-			      char *buf)
+static ssize_t pci_vpd_pci22_read(struct pci_dev *dev, loff_t pos, size_t count,
+				  void *arg)
 {
 	struct pci_vpd_pci22 *vpd =
 		container_of(dev->vpd, struct pci_vpd_pci22, base);
-	u32 val;
-	int ret = 0;
-	int begin, end, i;
+	int ret;
+	loff_t end = pos + count;
+	u8 *buf = arg;
 
-	if (pos < 0 || pos > vpd->base.len || size > vpd->base.len  - pos)
+	if (pos < 0 || pos > vpd->base.len || count > vpd->base.len - pos)
 		return -EINVAL;
-	if (size == 0)
-		return 0;
 
 	if (mutex_lock_killable(&vpd->lock))
 		return -EINTR;
@@ -196,73 +227,84 @@ static int pci_vpd_pci22_read(struct pci
 	ret = pci_vpd_pci22_wait(dev);
 	if (ret < 0)
 		goto out;
-	ret = pci_user_write_config_word(dev, vpd->cap + PCI_VPD_ADDR,
-					 pos & ~3);
-	if (ret < 0)
-		goto out;
 
-	vpd->busy = true;
-	vpd->flag = PCI_VPD_ADDR_F;
-	ret = pci_vpd_pci22_wait(dev);
-	if (ret < 0)
-		goto out;
-	ret = pci_user_read_config_dword(dev, vpd->cap + PCI_VPD_DATA,
-					 &val);
+	while (pos < end) {
+		u32 val;
+		unsigned int i, skip;
+
+		ret = pci_user_write_config_word(dev, vpd->cap + PCI_VPD_ADDR,
+						 pos & ~3);
+		if (ret < 0)
+			break;
+		vpd->busy = true;
+		vpd->flag = PCI_VPD_ADDR_F;
+		ret = pci_vpd_pci22_wait(dev);
+		if (ret < 0)
+			break;
+
+		ret = pci_user_read_config_dword(dev, vpd->cap + PCI_VPD_DATA, &val);
+		if (ret < 0)
+			break;
+
+		skip = pos & 3;
+		for (i = 0;  i < sizeof(u32); i++) {
+			if (i >= skip) {
+				*buf++ = val;
+				if (++pos == end)
+					break;
+			}
+			val >>= 8;
+		}
+	}
 out:
 	mutex_unlock(&vpd->lock);
-	if (ret < 0)
-		return ret;
-
-	/* Convert to bytes */
-	begin = pos & 3;
-	end = min(4, begin + size);
-	for (i = 0; i < end; ++i) {
-		if (i >= begin)
-			*buf++ = val;
-		val >>= 8;
-	}
-	return end - begin;
+	return ret ? ret : count;
 }
 
-static int pci_vpd_pci22_write(struct pci_dev *dev, int pos, int size,
-			       const char *buf)
+static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count,
+				   const void *arg)
 {
 	struct pci_vpd_pci22 *vpd =
 		container_of(dev->vpd, struct pci_vpd_pci22, base);
-	u32 val;
+	const u8 *buf = arg;
+	loff_t end = pos + count;
 	int ret = 0;
 
-	if (pos < 0 || pos > vpd->base.len || pos & 3 ||
-	    size > vpd->base.len - pos || size < 4)
+	if (pos > vpd->base.len || pos & 3)
 		return -EINVAL;
 
-	val = (u8) *buf++;
-	val |= ((u8) *buf++) << 8;
-	val |= ((u8) *buf++) << 16;
-	val |= ((u32)(u8) *buf++) << 24;
-
 	if (mutex_lock_killable(&vpd->lock))
 		return -EINTR;
+
 	ret = pci_vpd_pci22_wait(dev);
 	if (ret < 0)
 		goto out;
-	ret = pci_user_write_config_dword(dev, vpd->cap + PCI_VPD_DATA,
-					  val);
-	if (ret < 0)
-		goto out;
-	ret = pci_user_write_config_word(dev, vpd->cap + PCI_VPD_ADDR,
-					 pos | PCI_VPD_ADDR_F);
-	if (ret < 0)
-		goto out;
-	vpd->busy = true;
-	vpd->flag = 0;
-	ret = pci_vpd_pci22_wait(dev);
+
+	while (pos < end) {
+		u32 val;
+
+		val = *buf++;
+		val |= *buf++ << 8;
+		val |= *buf++ << 16;
+		val |= *buf++ << 24;
+
+		ret = pci_user_write_config_dword(dev, vpd->cap + PCI_VPD_DATA, val);
+		if (ret < 0)
+			break;
+		ret = pci_user_write_config_word(dev, vpd->cap + PCI_VPD_ADDR,
+						 pos | PCI_VPD_ADDR_F);
+		if (ret < 0)
+			break;
+
+		vpd->busy = true;
+		vpd->flag = 0;
+		ret = pci_vpd_pci22_wait(dev);
+
+		pos += sizeof(u32);
+	}
 out:
 	mutex_unlock(&vpd->lock);
-	if (ret < 0)
-		return ret;
-
-	return 4;
+	return ret ? ret : count;
 }
 
 static void pci_vpd_pci22_release(struct pci_dev *dev)
@@ -270,7 +312,7 @@ static void pci_vpd_pci22_release(struct
 	kfree(container_of(dev->vpd, struct pci_vpd_pci22, base));
 }
 
-static struct pci_vpd_ops pci_vpd_pci22_ops = {
+static const struct pci_vpd_ops pci_vpd_pci22_ops = {
 	.read = pci_vpd_pci22_read,
 	.write = pci_vpd_pci22_write,
 	.release = pci_vpd_pci22_release,
--- a/drivers/pci/pci.h	2008-09-09 13:56:10.000000000 -0700
+++ b/drivers/pci/pci.h	2008-09-09 13:57:05.000000000 -0700
@@ -44,14 +44,14 @@ extern int pci_user_write_config_word(st
 extern int pci_user_write_config_dword(struct pci_dev *dev, int where, u32 val);
 
 struct pci_vpd_ops {
-	int (*read)(struct pci_dev *dev, int pos, int size, char *buf);
-	int (*write)(struct pci_dev *dev, int pos, int size, const char *buf);
+	ssize_t (*read)(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
+	ssize_t (*write)(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
 	void (*release)(struct pci_dev *dev);
 };
 
 struct pci_vpd {
 	unsigned int len;
-	struct pci_vpd_ops *ops;
+	const struct pci_vpd_ops *ops;
 	struct bin_attribute *attr; /* descriptor for sysfs VPD entry */
 };
 
--- a/drivers/pci/pci-sysfs.c	2008-09-09 13:56:10.000000000 -0700
+++ b/drivers/pci/pci-sysfs.c	2008-09-09 13:57:05.000000000 -0700
@@ -360,55 +360,33 @@ pci_write_config(struct kobject *kobj, s
 }
 
 static ssize_t
-pci_read_vpd(struct kobject *kobj, struct bin_attribute *bin_attr,
-	     char *buf, loff_t off, size_t count)
+read_vpd_attr(struct kobject *kobj, struct bin_attribute *bin_attr,
+	      char *buf, loff_t off, size_t count)
 {
 	struct pci_dev *dev =
 		to_pci_dev(container_of(kobj, struct device, kobj));
-	int end;
-	int ret;
 
 	if (off > bin_attr->size)
 		count = 0;
 	else if (count > bin_attr->size - off)
 		count = bin_attr->size - off;
-	end = off + count;
 
-	while (off < end) {
-		ret = dev->vpd->ops->read(dev, off, end - off, buf);
-		if (ret < 0)
-			return ret;
-		buf += ret;
-		off += ret;
-	}
-
-	return count;
+	return pci_read_vpd(dev, off, count, buf);
 }
 
 static ssize_t
-pci_write_vpd(struct kobject *kobj, struct bin_attribute *bin_attr,
-	      char *buf, loff_t off, size_t count)
+write_vpd_attr(struct kobject *kobj, struct bin_attribute *bin_attr,
+	       char *buf, loff_t off, size_t count)
 {
 	struct pci_dev *dev =
 		to_pci_dev(container_of(kobj, struct device, kobj));
-	int end;
-	int ret;
 
 	if (off > bin_attr->size)
 		count = 0;
 	else if (count > bin_attr->size - off)
 		count = bin_attr->size - off;
-	end = off + count;
-
-	while (off < end) {
-		ret = dev->vpd->ops->write(dev, off, end - off, buf);
-		if (ret < 0)
-			return ret;
-		buf += ret;
-		off += ret;
-	}
 
-	return count;
+	return pci_write_vpd(dev, off, count, buf);
 }
 
 #ifdef HAVE_PCI_LEGACY
@@ -739,8 +717,8 @@ int __must_check pci_create_sysfs_dev_fi
 			attr->size = pdev->vpd->len;
 			attr->attr.name = "vpd";
 			attr->attr.mode = S_IRUSR | S_IWUSR;
-			attr->read = pci_read_vpd;
-			attr->write = pci_write_vpd;
+			attr->read = read_vpd_attr;
+			attr->write = write_vpd_attr;
 			retval = sysfs_create_bin_file(&pdev->dev.kobj, attr);
 			if (retval)
 				goto err_vpd;
--- a/include/linux/pci.h	2008-09-09 13:56:10.000000000 -0700
+++ b/include/linux/pci.h	2008-09-09 13:57:05.000000000 -0700
@@ -650,6 +650,10 @@ int pci_back_from_sleep(struct pci_dev *
 /* Functions for PCI Hotplug drivers to use */
 int pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap);
 
+/* Vital product data routines */
+ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
+ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
+
 /* Helper functions for low-level code (drivers/pci/setup-[bus,res].c) */
 void pci_bus_assign_resources(struct pci_bus *bus);
 void pci_bus_size_bridges(struct pci_bus *bus);

-- 

Re: [PATCH 2/9] PCI: revise VPD access interface (rev3)

[Ben Hutchings]

On Thu, 2008-09-25 at 09:48 -0700, Stephen Hemminger wrote:
[...]
> -static int pci_vpd_pci22_write(struct pci_dev *dev, int pos, int size,
> -			       const char *buf)
> +static ssize_t pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count,
> +				   const void *arg)
>  {
>  	struct pci_vpd_pci22 *vpd =
>  		container_of(dev->vpd, struct pci_vpd_pci22, base);
> -	u32 val;
> +	const u8 *buf = arg;
> +	loff_t end = pos + count;

count should be (count & ~3)

>  	int ret = 0;
>  
> -	if (pos < 0 || pos > vpd->base.len || pos & 3 ||
> -	    size > vpd->base.len - pos || size < 4)
> +	if (pos > vpd->base.len || pos & 3)
>  		return -EINVAL;

Why did you remove the tests for pos < 0 and size > vpd->base.len - pos?
I know write_vpd_attr() checks these but you're about to add other
callers.

[...]
>  out:
>  	mutex_unlock(&vpd->lock);
> -	if (ret < 0)
> -		return ret;
> -
> -	return 4;
> +	return ret ? ret : count;
>  }
[...]

count should be (count & ~3)

Alternately you could treat (count & 3) != 0 as invalid.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH 3/9] PCI: add interface to set visible size of VPD (rev3)

[Stephen Hemminger]

[-- Attachment #1: vpd-size.patch --]
[-- Type: text/plain, Size: 1676 bytes --]

The VPD on all devices may not be 32K. Unfortunately, there is no
generic way to find the size, so this adds a simple API hook
to reset it.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
 drivers/pci/access.c |   23 +++++++++++++++++++++++
 include/linux/pci.h  |    1 +
 2 files changed, 24 insertions(+)

--- a/drivers/pci/access.c	2008-09-09 13:57:05.000000000 -0700
+++ b/drivers/pci/access.c	2008-09-09 13:57:06.000000000 -0700
@@ -340,6 +340,29 @@ int pci_vpd_pci22_init(struct pci_dev *d
 }
 
 /**
+ * pci_vpd_truncate - Set available Vital Product Data size
+ * @dev:	pci device struct
+ * @size:	available memory in bytes
+ *
+ * Adjust size of available VPD area.
+ */
+int pci_vpd_truncate(struct pci_dev *dev, size_t size)
+{
+	if (!dev->vpd)
+		return -EINVAL;
+
+	/* limited by the access method */
+	if (size > dev->vpd->len)
+		return -EINVAL;
+
+	dev->vpd->len = size;
+	dev->vpd->attr->size = size;
+
+	return 0;
+}
+EXPORT_SYMBOL(pci_vpd_truncate);
+
+/**
  * pci_block_user_cfg_access - Block userspace PCI config reads/writes
  * @dev:	pci device struct
  *
--- a/include/linux/pci.h	2008-09-09 13:57:05.000000000 -0700
+++ b/include/linux/pci.h	2008-09-09 13:57:06.000000000 -0700
@@ -653,6 +653,7 @@ int pci_bus_find_capability(struct pci_b
 /* Vital product data routines */
 ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
 ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
+int pci_vpd_truncate(struct pci_dev *dev, size_t size);
 
 /* Helper functions for low-level code (drivers/pci/setup-[bus,res].c) */
 void pci_bus_assign_resources(struct pci_bus *bus);

-- 

Re: [PATCH 3/9] PCI: add interface to set visible size of VPD (rev3)

[Jeff Garzik]

Stephen Hemminger wrote:
> The VPD on all devices may not be 32K. Unfortunately, there is no
> generic way to find the size, so this adds a simple API hook
> to reset it.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> ---
>  drivers/pci/access.c |   23 +++++++++++++++++++++++
>  include/linux/pci.h  |    1 +
>  2 files changed, 24 insertions(+)
> 
> --- a/drivers/pci/access.c	2008-09-09 13:57:05.000000000 -0700
> +++ b/drivers/pci/access.c	2008-09-09 13:57:06.000000000 -0700
> @@ -340,6 +340,29 @@ int pci_vpd_pci22_init(struct pci_dev *d
>  }
>  
>  /**
> + * pci_vpd_truncate - Set available Vital Product Data size
> + * @dev:	pci device struct
> + * @size:	available memory in bytes
> + *
> + * Adjust size of available VPD area.
> + */
> +int pci_vpd_truncate(struct pci_dev *dev, size_t size)
> +{
> +	if (!dev->vpd)
> +		return -EINVAL;
> +
> +	/* limited by the access method */
> +	if (size > dev->vpd->len)
> +		return -EINVAL;
> +
> +	dev->vpd->len = size;
> +	dev->vpd->attr->size = size;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(pci_vpd_truncate);

What is the initial size?

It seems like a bad idea to start out assuming the max, and reduce once 
a knowledgeable driver appears.

It seems more safe to start out at zero, and let a driver increase it?

That way, by design, you eliminate the possibility (later, if not now) 
of someone assuming that the full 32k can be read as soon as the 
interface is available.

	Jeff

Re: [PATCH 3/9] PCI: add interface to set visible size of VPD (rev3)

[Ben Hutchings]

On Thu, 2008-09-25 at 13:22 -0400, Jeff Garzik wrote:
[...]
> What is the initial size?
> 
> It seems like a bad idea to start out assuming the max, and reduce once 
> a knowledgeable driver appears.
> 
> It seems more safe to start out at zero, and let a driver increase it?

VPD is meant to be usable even without a driver - that's why it's a
(somewhat) standard capability and not device-specific.

> That way, by design, you eliminate the possibility (later, if not now) 
> of someone assuming that the full 32k can be read as soon as the 
> interface is available.

I believe that's generally true, though it may be slow.

Maybe we could read as much as we need to find the real length when the
sysfs file is first opened?

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH 4/9] sky2: set VPD size

[Stephen Hemminger]

[-- Attachment #1: sky2-vpd-size.patch --]
[-- Type: text/plain, Size: 2073 bytes --]

Read configuration register during probe and use it to size the
available VPD. Move existing code using same register slightly
earlier in probe handling.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/sky2.c	2008-09-09 13:56:09.000000000 -0700
+++ b/drivers/net/sky2.c	2008-09-09 13:57:07.000000000 -0700
@@ -4281,6 +4281,7 @@ static int __devinit sky2_probe(struct p
 	struct net_device *dev;
 	struct sky2_hw *hw;
 	int err, using_dac = 0, wol_default;
+	u32 reg;
 	char buf1[16];
 
 	err = pci_enable_device(pdev);
@@ -4314,6 +4315,34 @@ static int __devinit sky2_probe(struct p
 		}
 	}
 
+	/* Get configuration information
+	 * Note: only regular PCI config access once to test for HW issues
+	 *       other PCI access through shared memory for speed and to
+	 *	 avoid MMCONFIG problems.
+	 */
+	err = pci_read_config_dword(pdev, PCI_DEV_REG2, &reg);
+	if (err) {
+		dev_err(&pdev->dev, "PCI read config failed\n");
+		goto err_out_free_regions;
+	}
+
+	/* size of available VPD, only impact sysfs */
+	err = pci_vpd_truncate(pdev, 1ul << (((reg & PCI_VPD_ROM_SZ) >> 14) + 8));
+	if (err)
+		dev_warn(&pdev->dev, "Can't set VPD size\n");
+
+#ifdef __BIG_ENDIAN
+	/* The sk98lin vendor driver uses hardware byte swapping but
+	 * this driver uses software swapping.
+	 */
+	reg &= ~PCI_REV_DESC;
+	err = pci_write_config_dword(pdev,PCI_DEV_REG2, reg);
+	if (err) {
+		dev_err(&pdev->dev, "PCI write config failed\n");
+		goto err_out_free_regions;
+	}
+#endif
+
 	wol_default = pci_wake_enabled(pdev) ? WAKE_MAGIC : 0;
 
 	err = -ENOMEM;
@@ -4331,18 +4360,6 @@ static int __devinit sky2_probe(struct p
 		goto err_out_free_hw;
 	}
 
-#ifdef __BIG_ENDIAN
-	/* The sk98lin vendor driver uses hardware byte swapping but
-	 * this driver uses software swapping.
-	 */
-	{
-		u32 reg;
-		reg = sky2_pci_read32(hw, PCI_DEV_REG2);
-		reg &= ~PCI_REV_DESC;
-		sky2_pci_write32(hw, PCI_DEV_REG2, reg);
-	}
-#endif
-
 	/* ring for status responses */
 	hw->st_le = pci_alloc_consistent(pdev, STATUS_LE_BYTES, &hw->st_dma);
 	if (!hw->st_le)

-- 

[PATCH 5/9] sky2: move VPD display into debug interface

[Stephen Hemminger]

[-- Attachment #1: sky2-vpd-debug.patch --]
[-- Type: text/plain, Size: 4504 bytes --]

The VPD stuff has more data and isn't generally that useful, so move
it into the existing debugfs display and use the new PCI VPD
accessor routines.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/sky2.c	2008-09-09 13:57:07.000000000 -0700
+++ b/drivers/net/sky2.c	2008-09-09 13:57:08.000000000 -0700
@@ -3861,6 +3861,86 @@ static const struct ethtool_ops sky2_eth
 
 static struct dentry *sky2_debug;
 
+
+/*
+ * Read and parse the first part of Vital Product Data
+ */
+#define VPD_SIZE	128
+#define VPD_MAGIC	0x82
+
+static const struct vpd_tag {
+	char tag[2];
+	char *label;
+} vpd_tags[] = {
+	{ "PN",	"Part Number" },
+	{ "EC", "Engineering Level" },
+	{ "MN", "Manufacturer" },
+	{ "SN", "Serial Number" },
+	{ "YA", "Asset Tag" },
+	{ "VL", "First Error Log Message" },
+	{ "VF", "Second Error Log Message" },
+	{ "VB", "Boot Agent ROM Configuration" },
+	{ "VE", "EFI UNDI Configuration" },
+};
+
+static void sky2_show_vpd(struct seq_file *seq, struct sky2_hw *hw)
+{
+	size_t vpd_size;
+	loff_t offs;
+	u8 len;
+	unsigned char *buf;
+	u16 reg2;
+
+	reg2 = sky2_pci_read16(hw, PCI_DEV_REG2);
+	vpd_size = 1 << ( ((reg2 & PCI_VPD_ROM_SZ) >> 14) + 8);
+
+	seq_printf(seq, "%s Product Data\n", pci_name(hw->pdev));
+	buf = kmalloc(vpd_size, GFP_KERNEL);
+	if (!buf) {
+		seq_puts(seq, "no memory!\n");
+		return;
+	}
+
+	if (pci_read_vpd(hw->pdev, 0, vpd_size, buf) < 0) {
+		seq_puts(seq, "VPD read failed\n");
+		goto out;
+	}
+
+	if (buf[0] != VPD_MAGIC) {
+		seq_printf(seq, "VPD tag mismatch: %#x\n", buf[0]);
+		goto out;
+	}
+	len = buf[1];
+	if (len == 0 || len > vpd_size - 4) {
+		seq_printf(seq, "Invalid id length: %d\n", len);
+		goto out;
+	}
+
+	seq_printf(seq, "%.*s\n", len, buf + 3);
+	offs = len + 3;
+
+	while (offs < vpd_size - 4) {
+		int i;
+
+		if (!memcmp("RW", buf + offs, 2))	/* end marker */
+			break;
+		len = buf[offs + 2];
+		if (offs + len + 3 >= vpd_size)
+			break;
+
+		for (i = 0; i < ARRAY_SIZE(vpd_tags); i++) {
+			if (!memcmp(vpd_tags[i].tag, buf + offs, 2)) {
+				seq_printf(seq, " %s: %.*s\n",
+					   vpd_tags[i].label, len, buf + offs + 3);
+				break;
+			}
+		}
+		offs += len + 3;
+	}
+out:
+	kfree(buf);
+}
+
 static int sky2_debug_show(struct seq_file *seq, void *v)
 {
 	struct net_device *dev = seq->private;
@@ -3870,14 +3950,18 @@ static int sky2_debug_show(struct seq_fi
 	unsigned idx, last;
 	int sop;
 
-	if (!netif_running(dev))
-		return -ENETDOWN;
+	sky2_show_vpd(seq, hw);
 
-	seq_printf(seq, "IRQ src=%x mask=%x control=%x\n",
+	seq_printf(seq, "\nIRQ src=%x mask=%x control=%x\n",
 		   sky2_read32(hw, B0_ISRC),
 		   sky2_read32(hw, B0_IMSK),
 		   sky2_read32(hw, B0_Y2_SP_ICR));
 
+	if (!netif_running(dev)) {
+		seq_printf(seq, "network not running\n");
+		return 0;
+	}
+
 	napi_disable(&hw->napi);
 	last = sky2_read16(hw, STAT_PUT_IDX);
 
@@ -4191,69 +4275,6 @@ static int __devinit pci_wake_enabled(st
 	return value & PCI_PM_CTRL_PME_ENABLE;
 }
 
-/*
- * Read and parse the first part of Vital Product Data
- */
-#define VPD_SIZE	128
-#define VPD_MAGIC	0x82
-
-static void __devinit sky2_vpd_info(struct sky2_hw *hw)
-{
-	int cap = pci_find_capability(hw->pdev, PCI_CAP_ID_VPD);
-	const u8 *p;
-	u8 *vpd_buf = NULL;
-	u16 len;
-	static struct vpd_tag {
-		char tag[2];
-		char *label;
-	} vpd_tags[] = {
-		{ "PN",	"Part Number" },
-		{ "EC", "Engineering Level" },
-		{ "MN", "Manufacturer" },
-	};
-
-	if (!cap)
-		goto out;
-
-	vpd_buf = kmalloc(VPD_SIZE, GFP_KERNEL);
-	if (!vpd_buf)
-		goto out;
-
-	if (sky2_vpd_read(hw, cap, vpd_buf, 0, VPD_SIZE))
-		goto out;
-
-	if (vpd_buf[0] != VPD_MAGIC)
-		goto out;
-	len = vpd_buf[1];
-	if (len == 0 || len > VPD_SIZE - 4)
-		goto out;
-	p = vpd_buf + 3;
-	dev_info(&hw->pdev->dev, "%.*s\n", len, p);
-	p += len;
-
-	while (p < vpd_buf + VPD_SIZE - 4) {
-		int i;
-
-		if (!memcmp("RW", p, 2))	/* end marker */
-			break;
-
-		len = p[2];
-		if (len > (p - vpd_buf) - 4)
-			break;
-
-		for (i = 0; i < ARRAY_SIZE(vpd_tags); i++) {
-			if (!memcmp(vpd_tags[i].tag, p, 2)) {
-				printk(KERN_DEBUG " %s: %.*s\n",
-				       vpd_tags[i].label, len, p + 3);
-				break;
-			}
-		}
-		p += len + 3;
-	}
-out:
-	kfree(vpd_buf);
-}
-
 /* This driver supports yukon2 chipset only */
 static const char *sky2_name(u8 chipid, char *buf, int sz)
 {
@@ -4374,8 +4395,6 @@ static int __devinit sky2_probe(struct p
 
 	sky2_reset(hw);
 
-	sky2_vpd_info(hw);
-
 	dev = sky2_init_netdev(hw, 0, using_dac, wol_default);
 	if (!dev) {
 		err = -ENOMEM;

-- 

[PATCH 6/9] sky2: remove VPD eeprom

[Stephen Hemminger]

[-- Attachment #1: sky2-no-eeprom.patch --]
[-- Type: text/plain, Size: 3742 bytes --]

The VPD is not really the firmware EEPROM. The real EEPROM is behind
an SPI interface, that maybe supported in later version.
Programming the VPD through the ethtool interface doesn't work
(doesn't really change firmware). Now that the interesting bits are
available through the debug interface and raw data is available through sysfs,
the old interface can go.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/sky2.c	2008-09-09 11:19:26.000000000 -0700
+++ b/drivers/net/sky2.c	2008-09-09 11:19:30.000000000 -0700
@@ -75,8 +75,6 @@
 #define NAPI_WEIGHT		64
 #define PHY_RETRIES		1000
 
-#define SKY2_EEPROM_MAGIC	0x9955aabb
-
 
 #define RING_NEXT(x,s)	(((x)+1) & ((s)-1))
 
@@ -3722,109 +3720,6 @@ static int sky2_set_tso(struct net_devic
 	return ethtool_op_set_tso(dev, data);
 }
 
-static int sky2_get_eeprom_len(struct net_device *dev)
-{
-	struct sky2_port *sky2 = netdev_priv(dev);
-	struct sky2_hw *hw = sky2->hw;
-	u16 reg2;
-
-	reg2 = sky2_pci_read16(hw, PCI_DEV_REG2);
-	return 1 << ( ((reg2 & PCI_VPD_ROM_SZ) >> 14) + 8);
-}
-
-static int sky2_vpd_wait(const struct sky2_hw *hw, int cap, u16 busy)
-{
-	unsigned long start = jiffies;
-
-	while ( (sky2_pci_read16(hw, cap + PCI_VPD_ADDR) & PCI_VPD_ADDR_F) == busy) {
-		/* Can take up to 10.6 ms for write */
-		if (time_after(jiffies, start + HZ/4)) {
-			dev_err(&hw->pdev->dev, PFX "VPD cycle timed out");
-			return -ETIMEDOUT;
-		}
-		mdelay(1);
-	}
-
-	return 0;
-}
-
-static int sky2_vpd_read(struct sky2_hw *hw, int cap, void *data,
-			 u16 offset, size_t length)
-{
-	int rc = 0;
-
-	while (length > 0) {
-		u32 val;
-
-		sky2_pci_write16(hw, cap + PCI_VPD_ADDR, offset);
-		rc = sky2_vpd_wait(hw, cap, 0);
-		if (rc)
-			break;
-
-		val = sky2_pci_read32(hw, cap + PCI_VPD_DATA);
-
-		memcpy(data, &val, min(sizeof(val), length));
-		offset += sizeof(u32);
-		data += sizeof(u32);
-		length -= sizeof(u32);
-	}
-
-	return rc;
-}
-
-static int sky2_vpd_write(struct sky2_hw *hw, int cap, const void *data,
-			  u16 offset, unsigned int length)
-{
-	unsigned int i;
-	int rc = 0;
-
-	for (i = 0; i < length; i += sizeof(u32)) {
-		u32 val = *(u32 *)(data + i);
-
-		sky2_pci_write32(hw, cap + PCI_VPD_DATA, val);
-		sky2_pci_write32(hw, cap + PCI_VPD_ADDR, offset | PCI_VPD_ADDR_F);
-
-		rc = sky2_vpd_wait(hw, cap, PCI_VPD_ADDR_F);
-		if (rc)
-			break;
-	}
-	return rc;
-}
-
-static int sky2_get_eeprom(struct net_device *dev, struct ethtool_eeprom *eeprom,
-			   u8 *data)
-{
-	struct sky2_port *sky2 = netdev_priv(dev);
-	int cap = pci_find_capability(sky2->hw->pdev, PCI_CAP_ID_VPD);
-
-	if (!cap)
-		return -EINVAL;
-
-	eeprom->magic = SKY2_EEPROM_MAGIC;
-
-	return sky2_vpd_read(sky2->hw, cap, data, eeprom->offset, eeprom->len);
-}
-
-static int sky2_set_eeprom(struct net_device *dev, struct ethtool_eeprom *eeprom,
-			   u8 *data)
-{
-	struct sky2_port *sky2 = netdev_priv(dev);
-	int cap = pci_find_capability(sky2->hw->pdev, PCI_CAP_ID_VPD);
-
-	if (!cap)
-		return -EINVAL;
-
-	if (eeprom->magic != SKY2_EEPROM_MAGIC)
-		return -EINVAL;
-
-	/* Partial writes not supported */
-	if ((eeprom->offset & 3) || (eeprom->len & 3))
-		return -EINVAL;
-
-	return sky2_vpd_write(sky2->hw, cap, data, eeprom->offset, eeprom->len);
-}
-
-
 static const struct ethtool_ops sky2_ethtool_ops = {
 	.get_settings	= sky2_get_settings,
 	.set_settings	= sky2_set_settings,
@@ -3837,9 +3732,6 @@ static const struct ethtool_ops sky2_eth
 	.get_regs_len	= sky2_get_regs_len,
 	.get_regs	= sky2_get_regs,
 	.get_link	= ethtool_op_get_link,
-	.get_eeprom_len	= sky2_get_eeprom_len,
-	.get_eeprom	= sky2_get_eeprom,
-	.set_eeprom	= sky2_set_eeprom,
 	.set_sg 	= ethtool_op_set_sg,
 	.set_tx_csum	= sky2_set_tx_csum,
 	.set_tso	= sky2_set_tso,

-- 

[PATCH 7/9] skge: set VPD size

[Stephen Hemminger]

[-- Attachment #1: skge-vpd-size.patch --]
[-- Type: text/plain, Size: 1129 bytes --]

Read configuration register during probe and use it to size the
available VPD.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/skge.c	2008-09-09 10:08:36.000000000 -0700
+++ b/drivers/net/skge.c	2008-09-09 11:19:31.000000000 -0700
@@ -3884,6 +3884,7 @@ static int __devinit skge_probe(struct p
 	struct net_device *dev, *dev1;
 	struct skge_hw *hw;
 	int err, using_dac = 0;
+	u32 reg;
 
 	err = pci_enable_device(pdev);
 	if (err) {
@@ -3912,15 +3913,17 @@ static int __devinit skge_probe(struct p
 		goto err_out_free_regions;
 	}
 
+	pci_read_config_dword(pdev, PCI_DEV_REG2, &reg);
+
+	/* set size of available VPD, only impacts sysfs */
+	err = pci_vpd_truncate(pdev, 1ul << (((reg & PCI_VPD_ROM_SZ) >> 14) + 8));
+	if (err)
+		dev_warn(&pdev->dev, "Can't set VPD size\n");
+
 #ifdef __BIG_ENDIAN
 	/* byte swap descriptors in hardware */
-	{
-		u32 reg;
-
-		pci_read_config_dword(pdev, PCI_DEV_REG2, &reg);
-		reg |= PCI_REV_DESC;
-		pci_write_config_dword(pdev, PCI_DEV_REG2, reg);
-	}
+	reg |= PCI_REV_DESC;
+	pci_write_config_dword(pdev, PCI_DEV_REG2, reg);
 #endif
 
 	err = -ENOMEM;

-- 

[PATCH 8/9] skge: add VPD display into debug interface

[Stephen Hemminger]

[-- Attachment #1: skge-vpd-debug.patch --]
[-- Type: text/plain, Size: 2844 bytes --]

The VPD stuff has more data and useful only when debugging.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/skge.c	2008-09-09 11:19:31.000000000 -0700
+++ b/drivers/net/skge.c	2008-09-09 11:19:33.000000000 -0700
@@ -3656,19 +3656,102 @@ static int skge_reset(struct skge_hw *hw
 
 static struct dentry *skge_debug;
 
+/*
+ * Read and parse the first part of Vital Product Data
+ */
+#define VPD_SIZE	128
+#define VPD_MAGIC	0x82
+
+static const struct vpd_tag {
+	char tag[2];
+	char *label;
+} vpd_tags[] = {
+	{ "PN",	"Part Number" },
+	{ "EC", "Engineering Level" },
+	{ "MN", "Manufacturer" },
+	{ "SN", "Serial Number" },
+	{ "YA", "Asset Tag" },
+	{ "VL", "First Error Log Message" },
+	{ "VF", "Second Error Log Message" },
+	{ "VB", "Boot Agent ROM Configuration" },
+	{ "VE", "EFI UNDI Configuration" },
+};
+
+static void skge_show_vpd(struct seq_file *seq, struct skge_hw *hw)
+{
+	size_t vpd_size;
+	loff_t offs;
+	u8 len;
+	unsigned char *buf;
+	u32 reg2;
+
+	pci_read_config_dword(hw->pdev, PCI_DEV_REG2, &reg2);
+	vpd_size = 1 << ( ((reg2 & PCI_VPD_ROM_SZ) >> 14) + 8);
+
+	seq_printf(seq, "%s Product Data\n", pci_name(hw->pdev));
+	buf = kmalloc(vpd_size, GFP_KERNEL);
+	if (!buf) {
+		seq_puts(seq, "no memory!\n");
+		return;
+	}
+
+	if (pci_read_vpd(hw->pdev, 0, vpd_size, buf) < 0) {
+		seq_puts(seq, "VPD read failed\n");
+		goto out;
+	}
+
+	if (buf[0] != VPD_MAGIC) {
+		seq_printf(seq, "VPD tag mismatch: %#x\n", buf[0]);
+		goto out;
+	}
+	len = buf[1];
+	if (len == 0 || len > vpd_size - 4) {
+		seq_printf(seq, "Invalid id length: %d\n", len);
+		goto out;
+	}
+
+	seq_printf(seq, "%.*s\n", len, buf + 3);
+	offs = len + 3;
+
+	while (offs < vpd_size - 4) {
+		int i;
+
+		if (!memcmp("RW", buf + offs, 2))	/* end marker */
+			break;
+		len = buf[offs + 2];
+		if (offs + len + 3 >= vpd_size)
+			break;
+
+		for (i = 0; i < ARRAY_SIZE(vpd_tags); i++) {
+			if (!memcmp(vpd_tags[i].tag, buf + offs, 2)) {
+				seq_printf(seq, " %s: %.*s\n",
+					   vpd_tags[i].label, len, buf + offs + 3);
+				break;
+			}
+		}
+		offs += len + 3;
+	}
+out:
+	kfree(buf);
+}
+
 static int skge_debug_show(struct seq_file *seq, void *v)
 {
 	struct net_device *dev = seq->private;
 	const struct skge_port *skge = netdev_priv(dev);
-	const struct skge_hw *hw = skge->hw;
+	struct skge_hw *hw = skge->hw;
 	const struct skge_element *e;
 
-	if (!netif_running(dev))
-		return -ENETDOWN;
+	skge_show_vpd(seq, hw);
 
 	seq_printf(seq, "IRQ src=%x mask=%x\n", skge_read32(hw, B0_ISRC),
 		   skge_read32(hw, B0_IMSK));
 
+	if (!netif_running(dev)) {
+		seq_printf(seq, "network not running\n");
+		return 0;
+	}
+
 	seq_printf(seq, "Tx Ring: (%d)\n", skge_avail(&skge->tx_ring));
 	for (e = skge->tx_ring.to_clean; e != skge->tx_ring.to_use; e = e->next) {
 		const struct skge_tx_desc *t = e->desc;

-- 

Re: [PATCH 8/9] skge: add VPD display into debug interface

[Jeff Garzik]

Stephen Hemminger wrote:
> The VPD stuff has more data and useful only when debugging.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

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

for all the net driver pieces.

[PATCH 9/9] skge: remove VPD eeprom

[Stephen Hemminger]

[-- Attachment #1: skge-no-eeprom.patch --]
[-- Type: text/plain, Size: 3510 bytes --]

The VPD is not really the firmware EEPROM.
Programming the VPD through the ethtool interface doesn't work
(doesn't really change firmware). Now that the interesting bits are
available through the debug interface and raw data is available through sysfs,
the old interface can go.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/skge.c	2008-09-09 11:19:33.000000000 -0700
+++ b/drivers/net/skge.c	2008-09-09 11:19:34.000000000 -0700
@@ -61,9 +61,6 @@
 #define BLINK_MS		250
 #define LINK_HZ			HZ
 
-#define SKGE_EEPROM_MAGIC	0x9933aabb
-
-
 MODULE_DESCRIPTION("SysKonnect Gigabit Ethernet driver");
 MODULE_AUTHOR("Stephen Hemminger <shemminger@linux-foundation.org>");
 MODULE_LICENSE("GPL");
@@ -803,98 +800,6 @@ static int skge_phys_id(struct net_devic
 	return 0;
 }
 
-static int skge_get_eeprom_len(struct net_device *dev)
-{
-	struct skge_port *skge = netdev_priv(dev);
-	u32 reg2;
-
-	pci_read_config_dword(skge->hw->pdev, PCI_DEV_REG2, &reg2);
-	return 1 << ( ((reg2 & PCI_VPD_ROM_SZ) >> 14) + 8);
-}
-
-static u32 skge_vpd_read(struct pci_dev *pdev, int cap, u16 offset)
-{
-	u32 val;
-
-	pci_write_config_word(pdev, cap + PCI_VPD_ADDR, offset);
-
-	do {
-		pci_read_config_word(pdev, cap + PCI_VPD_ADDR, &offset);
-	} while (!(offset & PCI_VPD_ADDR_F));
-
-	pci_read_config_dword(pdev, cap + PCI_VPD_DATA, &val);
-	return val;
-}
-
-static void skge_vpd_write(struct pci_dev *pdev, int cap, u16 offset, u32 val)
-{
-	pci_write_config_dword(pdev, cap + PCI_VPD_DATA, val);
-	pci_write_config_word(pdev, cap + PCI_VPD_ADDR,
-			      offset | PCI_VPD_ADDR_F);
-
-	do {
-		pci_read_config_word(pdev, cap + PCI_VPD_ADDR, &offset);
-	} while (offset & PCI_VPD_ADDR_F);
-}
-
-static int skge_get_eeprom(struct net_device *dev, struct ethtool_eeprom *eeprom,
-			   u8 *data)
-{
-	struct skge_port *skge = netdev_priv(dev);
-	struct pci_dev *pdev = skge->hw->pdev;
-	int cap = pci_find_capability(pdev, PCI_CAP_ID_VPD);
-	int length = eeprom->len;
-	u16 offset = eeprom->offset;
-
-	if (!cap)
-		return -EINVAL;
-
-	eeprom->magic = SKGE_EEPROM_MAGIC;
-
-	while (length > 0) {
-		u32 val = skge_vpd_read(pdev, cap, offset);
-		int n = min_t(int, length, sizeof(val));
-
-		memcpy(data, &val, n);
-		length -= n;
-		data += n;
-		offset += n;
-	}
-	return 0;
-}
-
-static int skge_set_eeprom(struct net_device *dev, struct ethtool_eeprom *eeprom,
-			   u8 *data)
-{
-	struct skge_port *skge = netdev_priv(dev);
-	struct pci_dev *pdev = skge->hw->pdev;
-	int cap = pci_find_capability(pdev, PCI_CAP_ID_VPD);
-	int length = eeprom->len;
-	u16 offset = eeprom->offset;
-
-	if (!cap)
-		return -EINVAL;
-
-	if (eeprom->magic != SKGE_EEPROM_MAGIC)
-		return -EINVAL;
-
-	while (length > 0) {
-		u32 val;
-		int n = min_t(int, length, sizeof(val));
-
-		if (n < sizeof(val))
-			val = skge_vpd_read(pdev, cap, offset);
-		memcpy(&val, data, n);
-
-		skge_vpd_write(pdev, cap, offset, val);
-
-		length -= n;
-		data += n;
-		offset += n;
-	}
-	return 0;
-}
-
 static const struct ethtool_ops skge_ethtool_ops = {
 	.get_settings	= skge_get_settings,
 	.set_settings	= skge_set_settings,
@@ -907,9 +812,6 @@ static const struct ethtool_ops skge_eth
 	.set_msglevel	= skge_set_msglevel,
 	.nway_reset	= skge_nway_reset,
 	.get_link	= ethtool_op_get_link,
-	.get_eeprom_len	= skge_get_eeprom_len,
-	.get_eeprom	= skge_get_eeprom,
-	.set_eeprom	= skge_set_eeprom,
 	.get_ringparam	= skge_get_ring_param,
 	.set_ringparam	= skge_set_ring_param,
 	.get_pauseparam = skge_get_pauseparam,

--