Skip to contentSkip to conversation

sky2: EEPROM read/write bug fixes

PATCH In review Stephen Hemminger 11 participants 24 messages Aug 28, 2008 – Sep 9, 2008 1 message not in the archive

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

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

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.

drivers/net/sky2.c +55 -42
@@ -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);
 }
 
 
#1 lore raw

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

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

---
This can wait for 2.6.28

drivers/net/sky2.c +69 -4
@@ -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);
 }
#2 lore raw
In reply to Stephen Hemminger
Quoted 6 lines · 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.

[...]

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.

Signature · 3 lines
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.
#3 lore raw
Quoted 7 lines · On Thu, 28 Aug 2008 12:13:25 +0100 Ben Hutchings <[email protected]>…
Quoted 6 lines · 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.

Quoted 8 lines

[...]

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.

#4 lore raw
Quoted 16 lines · On Thu, 28 Aug 2008 08:30:35 -0700 Stephen Hemminger…
Quoted 13 lines · On Thu, 28 Aug 2008 12:13:25 +0100 Ben Hutchings <[email protected]>…
Quoted 6 lines · 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.

[...]

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.

Quoted 7 lines
Quoted 2 lines

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.

#5 lore raw
Quoted 22 lines · Stephen Hemminger
Quoted 19 lines · On Thu, 28 Aug 2008 08:30:35 -0700 Stephen Hemminger…
Quoted 15 lines · On Thu, 28 Aug 2008 12:13:25 +0100 Ben Hutchings <[email protected]>…
Quoted 6 lines · 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.

[...]

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.

Quoted 7 lines
Quoted 4 lines

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.

Signature · 3 lines
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.
#6 lore raw
Quoted 33 lines · Ben Hutchings
Quoted 31 lines · Stephen Hemminger
Quoted 27 lines · On Thu, 28 Aug 2008 08:30:35 -0700 Stephen Hemminger…
Quoted 9 lines · On Thu, 28 Aug 2008 12:13:25 +0100 Ben Hutchings <[email protected]>…
Quoted 7 lines · 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.

Quoted 9 lines

[...]

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!

Quoted 10 lines
Quoted 3 lines

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?

#7 lore raw

PATCH 1/3 pci: VPD access timeout increase

In reply to Ben Hutchings

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

drivers/pci/access.c +20 -19
@@ -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;
#8 lore raw

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

Quoted 3 lines · 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.

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

Quoted 4 lines

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:

Review

Quoted 2 lines
(quoted diff) +1 -1
-	spin_lock_irq(&vpd->lock);
+	mutex_lock(&vpd->lock);

This should be:

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

drivers/pci/access.c static int pci_vpd_pci22_write(struct pc · line 235

Quoted 6 lines
drivers/pci/access.c +1 -1
@@ -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.

Signature · 4 lines
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."
#9 lore raw

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

Quoted 20 lines · Matthew Wilcox
Quoted 3 lines · 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.

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

Quoted 4 lines

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:

Review

Quoted 2 lines
(quoted diff) +1 -1
-	spin_lock_irq(&vpd->lock);
+	mutex_lock(&vpd->lock);

This should be:

Review

Quoted 2 lines
(quoted diff) +2 -0
+	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.

Signature · 3 lines
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.
#10 lore raw

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

Quoted 21 lines · Ben Hutchings
Quoted 20 lines · Matthew Wilcox
Quoted 3 lines · 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.

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

Quoted 4 lines

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:

Review

Quoted 2 lines
(quoted diff) +1 -1
-	spin_lock_irq(&vpd->lock);
+	mutex_lock(&vpd->lock);

This should be:

Quoted 7 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+	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?

Signature · 4 lines
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."
#11 lore raw

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

In reply to Ben Hutchings
Quoted 5 lines · On Thu, 4 Sep 2008 15:19:46 +0100 Ben Hutchings <[email protected]>…
Quoted 4 lines · Matthew Wilcox
Quoted 3 lines · 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.

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.

Quoted 15 lines
Quoted 15 lines

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

Quoted 4 lines

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:

Review

Quoted 2 lines
(quoted diff) +1 -1
-	spin_lock_irq(&vpd->lock);
+	mutex_lock(&vpd->lock);

This should be:

Quoted 7 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+	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.

#12 lore raw

PATCH 1/1 Return value from schedule()

In reply to Stephen Hemminger

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:

Review

Quoted 5 lines · Stephen Hemminger
(quoted diff) +3 -1
 			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.

include/linux/sched.h +1 -1
index 5270d44..39c6ef9 100644
@@ -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;
kernel/sched.c +3 -1
index 04160d2..ba3ab9a 100644
@@ -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);
 
Signature · 4 lines
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."
#13 lore raw

Re: [PATCH] Return value from schedule()

Quoted 15 lines · 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:

Review

Quoted 5 lines · Stephen Hemminger
(quoted diff) +3 -1
 			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).

Quoted 5 lines

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

#14 lore raw

Re: [PATCH] Return value from schedule()

Quoted 11 lines · Ingo Molnar
Quoted 5 lines

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?

Signature · 4 lines
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."
#15 lore raw

Re: [PATCH] Return value from schedule()

Quoted 13 lines · On Thu, 4 Sep 2008 10:21:11 -0600 Matthew Wilcox <[email protected]> wrote:
Quoted 11 lines · Ingo Molnar
Quoted 5 lines

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.

Quoted 6 lines

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

#16 lore raw

Re: [PATCH] Return value from schedule()

Quoted 2 lines · Arjan van de Ven

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.

Signature · 4 lines
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."
#17 lore raw

Re: [PATCH] Return value from schedule()

Quoted 6 lines · On Thu, 4 Sep 2008 11:48:45 -0600 Matthew Wilcox <[email protected]> wrote:
Quoted 2 lines · Arjan van de Ven

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:

Quoted 9 lines
	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.

maintainer
#18 lore raw

Re: [PATCH] Return value from schedule()

Quoted 16 lines · Stephen Hemminger
Quoted 13 lines · On Thu, 4 Sep 2008 11:48:45 -0600 Matthew Wilcox <[email protected]> wrote:
Quoted 2 lines · Arjan van de Ven

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;
Quoted 4 lines
Quoted 2 lines

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.

A message that is not in the archive (sent off-list or not yet received): 20080903155316.1a0a5698@extreme

#20 lore raw

PATCH 2/3 pci: revise VPD access interface

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

drivers/pci/access.c +101 -64
@@ -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,
drivers/pci/pci.h +3 -3
@@ -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 */
 };
 
drivers/pci/pci-sysfs.c +8 -30
@@ -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;
include/linux/pci.h +4 -0
@@ -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);
#21 lore raw

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

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

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.

drivers/net/sky2.c +29 -29
@@ -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 */
Acked-by me on v1 3/3
#22 lore raw

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

Quoted 87 lines · Stephen Hemminger

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

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.

drivers/net/sky2.c +28 -25
@@ -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",

drivers/net/sky2.c static void __devinit sky2_vpd_info(stru · line 4255

Quoted 10 lines
drivers/net/sky2.c +1 -4
-				       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?

#23 lore raw

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

Quoted 103 lines · Jeff Garzik
Quoted 89 lines · Stephen Hemminger

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

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.

drivers/net/sky2.c +28 -25
@@ -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",

drivers/net/sky2.c static void __devinit sky2_vpd_info(stru · line 4255

Quoted 10 lines
drivers/net/sky2.c +1 -4
-				       vpd_tags[i].label, len, p + 3);
+				       vpd_tags[i].label, len, str);
 				break;
 			}
 		}
-		p += len + 3;
 	}
-out:
-	kfree(vpd_buf);
 }
Acked-by me
Quoted 1 line

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

#24 lore raw
In reply to Stephen Hemminger
Quoted 10 lines · 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.

applied 1-2