[PATCH 0/3] PCI VPD changes

[PATCH 0/3] PCI VPD changes

[Stephen Hemminger]

Slightly revised from my last set. Allow mutex to be killed, and
just use yield.

-- 

[PATCH 1/3] PCI: vpd handle longer delays in access

[Stephen Hemminger]

[-- Attachment #1: vpd-delay.patch --]
[-- Type: text/plain, Size: 3108 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 SysKonnect 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 longer timeout
  * call schedule while spinning to provide some responsivness

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


--- a/drivers/pci/access.c	2008-09-04 09:06:51.000000000 -0700
+++ b/drivers/pci/access.c	2008-09-04 10:16:52.000000000 -0700
@@ -133,7 +133,7 @@ PCI_USER_WRITE_CONFIG(dword, u32)
 
 struct pci_vpd_pci22 {
 	struct pci_vpd base;
-	spinlock_t lock; /* controls access to hardware and the flags */
+	struct mutex lock;
 	u8	cap;
 	bool	busy;
 	bool	flag; /* value of F bit to wait for */
@@ -144,29 +144,33 @@ static int pci_vpd_pci22_wait(struct pci
 {
 	struct pci_vpd_pci22 *vpd =
 		container_of(dev->vpd, struct pci_vpd_pci22, base);
-	u16 flag, status;
-	int wait;
+	u16 flag = vpd->flag ? PCI_VPD_ADDR_F : 0;
+	unsigned long timeout = jiffies + (vpd->flag ? HZ/50 : HZ/10);
+	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)
-			return ret;
+			break;
+
 		if ((status & PCI_VPD_ADDR_F) == flag) {
 			vpd->busy = false;
-			return 0;
+			break;
 		}
-		if (wait-- == 0)
+
+		if (time_after(jiffies, timeout))
 			return -ETIMEDOUT;
-		udelay(10);
+		if (signal_pending(current))
+			return -EINTR;
+		yield();
 	}
+
+	return ret;
 }
 
 static int pci_vpd_pci22_read(struct pci_dev *dev, int pos, int size,
@@ -183,7 +187,9 @@ static int pci_vpd_pci22_read(struct pci
 	if (size == 0)
 		return 0;
 
-	spin_lock_irq(&vpd->lock);
+	ret = mutex_lock_killable(&vpd->lock);
+	if (ret)
+		return ret;
 	ret = pci_vpd_pci22_wait(dev);
 	if (ret < 0)
 		goto out;
@@ -199,7 +205,7 @@ static int pci_vpd_pci22_read(struct pci
 	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;
 
@@ -231,7 +237,9 @@ static int pci_vpd_pci22_write(struct pc
 	val |= ((u8) *buf++) << 16;
 	val |= ((u32)(u8) *buf++) << 24;
 
-	spin_lock_irq(&vpd->lock);
+	ret = mutex_lock_killable(&vpd->lock);
+	if (ret)
+		return ret;
 	ret = pci_vpd_pci22_wait(dev);
 	if (ret < 0)
 		goto out;
@@ -247,7 +255,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 +287,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;

-- 

[PATCH 2/3] PCI: revise VPD access interface

[Stephen Hemminger]

[-- Attachment #1: vpd-ops.patch --]
[-- Type: text/plain, Size: 9079 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
   * cleanup types of arguments
   * add exportable wrapper

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

--- a/drivers/pci/access.c	2008-09-04 10:16:52.000000000 -0700
+++ b/drivers/pci/access.c	2008-09-04 10:19:47.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
+ *
+ */
+int 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
+ *
+ */
+int 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
@@ -173,93 +206,97 @@ static int pci_vpd_pci22_wait(struct pci
 	return ret;
 }
 
-static int pci_vpd_pci22_read(struct pci_dev *dev, int pos, int size,
-			      char *buf)
+static int pci_vpd_pci22_read(struct pci_dev *dev, loff_t pos, size_t count,
+			      void *buf)
 {
 	struct pci_vpd_pci22 *vpd =
 		container_of(dev->vpd, struct pci_vpd_pci22, base);
-	u32 val;
 	int ret;
-	int begin, end, i;
+	loff_t end = pos + count;
+	unsigned char *p = buf;
 
-	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;
 
 	ret = mutex_lock_killable(&vpd->lock);
 	if (ret)
 		return ret;
-	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 = 1;
-	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:
-	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;
+	while (pos < end) {
+		u32 val;
+		unsigned int i, skip;
+
+		ret = pci_vpd_pci22_wait(dev);
+		if (ret < 0)
+			break;
+
+		ret = pci_user_write_config_word(dev, vpd->cap + PCI_VPD_ADDR,
+						 pos & ~3);
+		if (ret < 0)
+			break;
+		vpd->busy = true;
+		vpd->flag = 1;
+		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) {
+				*p++ = val;
+				if (++pos == end)
+					break;
+			}
+			val >>= 8;
+		}
 	}
-	return end - begin;
+	mutex_unlock(&vpd->lock);
+	return ret ? ret : count;
 }
 
-static int pci_vpd_pci22_write(struct pci_dev *dev, int pos, int size,
-			       const char *buf)
+static int pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count,
+			       const void *buf)
 {
 	struct pci_vpd_pci22 *vpd =
 		container_of(dev->vpd, struct pci_vpd_pci22, base);
-	u32 val;
+	loff_t end = pos + count;
 	int ret;
 
-	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;
-
 	ret = mutex_lock_killable(&vpd->lock);
 	if (ret)
 		return ret;
-	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);
-out:
-	mutex_unlock(&vpd->lock);
-	if (ret < 0)
-		return ret;
 
-	return 4;
+	while (pos < end) {
+		u32 val;
+
+		ret = pci_vpd_pci22_wait(dev);
+		if (ret < 0)
+			break;
+		memcpy(&val, buf, sizeof(u32));
+
+		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);
+
+		buf += sizeof(u32);
+		pos += sizeof(u32);
+	}
+	mutex_unlock(&vpd->lock);
+	return ret ? ret : count;
 }
 
 static void pci_vpd_pci22_release(struct pci_dev *dev)
@@ -267,7 +304,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-04 10:14:59.000000000 -0700
+++ b/drivers/pci/pci.h	2008-09-04 10:17:01.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);
+	int (*read)(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
+	int (*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-04 10:14:59.000000000 -0700
+++ b/drivers/pci/pci-sysfs.c	2008-09-04 10:17:01.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-04 10:14:59.000000000 -0700
+++ b/include/linux/pci.h	2008-09-04 10:17:01.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 */
+int pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
+int 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);

-- 

[PATCH 3/3] PCI: add interface to set visible size of VPD

[Stephen Hemminger]

[-- Attachment #1: vpd-size.patch --]
[-- Type: text/plain, Size: 1460 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>

--- a/drivers/pci/access.c	2008-09-04 11:29:22.000000000 -0700
+++ b/drivers/pci/access.c	2008-09-04 11:38:29.000000000 -0700
@@ -332,6 +332,24 @@ int pci_vpd_pci22_init(struct pci_dev *d
 }
 
 /**
+ * pci_vpd_size - Set available Vital Product Data size
+ * @dev:	pci device struct
+ * @size:	available memory in bytes
+ *
+ * Adjust size of available VPD area.
+ */
+int pci_vpd_size(struct pci_dev *dev, size_t size)
+{
+	if (!dev->vpd)
+		return -EINVAL;
+	if (size > PCI_VPD_PCI22_SIZE)
+		return -EINVAL;
+	dev->vpd->len = size;
+	return 0;
+}
+EXPORT_SYMBOL(pci_vpd_size);
+
+/**
  * pci_block_user_cfg_access - Block userspace PCI config reads/writes
  * @dev:	pci device struct
  *
--- a/include/linux/pci.h	2008-09-04 11:28:45.000000000 -0700
+++ b/include/linux/pci.h	2008-09-04 11:29:15.000000000 -0700
@@ -653,6 +653,7 @@ int pci_bus_find_capability(struct pci_b
 /* Vital product data routines */
 int pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
 int pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
+int pci_vpd_size(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 1/3] PCI: vpd handle longer delays in access

[Peter Zijlstra]

On Thu, 2008-09-04 at 13:56 -0700, Stephen Hemminger wrote:
> plain text document attachment (vpd-delay.patch)
> Accessing the VPD area can take a long time.  The existing 
> VPD access code fails consistently on my hardware. There are comments
> in the SysKonnect 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 longer timeout
>   * call schedule while spinning to provide some responsivness
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> 
> --- a/drivers/pci/access.c	2008-09-04 09:06:51.000000000 -0700
> +++ b/drivers/pci/access.c	2008-09-04 10:16:52.000000000 -0700
> @@ -133,7 +133,7 @@ PCI_USER_WRITE_CONFIG(dword, u32)
>  
>  struct pci_vpd_pci22 {
>  	struct pci_vpd base;
> -	spinlock_t lock; /* controls access to hardware and the flags */
> +	struct mutex lock;
>  	u8	cap;
>  	bool	busy;
>  	bool	flag; /* value of F bit to wait for */
> @@ -144,29 +144,33 @@ static int pci_vpd_pci22_wait(struct pci
>  {
>  	struct pci_vpd_pci22 *vpd =
>  		container_of(dev->vpd, struct pci_vpd_pci22, base);
> -	u16 flag, status;
> -	int wait;
> +	u16 flag = vpd->flag ? PCI_VPD_ADDR_F : 0;
> +	unsigned long timeout = jiffies + (vpd->flag ? HZ/50 : HZ/10);
> +	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)
> -			return ret;
> +			break;
> +
>  		if ((status & PCI_VPD_ADDR_F) == flag) {
>  			vpd->busy = false;
> -			return 0;
> +			break;
>  		}
> -		if (wait-- == 0)
> +
> +		if (time_after(jiffies, timeout))
>  			return -ETIMEDOUT;
> -		udelay(10);
> +		if (signal_pending(current))
> +			return -EINTR;
> +		yield();

At the very least put a big comment in here that we're polling the
hardware without completion event.

yield() is not good either, when used with a RT task (IMHO still the
only valid use of yield) it mostly doesn't spend any time away from this
task at all.

The other option, schedule_hrtimeout() isn't ideal either because on
crappy hardware it will fall back to jiffie based timeouts and I _hope_
that most hardware is less shitty than the 13ms reported in the
changelog.

Can we make this a per device delay that starts out small (the previous
10 us sound good) and gets doubled every once in a while when not enough
for a particular device?

That way we can - at some threshold - switch to sleeping delays..

>  	}
> +
> +	return ret;
>  }

Re: [PATCH 2/3] PCI: revise VPD access interface

[Ben Hutchings]

Stephen Hemminger wrote:
> Change PCI VPD API which was only used by sysfs to something usable
> in drivers. 
>    * move iteration over multiple words to the low level
>    * cleanup types of arguments
>    * add exportable wrapper
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> --- a/drivers/pci/access.c	2008-09-04 10:16:52.000000000 -0700
> +++ b/drivers/pci/access.c	2008-09-04 10:19:47.000000000 -0700
[...]
> -static int pci_vpd_pci22_write(struct pci_dev *dev, int pos, int size,
> -			       const char *buf)
> +static int pci_vpd_pci22_write(struct pci_dev *dev, loff_t pos, size_t count,
> +			       const void *buf)
>  {
>  	struct pci_vpd_pci22 *vpd =
>  		container_of(dev->vpd, struct pci_vpd_pci22, base);
> -	u32 val;
> +	loff_t end = pos + count;
>  	int ret;
>  
> -	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;
> -
>  	ret = mutex_lock_killable(&vpd->lock);
>  	if (ret)
>  		return ret;
> -	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);
> -out:
> -	mutex_unlock(&vpd->lock);
> -	if (ret < 0)
> -		return ret;
>  
> -	return 4;
> +	while (pos < end) {
> +		u32 val;
> +
> +		ret = pci_vpd_pci22_wait(dev);
> +		if (ret < 0)
> +			break;
> +		memcpy(&val, buf, sizeof(u32));
[...]

I'm not sure this is correct.  pci_user_write_config_dword() expects a
value in host byte order, but this memcpy() makes val always little-endian.

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.

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

[Ben Hutchings]

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>
> 
> --- a/drivers/pci/access.c	2008-09-04 11:29:22.000000000 -0700
> +++ b/drivers/pci/access.c	2008-09-04 11:38:29.000000000 -0700
> @@ -332,6 +332,24 @@ int pci_vpd_pci22_init(struct pci_dev *d
>  }
>  
>  /**
> + * pci_vpd_size - Set available Vital Product Data size
> + * @dev:	pci device struct
> + * @size:	available memory in bytes
> + *
> + * Adjust size of available VPD area.
> + */
> +int pci_vpd_size(struct pci_dev *dev, size_t size)
> +{
> +	if (!dev->vpd)
> +		return -EINVAL;
> +	if (size > PCI_VPD_PCI22_SIZE)
> +		return -EINVAL;

This assumes that PCI 2.2 VPD is the only VPD implementation (which is true
at the moment, but at least PCI 2.1 VPD should be added).  Maybe this should
compare with the current length instead?

Ben.

> +	dev->vpd->len = size;
> +	return 0;
> +}
> +EXPORT_SYMBOL(pci_vpd_size);
> +
> +/**
>   * pci_block_user_cfg_access - Block userspace PCI config reads/writes
>   * @dev:	pci device struct
>   *
> --- a/include/linux/pci.h	2008-09-04 11:28:45.000000000 -0700
> +++ b/include/linux/pci.h	2008-09-04 11:29:15.000000000 -0700
> @@ -653,6 +653,7 @@ int pci_bus_find_capability(struct pci_b
>  /* Vital product data routines */
>  int pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
>  int pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
> +int pci_vpd_size(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);
> 
> -- 
> 

-- 
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.

Re: [PATCH 1/3] PCI: vpd handle longer delays in access

[Matthew Wilcox]

On Thu, Sep 04, 2008 at 01:56:37PM -0700, Stephen Hemminger wrote:
> -		udelay(10);
> +		if (signal_pending(current))
> +			return -EINTR;

If you're going to use _killable instead of _interruptible, this needs
to be fatal_signal_pending().  Otherwise the one who owns the lock can
be interrupted by _any_ signal while those waiting for the lock can only
be interrupted by fatal signals.  Which seems daft to me.

> -	spin_lock_irq(&vpd->lock);
> +	ret = mutex_lock_killable(&vpd->lock);
> +	if (ret)
> +		return ret;

What's wrong with the shorter:

	if (mutex_lock_killable(&vpd->lock))
		return -EINTR;
?

The actual error is irrelevant here since userspace will never consume it.

(I agree with Peter about use of yield())

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

Re: [PATCH 1/3] PCI: vpd handle longer delays in access

[Andrew Morton]

On Thu, 04 Sep 2008 13:56:37 -0700
Stephen Hemminger <shemminger@vyatta.com> wrote:

> Accessing the VPD area can take a long time.  The existing 
> VPD access code fails consistently on my hardware. There are comments
> in the SysKonnect 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 longer timeout
>   * call schedule while spinning to provide some responsivness

It doesn't call schedule() - it calls yield().

yield() is pretty notoriously badly behaved in the presence of lots of
runnable tasks and there's been a general move to eradicate its
in-kernel callsites.

An alternative would be nice.

Re: [PATCH 2/3] PCI: revise VPD access interface

[Andrew Morton]

On Thu, 04 Sep 2008 13:56:38 -0700
Stephen Hemminger <shemminger@vyatta.com> wrote:

> Change PCI VPD API which was only used by sysfs to something usable
> in drivers. 
>    * move iteration over multiple words to the low level
>    * cleanup types of arguments
>    * add exportable wrapper
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> --- a/drivers/pci/access.c	2008-09-04 10:16:52.000000000 -0700
> +++ b/drivers/pci/access.c	2008-09-04 10:19:47.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
> + *
> + */
> +int 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);

"read" functions normally return ssize_t, not int.

>
> ...
>  
>  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)
>  {

This returns size_t.

>  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)

as does this.

> @@ -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;

But this (I think) is assigning a ssize_t-returning function to an
int-returning function pointer.

Re: [PATCH 1/3] PCI: vpd handle longer delays in access

[Stephen Hemminger]

On Mon, 8 Sep 2008 13:40:25 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu, 04 Sep 2008 13:56:37 -0700
> Stephen Hemminger <shemminger@vyatta.com> wrote:
> 
> > Accessing the VPD area can take a long time.  The existing 
> > VPD access code fails consistently on my hardware. There are comments
> > in the SysKonnect 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 longer timeout
> >   * call schedule while spinning to provide some responsivness
> 
> It doesn't call schedule() - it calls yield().
> 
> yield() is pretty notoriously badly behaved in the presence of lots of
> runnable tasks and there's been a general move to eradicate its
> in-kernel callsites.
> 
> An alternative would be nice.

What is a good way to say "i am polling for a while"?

Re: [PATCH 1/3] PCI: vpd handle longer delays in access

[Arjan van de Ven]

On Mon, 8 Sep 2008 14:08:24 -0700
Stephen Hemminger <shemminger@vyatta.com> wrote:

> 
> What is a good way to say "i am polling for a while"?


can you take a fixed time delay?

or use cond_resched()


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH 1/3] PCI: vpd handle longer delays in access

[Stephen Hemminger]

On Mon, 8 Sep 2008 14:26:05 -0700
Arjan van de Ven <arjan@infradead.org> wrote:

> On Mon, 8 Sep 2008 14:08:24 -0700
> Stephen Hemminger <shemminger@vyatta.com> wrote:
> 
> > 
> > What is a good way to say "i am polling for a while"?
> 
> 
> can you take a fixed time delay?
> 
> or use cond_resched()
cond_resched sounds like the best suggestion.  The problem is that this
code needs to deal with hardware that could be fast, or slow depending
on how the device is implemented.  

Re: [PATCH 1/3] PCI: vpd handle longer delays in access

[Arjan van de Ven]

On Tue, 9 Sep 2008 09:55:48 -0700
Stephen Hemminger <shemminger@vyatta.com> wrote:

> On Mon, 8 Sep 2008 14:26:05 -0700
> Arjan van de Ven <arjan@infradead.org> wrote:
> 
> > On Mon, 8 Sep 2008 14:08:24 -0700
> > Stephen Hemminger <shemminger@vyatta.com> wrote:
> > 
> > > 
> > > What is a good way to say "i am polling for a while"?
> > 
> > 
> > can you take a fixed time delay?
> > 
> > or use cond_resched()
> cond_resched sounds like the best suggestion.  The problem is that
> this code needs to deal with hardware that could be fast, or slow
> depending on how the device is implemented.  

one option is poll fast for <X jiffies> and then back off with sleeping
delays



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org