On Thu, 2008-09-04 at 13:56 -0700, Stephen Hemminger wrote:
> plain text document attachment (vpd-delay.patch)
> Accessing the VPD area can take a long time. The existing
> VPD access code fails consistently on my hardware. There are comments
> in the SysKonnect vendor driver that it can take up to 13ms per word.
>
> Change the access routines to:
> * use a mutex rather than spinning with IRQ's disabled and lock held
> * have a longer timeout
> * call schedule while spinning to provide some responsivness
>
> Signed-off-by: Stephen Hemminger <shemminger@xxxxxxxxxx>
>
>
> --- a/drivers/pci/access.c 2008-09-04 09:06:51.000000000 -0700
> +++ b/drivers/pci/access.c 2008-09-04 10:16:52.000000000 -0700
> @@ -133,7 +133,7 @@ PCI_USER_WRITE_CONFIG(dword, u32)
>
> struct pci_vpd_pci22 {
> struct pci_vpd base;
> - spinlock_t lock; /* controls access to hardware and the flags */
> + struct mutex lock;
> u8 cap;
> bool busy;
> bool flag; /* value of F bit to wait for */
> @@ -144,29 +144,33 @@ static int pci_vpd_pci22_wait(struct pci
> {
> struct pci_vpd_pci22 *vpd =
> container_of(dev->vpd, struct pci_vpd_pci22, base);
> - u16 flag, status;
> - int wait;
> + u16 flag = vpd->flag ? PCI_VPD_ADDR_F : 0;
> + unsigned long timeout = jiffies + (vpd->flag ? HZ/50 : HZ/10);
> + u16 status;
> int ret;
>
> if (!vpd->busy)
> return 0;
>
> - flag = vpd->flag ? PCI_VPD_ADDR_F : 0;
> - wait = vpd->flag ? 10 : 1000; /* read: 100 us; write: 10 ms */
> for (;;) {
> - ret = pci_user_read_config_word(dev,
> - vpd->cap + PCI_VPD_ADDR,
> + ret = pci_user_read_config_word(dev, vpd->cap + PCI_VPD_ADDR,
> &status);
> if (ret < 0)
> - return ret;
> + break;
> +
> if ((status & PCI_VPD_ADDR_F) == flag) {
> vpd->busy = false;
> - return 0;
> + break;
> }
> - if (wait-- == 0)
> +
> + if (time_after(jiffies, timeout))
> return -ETIMEDOUT;
> - udelay(10);
> + if (signal_pending(current))
> + return -EINTR;
> + yield();
At the very least put a big comment in here that we're polling the
hardware without completion event.
yield() is not good either, when used with a RT task (IMHO still the
only valid use of yield) it mostly doesn't spend any time away from this
task at all.
The other option, schedule_hrtimeout() isn't ideal either because on
crappy hardware it will fall back to jiffie based timeouts and I _hope_
that most hardware is less shitty than the 13ms reported in the
changelog.
Can we make this a per device delay that starts out small (the previous
10 us sound good) and gets doubled every once in a while when not enough
for a particular device?
That way we can - at some threshold - switch to sleeping delays..
> }
> +
> + return ret;
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
