[PATCH 1/2] sky2: EEPROM read/write bug fixes

[PATCH 1/2] sky2: EEPROM read/write bug fixes

[Stephen Hemminger]

Cleanup and harden the routines accessing the EEPROM.
  1. Prevent spin forever waiting for the TWSI bus
  2. Fix write eeprom to write full words rather than only 16 bits 
    Luckly the vendor doesn't provide EEPROM in Linux format so it must never
    have been used.
  3. Don't allow partial eeprom writes, not needed, not safe.

These are non-urgent bug fixes.

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


--- a/drivers/net/sky2.c	2008-08-27 19:15:52.000000000 -0700
+++ b/drivers/net/sky2.c	2008-08-27 19:21:13.000000000 -0700
@@ -3732,27 +3732,63 @@ static int sky2_get_eeprom_len(struct ne
 	return 1 << ( ((reg2 & PCI_VPD_ROM_SZ) >> 14) + 8);
 }
 
-static u32 sky2_vpd_read(struct sky2_hw *hw, int cap, u16 offset)
+static int sky2_vpd_wait(const struct sky2_hw *hw, int cap, u16 busy)
 {
-	u32 val;
+	unsigned long start = jiffies;
 
-	sky2_pci_write16(hw, cap + PCI_VPD_ADDR, offset);
+	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;
+}
 
-	do {
-		offset = sky2_pci_read16(hw, cap + PCI_VPD_ADDR);
-	} while (!(offset & PCI_VPD_ADDR_F));
+static int sky2_vpd_read(struct sky2_hw *hw, int cap, void *data,
+			 u16 offset, size_t length)
+{
+	int rc = 0;
 
-	val = sky2_pci_read32(hw, cap + PCI_VPD_DATA);
-	return val;
+	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 void sky2_vpd_write(struct sky2_hw *hw, int cap, u16 offset, u32 val)
+static int sky2_vpd_write(struct sky2_hw *hw, int cap, const void *data,
+			  u16 offset, unsigned int length)
 {
-	sky2_pci_write16(hw, cap + PCI_VPD_DATA, val);
-	sky2_pci_write32(hw, cap + PCI_VPD_ADDR, offset | PCI_VPD_ADDR_F);
-	do {
-		offset = sky2_pci_read16(hw, cap + PCI_VPD_ADDR);
-	} while (offset & PCI_VPD_ADDR_F);
+	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,
@@ -3760,24 +3796,13 @@ static int sky2_get_eeprom(struct net_de
 {
 	struct sky2_port *sky2 = netdev_priv(dev);
 	int cap = pci_find_capability(sky2->hw->pdev, PCI_CAP_ID_VPD);
-	int length = eeprom->len;
-	u16 offset = eeprom->offset;
 
 	if (!cap)
 		return -EINVAL;
 
 	eeprom->magic = SKY2_EEPROM_MAGIC;
 
-	while (length > 0) {
-		u32 val = sky2_vpd_read(sky2->hw, cap, offset);
-		int n = min_t(int, length, sizeof(val));
-
-		memcpy(data, &val, n);
-		length -= n;
-		data += n;
-		offset += n;
-	}
-	return 0;
+	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,
@@ -3785,8 +3810,6 @@ static int sky2_set_eeprom(struct net_de
 {
 	struct sky2_port *sky2 = netdev_priv(dev);
 	int cap = pci_find_capability(sky2->hw->pdev, PCI_CAP_ID_VPD);
-	int length = eeprom->len;
-	u16 offset = eeprom->offset;
 
 	if (!cap)
 		return -EINVAL;
@@ -3794,21 +3817,11 @@ static int sky2_set_eeprom(struct net_de
 	if (eeprom->magic != SKY2_EEPROM_MAGIC)
 		return -EINVAL;
 
-	while (length > 0) {
-		u32 val;
-		int n = min_t(int, length, sizeof(val));
-
-		if (n < sizeof(val))
-			val = sky2_vpd_read(sky2->hw, cap, offset);
-		memcpy(&val, data, n);
-
-		sky2_vpd_write(sky2->hw, cap, offset, val);
+	/* Partial writes not supported */
+	if ((eeprom->offset & 3) || (eeprom->len & 3))
+		return -EINVAL;
 
-		length -= n;
-		data += n;
-		offset += n;
-	}
-	return 0;
+	return sky2_vpd_write(sky2->hw, cap, data, eeprom->offset, eeprom->len);
 }
 
 

[PATCH 2/2] sky2: display product info on boot.

[Stephen Hemminger]

Change bootup messages to print more information. This is to help users
who may have old buggy EEPROM image.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
---
This can wait for 2.6.28

--- a/drivers/net/sky2.c	2008-08-27 19:21:52.000000000 -0700
+++ b/drivers/net/sky2.c	2008-08-27 19:21:56.000000000 -0700
@@ -4191,6 +4191,69 @@ 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)
 {
@@ -4289,13 +4352,13 @@ static int __devinit sky2_probe(struct p
 	if (err)
 		goto err_out_iounmap;
 
-	dev_info(&pdev->dev, "v%s addr 0x%llx irq %d Yukon-2 %s rev %d\n",
-		 DRV_VERSION, (unsigned long long)pci_resource_start(pdev, 0),
-		 pdev->irq, sky2_name(hw->chip_id, buf1, sizeof(buf1)),
-		 hw->chip_rev);
+	dev_info(&pdev->dev, "Yukon-2 %s chip revision %d\n",
+		 sky2_name(hw->chip_id, buf1, sizeof(buf1)), hw->chip_rev);
 
 	sky2_reset(hw);
 
+	sky2_vpd_info(hw);
+
 	dev = sky2_init_netdev(hw, 0, using_dac, wol_default);
 	if (!dev) {
 		err = -ENOMEM;
@@ -4546,6 +4609,8 @@ static struct pci_driver sky2_driver = {
 
 static int __init sky2_init_module(void)
 {
+	pr_info(PFX "driver version " DRV_VERSION "\n");
+
 	sky2_debug_init();
 	return pci_register_driver(&sky2_driver);
 }

Re: [PATCH 1/2] sky2: EEPROM read/write bug fixes

[Ben Hutchings]

Stephen Hemminger wrote:
> Cleanup and harden the routines accessing the EEPROM.
>   1. Prevent spin forever waiting for the TWSI bus
>   2. Fix write eeprom to write full words rather than only 16 bits 
>     Luckly the vendor doesn't provide EEPROM in Linux format so it must never
>     have been used.
>   3. Don't allow partial eeprom writes, not needed, not safe.
[...]

You should be able to replace the VPD access code with calls through
pci_dev->vpd->ops - though you'd need to remove some declarations from
drivers/pci/pci.h to include/linux/pci.h.

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 1/2] sky2: EEPROM read/write bug fixes

[Stephen Hemminger]

On Thu, 28 Aug 2008 12:13:25 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> Stephen Hemminger wrote:
> > Cleanup and harden the routines accessing the EEPROM.
> >   1. Prevent spin forever waiting for the TWSI bus
> >   2. Fix write eeprom to write full words rather than only 16 bits 
> >     Luckly the vendor doesn't provide EEPROM in Linux format so it must never
> >     have been used.
> >   3. Don't allow partial eeprom writes, not needed, not safe.
> [...]
> 
> You should be able to replace the VPD access code with calls through
> pci_dev->vpd->ops - though you'd need to remove some declarations from
> drivers/pci/pci.h to include/linux/pci.h.
> 
> Ben.
> 

Generically a good idea, but it won't work for this device.
It turns out that the read/write timeouts in pci/access.c are too
short. Since the pci vpd code spins under spin lock with irq's disabled,
it really can't wait for up to 10ms!

Minor note: the pci code seems to be much more verbose with little
gain in real functionality.

Re: [PATCH 1/2] sky2: EEPROM read/write bug fixes

[Stephen Hemminger]

On Thu, 28 Aug 2008 08:30:35 -0700
Stephen Hemminger <shemminger@linux-foundation.org> wrote:

> On Thu, 28 Aug 2008 12:13:25 +0100
> Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> > Stephen Hemminger wrote:
> > > Cleanup and harden the routines accessing the EEPROM.
> > >   1. Prevent spin forever waiting for the TWSI bus
> > >   2. Fix write eeprom to write full words rather than only 16 bits 
> > >     Luckly the vendor doesn't provide EEPROM in Linux format so it must never
> > >     have been used.
> > >   3. Don't allow partial eeprom writes, not needed, not safe.
> > [...]
> > 
> > You should be able to replace the VPD access code with calls through
> > pci_dev->vpd->ops - though you'd need to remove some declarations from
> > drivers/pci/pci.h to include/linux/pci.h.
> > 
> > Ben.
> > 
> 
> Generically a good idea, but it won't work for this device.
> It turns out that the read/write timeouts in pci/access.c are too
> short. Since the pci vpd code spins under spin lock with irq's disabled,
> it really can't wait for up to 10ms!

You can show that the pci->vpd code won't work because any access to
/sys/class/net/eth0/device/vpd gets ETIMEDOUT.

Re: [PATCH 1/2] sky2: EEPROM read/write bug fixes

[Ben Hutchings]

Stephen Hemminger wrote:
> On Thu, 28 Aug 2008 08:30:35 -0700
> Stephen Hemminger <shemminger@linux-foundation.org> wrote:
> 
> > On Thu, 28 Aug 2008 12:13:25 +0100
> > Ben Hutchings <bhutchings@solarflare.com> wrote:
> > 
> > > Stephen Hemminger wrote:
> > > > Cleanup and harden the routines accessing the EEPROM.
> > > >   1. Prevent spin forever waiting for the TWSI bus
> > > >   2. Fix write eeprom to write full words rather than only 16 bits 
> > > >     Luckly the vendor doesn't provide EEPROM in Linux format so it must never
> > > >     have been used.
> > > >   3. Don't allow partial eeprom writes, not needed, not safe.
> > > [...]
> > > 
> > > You should be able to replace the VPD access code with calls through
> > > pci_dev->vpd->ops - though you'd need to remove some declarations from
> > > drivers/pci/pci.h to include/linux/pci.h.
> > > 
> > > Ben.
> > > 
> > 
> > Generically a good idea, but it won't work for this device.
> > It turns out that the read/write timeouts in pci/access.c are too
> > short. Since the pci vpd code spins under spin lock with irq's disabled,
> > it really can't wait for up to 10ms!
> 
> You can show that the pci->vpd code won't work because any access to
> /sys/class/net/eth0/device/vpd gets ETIMEDOUT.

Then please change the time limit.  There is no time limit for VPD in the
PCI spec so I started with a value that I knew was enough for our devices.

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 1/2] sky2: EEPROM read/write bug fixes

[Stephen Hemminger]

Ben Hutchings wrote:
> Stephen Hemminger wrote:
>   
>> On Thu, 28 Aug 2008 08:30:35 -0700
>> Stephen Hemminger <shemminger@linux-foundation.org> wrote:
>>
>>     
>>> On Thu, 28 Aug 2008 12:13:25 +0100
>>> Ben Hutchings <bhutchings@solarflare.com> wrote:
>>>
>>>       
>>>> Stephen Hemminger wrote:
>>>>         
>>>>> Cleanup and harden the routines accessing the EEPROM.
>>>>>   1. Prevent spin forever waiting for the TWSI bus
>>>>>   2. Fix write eeprom to write full words rather than only 16 bits 
>>>>>     Luckly the vendor doesn't provide EEPROM in Linux format so it must never
>>>>>     have been used.
>>>>>   3. Don't allow partial eeprom writes, not needed, not safe.
>>>>>           
>>>> [...]
>>>>
>>>> You should be able to replace the VPD access code with calls through
>>>> pci_dev->vpd->ops - though you'd need to remove some declarations from
>>>> drivers/pci/pci.h to include/linux/pci.h.
>>>>
>>>> Ben.
>>>>
>>>>         
>>> Generically a good idea, but it won't work for this device.
>>> It turns out that the read/write timeouts in pci/access.c are too
>>> short. Since the pci vpd code spins under spin lock with irq's disabled,
>>> it really can't wait for up to 10ms!
>>>       
>> You can show that the pci->vpd code won't work because any access to
>> /sys/class/net/eth0/device/vpd gets ETIMEDOUT.
>>     
>
> Then please change the time limit.  There is no time limit for VPD in the
> PCI spec so I started with a value that I knew was enough for our devices.
>
> Ben.
>
>   
The time limit is part of the generic pci vpd ops reading code. Maybe if 
I have time, I'll have sky2 driver
overload the vpd->ops with its own ops, but not sure if it is really 
worth it.  What besides device/vpd uses
or plans to use vpd ops?

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

[Stephen Hemminger]

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-03 09:01:53.000000000 -0700
+++ b/drivers/pci/access.c	2008-09-03 11:47:41.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
@@ -170,89 +203,93 @@ 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;
+	int ret = 0;
+	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;
 
 	mutex_lock(&vpd->lock);
-	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);
+	while (pos < end) {
+		u32 val;
+		unsigned int i, skip;
+
+		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);
+		if (ret < 0)
+			goto out;
+
+		skip = pos & 3;
+		for (i = 0;  i < sizeof(u32); i++) {
+			if (i >= skip) {
+				*p++ = 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 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;
-	int ret;
+	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;
-
 	mutex_lock(&vpd->lock);
-	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;
+
+		ret = pci_vpd_pci22_wait(dev);
+		if (ret < 0)
+			goto out;
+		memcpy(&val, buf, sizeof(u32));
+
+		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);
+
+		buf += sizeof(u32);
+		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)
@@ -260,7 +297,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-03 08:58:03.000000000 -0700
+++ b/drivers/pci/pci.h	2008-09-03 10:47:52.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-03 09:17:19.000000000 -0700
+++ b/drivers/pci/pci-sysfs.c	2008-09-03 10:48:25.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-03 09:12:51.000000000 -0700
+++ b/include/linux/pci.h	2008-09-03 10:38:52.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] sky2: use pci_read_vpd to read info during boot

[Stephen Hemminger]

Change sky2 driver (in netdev next tree) to use vpd access routines.

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

Note: other usage of vpd internal access routines will go away in later patches.

---
Patch against netdev-2.6#upstream-next assumes the previous PCI API change.

--- a/drivers/net/sky2.c	2008-09-03 11:35:47.000000000 -0700
+++ b/drivers/net/sky2.c	2008-09-03 11:44:06.000000000 -0700
@@ -4199,10 +4199,9 @@ static int __devinit pci_wake_enabled(st
 
 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;
+	loff_t offs;
+	u8 len;
+	u8 tag[3];
 	static struct vpd_tag {
 		char tag[2];
 		char *label;
@@ -4211,47 +4210,48 @@ static void __devinit sky2_vpd_info(stru
 		{ "EC", "Engineering Level" },
 		{ "MN", "Manufacturer" },
 	};
+	char str[128];
 
-	if (!cap)
-		goto out;
-
-	vpd_buf = kmalloc(VPD_SIZE, GFP_KERNEL);
-	if (!vpd_buf)
-		goto out;
+	if (pci_read_vpd(hw->pdev, 0, sizeof(tag), tag) < 0)
+		return;
+	if (tag[0] != VPD_MAGIC)
+		return;
+	len = tag[1];
+	if (len == 0 || len > sizeof(str))
+		return;
 
-	if (sky2_vpd_read(hw, cap, vpd_buf, 0, VPD_SIZE))
-		goto out;
+	offs = 3;
+	if (pci_read_vpd(hw->pdev, offs, len, str) < 0)
+		return;
 
-	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;
+	dev_info(&hw->pdev->dev, "%.*s\n", len, str);
 
-	while (p < vpd_buf + VPD_SIZE - 4) {
+	for(;;) {
 		int i;
 
-		if (!memcmp("RW", p, 2))	/* end marker */
+		offs += len;
+		if (pci_read_vpd(hw->pdev, offs, sizeof(tag), tag) < 0)
 			break;
 
-		len = p[2];
-		if (len > (p - vpd_buf) - 4)
+		if (!memcmp("RW", tag, 2))	/* end marker */
+			break;
+
+		offs += sizeof(tag);
+		len = tag[2];
+		if (len > sizeof(str))
 			break;
 
 		for (i = 0; i < ARRAY_SIZE(vpd_tags); i++) {
-			if (!memcmp(vpd_tags[i].tag, p, 2)) {
+			if (!memcmp(vpd_tags[i].tag, tag, 2)) {
+				if (pci_read_vpd(hw->pdev, offs, len, str) < 0)
+					return;
+
 				printk(KERN_DEBUG " %s: %.*s\n",
-				       vpd_tags[i].label, len, p + 3);
+				       vpd_tags[i].label, len, str);
 				break;
 			}
 		}
-		p += len + 3;
 	}
-out:
-	kfree(vpd_buf);
 }
 
 /* This driver supports yukon2 chipset only */

Re: [PATCH 3/3] sky2: use pci_read_vpd to read info during boot

[Jeff Garzik]

Stephen Hemminger wrote:
> Change sky2 driver (in netdev next tree) to use vpd access routines.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> Note: other usage of vpd internal access routines will go away in later patches.
> 
> ---
> Patch against netdev-2.6#upstream-next assumes the previous PCI API change.
> 
> --- a/drivers/net/sky2.c	2008-09-03 11:35:47.000000000 -0700
> +++ b/drivers/net/sky2.c	2008-09-03 11:44:06.000000000 -0700
> @@ -4199,10 +4199,9 @@ static int __devinit pci_wake_enabled(st
>  
>  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;
> +	loff_t offs;
> +	u8 len;
> +	u8 tag[3];
>  	static struct vpd_tag {
>  		char tag[2];
>  		char *label;
> @@ -4211,47 +4210,48 @@ static void __devinit sky2_vpd_info(stru
>  		{ "EC", "Engineering Level" },
>  		{ "MN", "Manufacturer" },
>  	};
> +	char str[128];
>  
> -	if (!cap)
> -		goto out;
> -
> -	vpd_buf = kmalloc(VPD_SIZE, GFP_KERNEL);
> -	if (!vpd_buf)
> -		goto out;
> +	if (pci_read_vpd(hw->pdev, 0, sizeof(tag), tag) < 0)
> +		return;
> +	if (tag[0] != VPD_MAGIC)
> +		return;
> +	len = tag[1];
> +	if (len == 0 || len > sizeof(str))
> +		return;
>  
> -	if (sky2_vpd_read(hw, cap, vpd_buf, 0, VPD_SIZE))
> -		goto out;
> +	offs = 3;
> +	if (pci_read_vpd(hw->pdev, offs, len, str) < 0)
> +		return;
>  
> -	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;
> +	dev_info(&hw->pdev->dev, "%.*s\n", len, str);
>  
> -	while (p < vpd_buf + VPD_SIZE - 4) {
> +	for(;;) {
>  		int i;
>  
> -		if (!memcmp("RW", p, 2))	/* end marker */
> +		offs += len;
> +		if (pci_read_vpd(hw->pdev, offs, sizeof(tag), tag) < 0)
>  			break;
>  
> -		len = p[2];
> -		if (len > (p - vpd_buf) - 4)
> +		if (!memcmp("RW", tag, 2))	/* end marker */
> +			break;
> +
> +		offs += sizeof(tag);
> +		len = tag[2];
> +		if (len > sizeof(str))
>  			break;
>  
>  		for (i = 0; i < ARRAY_SIZE(vpd_tags); i++) {
> -			if (!memcmp(vpd_tags[i].tag, p, 2)) {
> +			if (!memcmp(vpd_tags[i].tag, tag, 2)) {
> +				if (pci_read_vpd(hw->pdev, offs, len, str) < 0)
> +					return;
> +
>  				printk(KERN_DEBUG " %s: %.*s\n",
> -				       vpd_tags[i].label, len, p + 3);
> +				       vpd_tags[i].label, len, str);
>  				break;
>  			}
>  		}
> -		p += len + 3;
>  	}
> -out:
> -	kfree(vpd_buf);
>  }

Acked-by: me

I presume this will go via PCI tree?

Re: [PATCH 3/3] sky2: use pci_read_vpd to read info during boot

[Jesse Barnes]

On Thursday, September 04, 2008 12:36 am Jeff Garzik wrote:
> Stephen Hemminger wrote:
> > Change sky2 driver (in netdev next tree) to use vpd access routines.
> >
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> >
> > Note: other usage of vpd internal access routines will go away in later
> > patches.
> >
> > ---
> > Patch against netdev-2.6#upstream-next assumes the previous PCI API
> > change.
> >
> > --- a/drivers/net/sky2.c	2008-09-03 11:35:47.000000000 -0700
> > +++ b/drivers/net/sky2.c	2008-09-03 11:44:06.000000000 -0700
> > @@ -4199,10 +4199,9 @@ static int __devinit pci_wake_enabled(st
> >
> >  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;
> > +	loff_t offs;
> > +	u8 len;
> > +	u8 tag[3];
> >  	static struct vpd_tag {
> >  		char tag[2];
> >  		char *label;
> > @@ -4211,47 +4210,48 @@ static void __devinit sky2_vpd_info(stru
> >  		{ "EC", "Engineering Level" },
> >  		{ "MN", "Manufacturer" },
> >  	};
> > +	char str[128];
> >
> > -	if (!cap)
> > -		goto out;
> > -
> > -	vpd_buf = kmalloc(VPD_SIZE, GFP_KERNEL);
> > -	if (!vpd_buf)
> > -		goto out;
> > +	if (pci_read_vpd(hw->pdev, 0, sizeof(tag), tag) < 0)
> > +		return;
> > +	if (tag[0] != VPD_MAGIC)
> > +		return;
> > +	len = tag[1];
> > +	if (len == 0 || len > sizeof(str))
> > +		return;
> >
> > -	if (sky2_vpd_read(hw, cap, vpd_buf, 0, VPD_SIZE))
> > -		goto out;
> > +	offs = 3;
> > +	if (pci_read_vpd(hw->pdev, offs, len, str) < 0)
> > +		return;
> >
> > -	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;
> > +	dev_info(&hw->pdev->dev, "%.*s\n", len, str);
> >
> > -	while (p < vpd_buf + VPD_SIZE - 4) {
> > +	for(;;) {
> >  		int i;
> >
> > -		if (!memcmp("RW", p, 2))	/* end marker */
> > +		offs += len;
> > +		if (pci_read_vpd(hw->pdev, offs, sizeof(tag), tag) < 0)
> >  			break;
> >
> > -		len = p[2];
> > -		if (len > (p - vpd_buf) - 4)
> > +		if (!memcmp("RW", tag, 2))	/* end marker */
> > +			break;
> > +
> > +		offs += sizeof(tag);
> > +		len = tag[2];
> > +		if (len > sizeof(str))
> >  			break;
> >
> >  		for (i = 0; i < ARRAY_SIZE(vpd_tags); i++) {
> > -			if (!memcmp(vpd_tags[i].tag, p, 2)) {
> > +			if (!memcmp(vpd_tags[i].tag, tag, 2)) {
> > +				if (pci_read_vpd(hw->pdev, offs, len, str) < 0)
> > +					return;
> > +
> >  				printk(KERN_DEBUG " %s: %.*s\n",
> > -				       vpd_tags[i].label, len, p + 3);
> > +				       vpd_tags[i].label, len, str);
> >  				break;
> >  			}
> >  		}
> > -		p += len + 3;
> >  	}
> > -out:
> > -	kfree(vpd_buf);
> >  }
>
> Acked-by: me
>
> I presume this will go via PCI tree?

The sky2 bits too?  Sure, that's fine with me.  I'll stuff it into my 
linux-next tree tomorrow after a quick look.

Thanks,
Jesse

[PATCH 1/3] pci: VPD access timeout increase

[Stephen Hemminger]

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

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-02 10:42:12.000000000 -0700
+++ b/drivers/pci/access.c	2008-09-03 08:47:49.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,30 @@ 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,
-						&status);
-		if (ret < 0)
-			return ret;
+	while ( (ret = pci_user_read_config_word(dev,
+						 vpd->cap + PCI_VPD_ADDR,
+						 &status)) == 0) {
 		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;
+		schedule();
 	}
+
+	return ret;
 }
 
 static int pci_vpd_pci22_read(struct pci_dev *dev, int pos, int size,
@@ -183,7 +184,7 @@ static int pci_vpd_pci22_read(struct pci
 	if (size == 0)
 		return 0;
 
-	spin_lock_irq(&vpd->lock);
+	mutex_lock(&vpd->lock);
 	ret = pci_vpd_pci22_wait(dev);
 	if (ret < 0)
 		goto out;
@@ -199,7 +200,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 +232,7 @@ static int pci_vpd_pci22_write(struct pc
 	val |= ((u8) *buf++) << 16;
 	val |= ((u32)(u8) *buf++) << 24;
 
-	spin_lock_irq(&vpd->lock);
+	mutex_lock(&vpd->lock);
 	ret = pci_vpd_pci22_wait(dev);
 	if (ret < 0)
 		goto out;
@@ -247,7 +248,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 +280,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/3] pci: VPD access timeout increase

[Matthew Wilcox]

On Wed, Sep 03, 2008 at 03:57:13PM -0700, Stephen Hemminger wrote:
> Accessing the VPD area can take a long time. There are comments in the
> SysKonnect vendor driver that it can take up to 25ms.  The existing vpd
> access code fails consistently on my hardware.

Wow, that's slow.  If you were to try to read all 32k, it'd take more
than three minutes!  (I presume it doesn't actually have as much as 32k).

> 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

I agree with your approach, but have one minor comment:

> -	spin_lock_irq(&vpd->lock);
> +	mutex_lock(&vpd->lock);

This should be:

+	if (mutex_lock_interruptible(&vpd->lock))
+		return -EINTR;

> @@ -231,7 +232,7 @@ static int pci_vpd_pci22_write(struct pc
>  	val |= ((u8) *buf++) << 16;
>  	val |= ((u32)(u8) *buf++) << 24;
>  
> -	spin_lock_irq(&vpd->lock);
> +	mutex_lock(&vpd->lock);

And the same here, of course.

-- 
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 access timeout increase

[Ben Hutchings]

Matthew Wilcox wrote:
> On Wed, Sep 03, 2008 at 03:57:13PM -0700, Stephen Hemminger wrote:
> > Accessing the VPD area can take a long time. There are comments in the
> > SysKonnect vendor driver that it can take up to 25ms.  The existing vpd
> > access code fails consistently on my hardware.
> 
> Wow, that's slow.  If you were to try to read all 32k, it'd take more
> than three minutes!  (I presume it doesn't actually have as much as 32k).
> 
> > 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
> 
> I agree with your approach, but have one minor comment:
> 
> > -	spin_lock_irq(&vpd->lock);
> > +	mutex_lock(&vpd->lock);
> 
> This should be:
> 
> +	if (mutex_lock_interruptible(&vpd->lock))
> +		return -EINTR;
[...]

This is fine for the sysfs case, but not if this is called during device
probe - we don't want signals to modprobe to break device initialisation,
do we?

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 1/3] pci: VPD access timeout increase

[Matthew Wilcox]

On Thu, Sep 04, 2008 at 03:19:46PM +0100, Ben Hutchings wrote:
> Matthew Wilcox wrote:
> > On Wed, Sep 03, 2008 at 03:57:13PM -0700, Stephen Hemminger wrote:
> > > Accessing the VPD area can take a long time. There are comments in the
> > > SysKonnect vendor driver that it can take up to 25ms.  The existing vpd
> > > access code fails consistently on my hardware.
> > 
> > Wow, that's slow.  If you were to try to read all 32k, it'd take more
> > than three minutes!  (I presume it doesn't actually have as much as 32k).
> > 
> > > 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
> > 
> > I agree with your approach, but have one minor comment:
> > 
> > > -	spin_lock_irq(&vpd->lock);
> > > +	mutex_lock(&vpd->lock);
> > 
> > This should be:
> > 
> > +	if (mutex_lock_interruptible(&vpd->lock))
> > +		return -EINTR;
> [...]
> 
> This is fine for the sysfs case, but not if this is called during device
> probe - we don't want signals to modprobe to break device initialisation,
> do we?

Probably only fatal signals -- in which case the if (signal_pending)
check should be a fatal_signal_pending() and mutex_lock_interruptible()
should be mutex_lock_killable().

OTOH, who's signalling modprobe to do anything other than die?

-- 
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 access timeout increase

[Stephen Hemminger]

On Thu, 4 Sep 2008 15:19:46 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> Matthew Wilcox wrote:
> > On Wed, Sep 03, 2008 at 03:57:13PM -0700, Stephen Hemminger wrote:
> > > Accessing the VPD area can take a long time. There are comments in the
> > > SysKonnect vendor driver that it can take up to 25ms.  The existing vpd
> > > access code fails consistently on my hardware.

It's bad but not that bad more details are:

				MIN		MAX
 -------------------------------------------------------------------
 write				1.8 ms		3.6 ms
 internal write cyles		0.7 ms		7.0 ms
 -------------------------------------------------------------------
 over all program time	 	2.5 ms		10.6 ms
 read				1.3 ms		2.6 ms
 -------------------------------------------------------------------
 over all 			3.8 ms		13.2 ms


Usable VPD is limited to 2K so worst case read is 27 seconds.
Note: there doesn't appear to be an standard for VPD size register in
PCI spec, but there is a device specific register.

> > Wow, that's slow.  If you were to try to read all 32k, it'd take more
> > than three minutes!  (I presume it doesn't actually have as much as 32k).
> > 
> > > 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
> > 
> > I agree with your approach, but have one minor comment:
> > 
> > > -	spin_lock_irq(&vpd->lock);
> > > +	mutex_lock(&vpd->lock);
> > 
> > This should be:
> > 
> > +	if (mutex_lock_interruptible(&vpd->lock))
> > +		return -EINTR;
> [...]
> 
> This is fine for the sysfs case, but not if this is called during device
> probe - we don't want signals to modprobe to break device initialisation,
> do we?

Why not, it makes sense to allow killing a stuck modprobe.

[PATCH] Return value from schedule()

[Matthew Wilcox]

In some circumstances, you want to wait for an event to happen.  let's
assume that it's a hardware event, so you can't just add a notifier of
some kind, you have to poll.  Here's an example:

On Wed, Sep 03, 2008 at 03:57:13PM -0700, Stephen Hemminger wrote:
>  			return -ETIMEDOUT;
> -		udelay(10);
> +		if (signal_pending(current))
> +			return -EINTR;
> +		schedule();

If there's no other task ready to run, schedule() could return in much
less than 10 microseconds (actually, it could return in much less than
10 microseconds even if another task does run, but let's ignore that case).

If schedule() returned whether or not it had scheduled another task, we
could do something like:

		if (!schedule())
			udelay(10);

Please consider this patch:

It can be useful to know whether a call to schedule() ran another task
or not.  For example, if you're giving up the CPU while waiting for an
event to happen, you might want to delay if no other process wants the
CPU to ensure you're not just bashing away at a device that is unlikely
to have finished yet.

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

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 5270d44..39c6ef9 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -329,7 +329,7 @@ extern signed long schedule_timeout(signed long timeout);
 extern signed long schedule_timeout_interruptible(signed long timeout);
 extern signed long schedule_timeout_killable(signed long timeout);
 extern signed long schedule_timeout_uninterruptible(signed long timeout);
-asmlinkage void schedule(void);
+asmlinkage int schedule(void);
 
 struct nsproxy;
 struct user_namespace;
diff --git a/kernel/sched.c b/kernel/sched.c
index 04160d2..ba3ab9a 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -4337,8 +4337,9 @@ pick_next_task(struct rq *rq, struct task_struct *prev)
 
 /*
  * schedule() is the main scheduler function.
+ * It returns 1 if we scheduled a new task and 0 if no other task was chosen.
  */
-asmlinkage void __sched schedule(void)
+asmlinkage int __sched schedule(void)
 {
 	struct task_struct *prev, *next;
 	unsigned long *switch_count;
@@ -4411,6 +4412,7 @@ need_resched_nonpreemptible:
 	preempt_enable_no_resched();
 	if (unlikely(test_thread_flag(TIF_NEED_RESCHED)))
 		goto need_resched;
+	return (next != prev);
 }
 EXPORT_SYMBOL(schedule);
 

-- 
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] Return value from schedule()

[Ingo Molnar]

* Matthew Wilcox <matthew@wil.cx> wrote:

> In some circumstances, you want to wait for an event to happen.  let's 
> assume that it's a hardware event, so you can't just add a notifier of 
> some kind, you have to poll.  Here's an example:
> 
> On Wed, Sep 03, 2008 at 03:57:13PM -0700, Stephen Hemminger wrote:
> >  			return -ETIMEDOUT;
> > -		udelay(10);
> > +		if (signal_pending(current))
> > +			return -EINTR;
> > +		schedule();
> 
> If there's no other task ready to run, schedule() could return in much
> less than 10 microseconds (actually, it could return in much less than
> 10 microseconds even if another task does run, but let's ignore that case).
> 
> If schedule() returned whether or not it had scheduled another task, we
> could do something like:
> 
> 		if (!schedule())
> 			udelay(10);

hm, i'm not really sure - this really just seems to be a higher prio 
variant of yield() combined with some weird code. Do we really want to 
promote such arguably broken behavior? If there's any chance of any 
polling to take a material amount of CPU time it should be event driven 
to begin with.

	Ingo

Re: [PATCH] Return value from schedule()

[Matthew Wilcox]

On Thu, Sep 04, 2008 at 06:14:24PM +0200, Ingo Molnar wrote:
> > If schedule() returned whether or not it had scheduled another task, we
> > could do something like:
> > 
> > 		if (!schedule())
> > 			udelay(10);
> 
> hm, i'm not really sure - this really just seems to be a higher prio 
> variant of yield() combined with some weird code. Do we really want to 
> promote such arguably broken behavior? If there's any chance of any 
> polling to take a material amount of CPU time it should be event driven 
> to begin with.

Oh, I'm not concerned about CPU utilisation, I'm concerned about PCI bus
utilisation.  Perhaps I'd like a yield_timeout() function instead where
I say that I'd like to not run for at least 10 microseconds?

Can we do that, or are we still jiffie-based there?

-- 
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] Return value from schedule()

[Arjan van de Ven]

On Thu, 4 Sep 2008 10:21:11 -0600
Matthew Wilcox <matthew@wil.cx> wrote:

> On Thu, Sep 04, 2008 at 06:14:24PM +0200, Ingo Molnar wrote:
> > > If schedule() returned whether or not it had scheduled another
> > > task, we could do something like:
> > > 
> > > 		if (!schedule())
> > > 			udelay(10);
> > 
> > hm, i'm not really sure - this really just seems to be a higher
> > prio variant of yield() combined with some weird code. Do we really
> > want to promote such arguably broken behavior? If there's any
> > chance of any polling to take a material amount of CPU time it
> > should be event driven to begin with.
> 
> Oh, I'm not concerned about CPU utilisation, I'm concerned about PCI
> bus utilisation.  Perhaps I'd like a yield_timeout() function instead
> where I say that I'd like to not run for at least 10 microseconds?
> 
> Can we do that, or are we still jiffie-based there?
> 

use schedule_hrtimerout() for this (hopefully will be in 2.6.28);
see this weeks LWN for an article describing it

Re: [PATCH] Return value from schedule()

[Matthew Wilcox]

On Thu, Sep 04, 2008 at 10:30:49AM -0700, Arjan van de Ven wrote:
> use schedule_hrtimerout() for this (hopefully will be in 2.6.28);
> see this weeks LWN for an article describing it

OK, so something like:

	struct timespec ts = { 0, 10 * 1000 };

	set_task_state(TASK_INTERRUPTIBLE);
	schedule_hrtimeout(&ts, HRTIMER_MODE_REL);
	if (fatal_signal_pending())
		return -EINTR;

should do the trick.

-- 
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] Return value from schedule()

[Stephen Hemminger]

On Thu, 4 Sep 2008 11:48:45 -0600
Matthew Wilcox <matthew@wil.cx> wrote:

> On Thu, Sep 04, 2008 at 10:30:49AM -0700, Arjan van de Ven wrote:
> > use schedule_hrtimerout() for this (hopefully will be in 2.6.28);
> > see this weeks LWN for an article describing it
> 
> OK, so something like:
> 
> 	struct timespec ts = { 0, 10 * 1000 };
> 
> 	set_task_state(TASK_INTERRUPTIBLE);
> 	schedule_hrtimeout(&ts, HRTIMER_MODE_REL);
> 	if (fatal_signal_pending())
> 		return -EINTR;
> 
> should do the trick.
> 

Never mind, I changed it to just yield() in revision.

Re: [PATCH] Return value from schedule()

[Peter Zijlstra]

On Thu, 2008-09-04 at 12:05 -0700, Stephen Hemminger wrote:
> On Thu, 4 Sep 2008 11:48:45 -0600
> Matthew Wilcox <matthew@wil.cx> wrote:
> 
> > On Thu, Sep 04, 2008 at 10:30:49AM -0700, Arjan van de Ven wrote:
> > > use schedule_hrtimerout() for this (hopefully will be in 2.6.28);
> > > see this weeks LWN for an article describing it
> > 
> > OK, so something like:
> > 
> > 	struct timespec ts = { 0, 10 * 1000 };
> > 
> > 	set_task_state(TASK_INTERRUPTIBLE);
> > 	schedule_hrtimeout(&ts, HRTIMER_MODE_REL);
> > 	if (fatal_signal_pending())
> > 		return -EINTR;
> > 
> > should do the trick.
> > 
> 
> Never mind, I changed it to just yield() in revision.

Gah, not another yield in the network code we have to figure out wtf its
meant to do.

Re: [PATCH 1/2] sky2: EEPROM read/write bug fixes

[Jeff Garzik]

Stephen Hemminger wrote:
> Cleanup and harden the routines accessing the EEPROM.
>   1. Prevent spin forever waiting for the TWSI bus
>   2. Fix write eeprom to write full words rather than only 16 bits 
>     Luckly the vendor doesn't provide EEPROM in Linux format so it must never
>     have been used.
>   3. Don't allow partial eeprom writes, not needed, not safe.
> 
> These are non-urgent bug fixes.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

applied 1-2