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

[Peter Zijlstra <peterz@infradead.org>]

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

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

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

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

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

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

>  	}
> +
> +	return ret;
>  }