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

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

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


--- a/drivers/pci/access.c	2008-09-04 09:06:51.000000000 -0700
+++ b/drivers/pci/access.c	2008-09-04 10:16:52.000000000 -0700
@@ -133,7 +133,7 @@ PCI_USER_WRITE_CONFIG(dword, u32)
 
 struct pci_vpd_pci22 {
 	struct pci_vpd base;
-	spinlock_t lock; /* controls access to hardware and the flags */
+	struct mutex lock;
 	u8	cap;
 	bool	busy;
 	bool	flag; /* value of F bit to wait for */
@@ -144,29 +144,33 @@ static int pci_vpd_pci22_wait(struct pci
 {
 	struct pci_vpd_pci22 *vpd =
 		container_of(dev->vpd, struct pci_vpd_pci22, base);
-	u16 flag, status;
-	int wait;
+	u16 flag = vpd->flag ? PCI_VPD_ADDR_F : 0;
+	unsigned long timeout = jiffies + (vpd->flag ? HZ/50 : HZ/10);
+	u16 status;
 	int ret;
 
 	if (!vpd->busy)
 		return 0;
 
-	flag = vpd->flag ? PCI_VPD_ADDR_F : 0;
-	wait = vpd->flag ? 10 : 1000; /* read: 100 us; write: 10 ms */
 	for (;;) {
-		ret = pci_user_read_config_word(dev,
-						vpd->cap + PCI_VPD_ADDR,
+		ret = pci_user_read_config_word(dev, vpd->cap + PCI_VPD_ADDR,
 						&status);
 		if (ret < 0)
-			return ret;
+			break;
+
 		if ((status & PCI_VPD_ADDR_F) == flag) {
 			vpd->busy = false;
-			return 0;
+			break;
 		}
-		if (wait-- == 0)
+
+		if (time_after(jiffies, timeout))
 			return -ETIMEDOUT;
-		udelay(10);
+		if (signal_pending(current))
+			return -EINTR;
+		yield();
 	}
+
+	return ret;
 }
 
 static int pci_vpd_pci22_read(struct pci_dev *dev, int pos, int size,
@@ -183,7 +187,9 @@ static int pci_vpd_pci22_read(struct pci
 	if (size == 0)
 		return 0;
 ...
[ message continues ]

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

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

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

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



--

[ message continues ]

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

What's wrong with the shorter:

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

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

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

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

[ message continues ]

On Thu, 04 Sep 2008 13:56:37 -0700

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

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

An alternative would be nice.

--

[ message continues ]

On Mon, 8 Sep 2008 13:40:25 -0700

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

[ message continues ]

On Mon, 8 Sep 2008 14:08:24 -0700


can you take a fixed time delay?

or use cond_resched()


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

[ message continues ]

On Mon, 8 Sep 2008 14:26:05 -0700
cond_resched sounds like the best suggestion.  The problem is that this
code needs to deal with hardware that could be fast, or slow depending
on how the device is implemented.  
--

[ message continues ]

On Tue, 9 Sep 2008 09:55:48 -0700

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



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

[ message continues ]