Re: [PATCH V2 2/2] pseries/eeh: Handle RTAS delay requests in configure_bridge

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Tue, Mar 29, 2016 at 12:51:51PM +1000, 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.
>

When talking about RTAS calls, it might be better to have their full
names defined in PAPR spec. So I guess it'd better to replace configure_pe
and configure_bridge with their corresponding full RTAS call names:
"ibm,configure-pe" and "ibm,configure-bridge".

>Cc: <stable@vger.kernel.org> # 3.10-

I think it would be #3.10+.

>Signed-off-by: Russell Currey <ruscur@russell.cc>
>---
>V2: Use rtas_busy_delay and the new ibm_configure_pe token, refactoring
>---
> arch/powerpc/platforms/pseries/eeh_pseries.c | 35 +++++++++++++++++++++++-----
> 1 file changed, 29 insertions(+), 6 deletions(-)
>
>diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
>index 231b1df..c442b11 100644
>--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
>+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
>@@ -619,20 +619,43 @@ static int pseries_eeh_configure_bridge(struct eeh_pe *pe)
> {
> 	int config_addr;
> 	int ret;
>+	/* 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;
> 
>-	ret = rtas_call(ibm_configure_pe, 3, 1, NULL,
>-			config_addr, BUID_HI(pe->phb->buid),
>-			BUID_LO(pe->phb->buid));
>+	while (max_wait > 0) {
>+		ret = rtas_call(ibm_configure_pe, 3, 1, NULL,
>+				config_addr, BUID_HI(pe->phb->buid),
>+				BUID_LO(pe->phb->buid));
> 
>-	if (ret)
>-		pr_warn("%s: Unable to configure bridge PHB#%d-PE#%x (%d)\n",
>-			__func__, pe->phb->global_number, pe->addr, ret);
>+		/*
>+		 * RTAS can return a delay value of up to 10^5 milliseconds
>+		 * (RTAS_EXTENDED_DELAY_MAX), which is too long.  Only respect
>+		 * the delay if it's 100ms or less.
>+		 */
> 

The PAPR spec states that the delay can be up to 10^5ms. The spec also suggests
the maximal delay is 10^2. I guess it's worthy to mention it in the above comments
if you like.

>+		switch (ret) {
>+		case 0:
>+			return ret;
>+		case RTAS_EXTENDED_DELAY_MIN:
>+		case RTAS_EXTENDED_DELAY_MIN+1:
>+		case RTAS_EXTENDED_DELAY_MIN+2:
>+			mwait = rtas_busy_delay(ret);
>+			break;
>+		default:
>+			goto err;
>+		}
>+
>+		max_wait -= mwait;

If you like, the block can be simplified to as below. In that case,
tag #err isn't needed.

                if (!ret)
                        return ret;

                max_wait -= rtas_busy_delay(ret);

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

Thanks,
Gavin