Re: pseries/eeh: Handle RTAS delay requests in configure_bridge

[Michael Ellerman <mpe@ellerman.id.au>]

On Tue, 2016-22-03 at 00:34:55 UTC, Russell Currey wrote:
> In the configure_pe and configure_bridge RTAS calls, the spec states
> that values of 9900-9905 can be returned, indicating that software
> should delay for 10^x (where x is the last digit, i.e. 990x)
> milliseconds and attempt the call again. Currently, the kernel doesn't
> know about this, and respecting it fixes some PCI failures when the
> hypervisor is busy.
> 
> The delay is capped at 0.2 seconds.
> 
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
>  arch/powerpc/platforms/pseries/eeh_pseries.c | 63 +++++++++++++++++++++-------
>  1 file changed, 47 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
> index ac3ffd9..c5603185 100644
> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> @@ -614,30 +614,61 @@ static int pseries_eeh_get_log(struct eeh_pe *pe, int severity, char *drv_log, u
>  static int pseries_eeh_configure_bridge(struct eeh_pe *pe)
>  {
>  	int config_addr;
> -	int ret;
> +	int ret = -1;

If possible it's preferable not to pre-initialise your return value.

If you leave it uninitialised then the compiler can (hopefully) detect any
paths where you fail to initialise it.

> +	/* Waiting 0.2s maximum before skipping configuration */
> +	int max_wait = 200;
> +	int mwait;
>  
>  	/* Figure out the PE address */
>  	config_addr = pe->config_addr;
>  	if (pe->addr)
>  		config_addr = pe->addr;
>  
> -	/* Use new configure-pe function, if supported */
> -	if (ibm_configure_pe != RTAS_UNKNOWN_SERVICE) {
> -		ret = rtas_call(ibm_configure_pe, 3, 1, NULL,
> -				config_addr, BUID_HI(pe->phb->buid),
> -				BUID_LO(pe->phb->buid));
> -	} else if (ibm_configure_bridge != RTAS_UNKNOWN_SERVICE) {
> -		ret = rtas_call(ibm_configure_bridge, 3, 1, NULL,
> -				config_addr, BUID_HI(pe->phb->buid),
> -				BUID_LO(pe->phb->buid));
> -	} else {
> -		return -EFAULT;
> -	}
> +	while (1) {
> +		if (max_wait < 0)
> +			goto err;

Can't you just do:

	while (max_wait > 0) {

?

> +
> +		/* Use new configure-pe function, if supported */
> +		if (ibm_configure_pe != RTAS_UNKNOWN_SERVICE) {
> +			ret = rtas_call(ibm_configure_pe, 3, 1, NULL,
> +					config_addr, BUID_HI(pe->phb->buid),
> +					BUID_LO(pe->phb->buid));
> +		} else if (ibm_configure_bridge != RTAS_UNKNOWN_SERVICE) {
> +			ret = rtas_call(ibm_configure_bridge, 3, 1, NULL,
> +					config_addr, BUID_HI(pe->phb->buid),
> +					BUID_LO(pe->phb->buid));
> +		} else {
> +			return -EFAULT;
> +		}

I realise you've just indented that code, but it would be nice to clean it up
as a precursor patch.

AFAICS the args are identical, so you could just put the token in a variable
and do the rtas_call() once. It also looks like we check at startup that we
have one of the tokens, so we could choose the appropriate token then and avoid
any conditinals in this code.

>  
> -	if (ret)
> -		pr_warn("%s: Unable to configure bridge PHB#%d-PE#%x (%d)\n",
> -			__func__, pe->phb->global_number, pe->addr, ret);
> +		/*
> +		 * If RTAS returns a delay value, it expects software to sleep
> +		 * for 10^x milliseconds.  The max value it can return is thus
> +		 * 10^5 (RTAS_EXTENDED_DELAY_MAX), which is way too long.
> +		 */
>  
> +		switch (ret) {
> +		case 0:
> +			return ret;
> +		case RTAS_EXTENDED_DELAY_MIN:
> +			mwait = 1;
> +			break;
> +		case RTAS_EXTENDED_DELAY_MAX:
> +			mwait = 10;
> +			break;
> +		case RTAS_EXTENDED_DELAY_MIN+2:
> +			mwait = 100;
> +			break;
> +		default:
> +			goto err;
> +		}
> +
> +		max_wait -= mwait;
> +		msleep(mwait);

Can you use rtas_busy_delay() ?

> +	}
> + err:
> +	pr_warn("%s: Unable to configure bridge PHB#%d-PE#%x (%d)\n",
> +		__func__, pe->phb->global_number, pe->addr, ret);
>  	return ret;
>  }


cheers