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 <rus...@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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
