* [PATCH] pseries/eeh: Handle RTAS delay requests in configure_bridge
@ 2016-03-22 0:34 Russell Currey
2016-03-23 0:28 ` Russell Currey
2016-03-23 0:38 ` Michael Ellerman
0 siblings, 2 replies; 6+ messages in thread
From: Russell Currey @ 2016-03-22 0:34 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Russell Currey
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;
+ /* 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;
+
+ /* 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;
+ }
- 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);
+ }
+ err:
+ pr_warn("%s: Unable to configure bridge PHB#%d-PE#%x (%d)\n",
+ __func__, pe->phb->global_number, pe->addr, ret);
return ret;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] pseries/eeh: Handle RTAS delay requests in configure_bridge
2016-03-22 0:34 [PATCH] pseries/eeh: Handle RTAS delay requests in configure_bridge Russell Currey
@ 2016-03-23 0:28 ` Russell Currey
2016-03-23 0:38 ` Michael Ellerman
1 sibling, 0 replies; 6+ messages in thread
From: Russell Currey @ 2016-03-23 0:28 UTC (permalink / raw)
To: linuxppc-dev
On Tue, 2016-03-22 at 11:34 +1100, 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>
Forgot to mention this patch should go to stable, 3.10+
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: pseries/eeh: Handle RTAS delay requests in configure_bridge
2016-03-22 0:34 [PATCH] pseries/eeh: Handle RTAS delay requests in configure_bridge Russell Currey
2016-03-23 0:28 ` Russell Currey
@ 2016-03-23 0:38 ` Michael Ellerman
2016-03-23 2:52 ` Russell Currey
1 sibling, 1 reply; 6+ messages in thread
From: Michael Ellerman @ 2016-03-23 0:38 UTC (permalink / raw)
To: Russell Currey, linuxppc-dev
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: pseries/eeh: Handle RTAS delay requests in configure_bridge
2016-03-23 0:38 ` Michael Ellerman
@ 2016-03-23 2:52 ` Russell Currey
2016-03-23 10:39 ` Michael Ellerman
0 siblings, 1 reply; 6+ messages in thread
From: Russell Currey @ 2016-03-23 2:52 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev
On Wed, 2016-03-23 at 11:38 +1100, Michael Ellerman wrote:
> 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.
>
Relic of a previous iteration, my bad.
> >
> > + /* 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) {
>
> ?
...yes.
>
> >
> > +
> > + /* 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.
Yes, good idea.
>
> >
> >
> > - 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() ?
Wasn't aware of that, makes life a lot easier. Do you know if the 0.2s
maximum delay also applies across the board? I definitely want to enforce
it here, but if it is ubiquitous then it should be in 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: pseries/eeh: Handle RTAS delay requests in configure_bridge
2016-03-23 2:52 ` Russell Currey
@ 2016-03-23 10:39 ` Michael Ellerman
2016-03-23 23:56 ` Russell Currey
0 siblings, 1 reply; 6+ messages in thread
From: Michael Ellerman @ 2016-03-23 10:39 UTC (permalink / raw)
To: Russell Currey, linuxppc-dev
On Wed, 2016-03-23 at 13:52 +1100, Russell Currey wrote:
> On Wed, 2016-03-23 at 11:38 +1100, Michael Ellerman wrote:
> > On Tue, 2016-22-03 at 00:34:55 UTC, Russell Currey wrote:
> > >
> > > + case RTAS_EXTENDED_DELAY_MIN+2:
> > > + mwait = 100;
> > > + break;
> > > + default:
> > > + goto err;
> > > + }
> > > +
> > > + max_wait -= mwait;
> > > + msleep(mwait);
> > Can you use rtas_busy_delay() ?
>
> Wasn't aware of that, makes life a lot easier. Do you know if the 0.2s
> maximum delay also applies across the board? I definitely want to enforce
> it here, but if it is ubiquitous then it should be in rtas_busy_delay.
Not sure sorry, you'll have to read PAPR :)
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: pseries/eeh: Handle RTAS delay requests in configure_bridge
2016-03-23 10:39 ` Michael Ellerman
@ 2016-03-23 23:56 ` Russell Currey
0 siblings, 0 replies; 6+ messages in thread
From: Russell Currey @ 2016-03-23 23:56 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev
On Wed, 2016-03-23 at 21:39 +1100, Michael Ellerman wrote:
> On Wed, 2016-03-23 at 13:52 +1100, Russell Currey wrote:
> >
> > On Wed, 2016-03-23 at 11:38 +1100, Michael Ellerman wrote:
> > >
> > > On Tue, 2016-22-03 at 00:34:55 UTC, Russell Currey wrote:
> > > >
> > > >
> > > > + case RTAS_EXTENDED_DELAY_MIN+2:
> > > > + mwait = 100;
> > > > + break;
> > > > + default:
> > > > + goto err;
> > > > + }
> > > > +
> > > > + max_wait -= mwait;
> > > > + msleep(mwait);
> >
> > >
> > > Can you use rtas_busy_delay() ?
> > Wasn't aware of that, makes life a lot easier. Do you know if the 0.2s
> > maximum delay also applies across the board? I definitely want to
> > enforce
> > it here, but if it is ubiquitous then it should be in rtas_busy_delay.
> Not sure sorry, you'll have to read PAPR :)
It doesn't mention anything, but I was more concerned about whether it's
ever sensible to have that much of a sleep in any case. Probably safe to
ignore given it hasn't been a problem up to this point.
>
> cheers
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-03-23 23:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-22 0:34 [PATCH] pseries/eeh: Handle RTAS delay requests in configure_bridge Russell Currey
2016-03-23 0:28 ` Russell Currey
2016-03-23 0:38 ` Michael Ellerman
2016-03-23 2:52 ` Russell Currey
2016-03-23 10:39 ` Michael Ellerman
2016-03-23 23:56 ` Russell Currey
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.