[PATCH] powerpc/powernv/nvram: opal_nvram_write handle unknown OPAL errors

[PATCH] powerpc/powernv/nvram: opal_nvram_write handle unknown OPAL errors

[Nicholas Piggin]

opal_nvram_write currently just assumes success if it encounters an
error other than OPAL_BUSY or OPAL_BUSY_EVENT. Have it return -EIO
on other errors instead.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/platforms/powernv/opal-nvram.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/opal-nvram.c b/arch/powerpc/platforms/powernv/opal-nvram.c
index 9db4398ded5d..13bf625dc3e8 100644
--- a/arch/powerpc/platforms/powernv/opal-nvram.c
+++ b/arch/powerpc/platforms/powernv/opal-nvram.c
@@ -59,6 +59,8 @@ static ssize_t opal_nvram_write(char *buf, size_t count, loff_t *index)
 		if (rc == OPAL_BUSY_EVENT)
 			opal_poll_events(NULL);
 	}
+	if (rc)
+		return -EIO;
 	*index += count;
 	return count;
 }
-- 
2.16.1

Re: [PATCH] powerpc/powernv/nvram: opal_nvram_write handle unknown OPAL errors

[Vasant Hegde]

On 03/26/2018 08:32 PM, Nicholas Piggin wrote:
> opal_nvram_write currently just assumes success if it encounters an
> error other than OPAL_BUSY or OPAL_BUSY_EVENT. Have it return -EIO
> on other errors instead.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   arch/powerpc/platforms/powernv/opal-nvram.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/arch/powerpc/platforms/powernv/opal-nvram.c b/arch/powerpc/platforms/powernv/opal-nvram.c
> index 9db4398ded5d..13bf625dc3e8 100644
> --- a/arch/powerpc/platforms/powernv/opal-nvram.c
> +++ b/arch/powerpc/platforms/powernv/opal-nvram.c
> @@ -59,6 +59,8 @@ static ssize_t opal_nvram_write(char *buf, size_t count, loff_t *index)
>   		if (rc == OPAL_BUSY_EVENT)
>   			opal_poll_events(NULL);

Current code does continuous poller here. May be we have small breathing time 
here. What you say?


>   	}
> +	if (rc)
> +		return -EIO;

Good catch. Thanks!

Reviewed-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>

-Vasant

Re: [PATCH] powerpc/powernv/nvram: opal_nvram_write handle unknown OPAL errors

[Nicholas Piggin]

On Tue, 27 Mar 2018 12:47:31 +0530
Vasant Hegde <hegdevasant@linux.vnet.ibm.com> wrote:

> On 03/26/2018 08:32 PM, Nicholas Piggin wrote:
> > opal_nvram_write currently just assumes success if it encounters an
> > error other than OPAL_BUSY or OPAL_BUSY_EVENT. Have it return -EIO
> > on other errors instead.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >   arch/powerpc/platforms/powernv/opal-nvram.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/powerpc/platforms/powernv/opal-nvram.c b/arch/powerpc/platforms/powernv/opal-nvram.c
> > index 9db4398ded5d..13bf625dc3e8 100644
> > --- a/arch/powerpc/platforms/powernv/opal-nvram.c
> > +++ b/arch/powerpc/platforms/powernv/opal-nvram.c
> > @@ -59,6 +59,8 @@ static ssize_t opal_nvram_write(char *buf, size_t count, loff_t *index)
> >   		if (rc == OPAL_BUSY_EVENT)
> >   			opal_poll_events(NULL);  
> 
> Current code does continuous poller here. May be we have small breathing time 
> here. What you say?

Yeah that's probably not a bad idea. I cc'ed skiboot list -- what's a
reasonable delay between retries? Linux has a bunch of similar kind of
loops if you grep for opal_poll_events and OPAL_BUSY. It would be good
to convert them all to a standard form with a standard delay as
recommended by OPAL, and where specific calls have different delay
for a good reason, that would be documented in the OPAL API docs.

Thanks,
Nick

Re: [PATCH] powerpc/powernv/nvram: opal_nvram_write handle unknown OPAL errors

[Vasant Hegde]

On 03/27/2018 01:08 PM, Nicholas Piggin wrote:
> On Tue, 27 Mar 2018 12:47:31 +0530
> Vasant Hegde <hegdevasant@linux.vnet.ibm.com> wrote:
> 
>> On 03/26/2018 08:32 PM, Nicholas Piggin wrote:
>>> opal_nvram_write currently just assumes success if it encounters an
>>> error other than OPAL_BUSY or OPAL_BUSY_EVENT. Have it return -EIO
>>> on other errors instead.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>>    arch/powerpc/platforms/powernv/opal-nvram.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/arch/powerpc/platforms/powernv/opal-nvram.c b/arch/powerpc/platforms/powernv/opal-nvram.c
>>> index 9db4398ded5d..13bf625dc3e8 100644
>>> --- a/arch/powerpc/platforms/powernv/opal-nvram.c
>>> +++ b/arch/powerpc/platforms/powernv/opal-nvram.c
>>> @@ -59,6 +59,8 @@ static ssize_t opal_nvram_write(char *buf, size_t count, loff_t *index)
>>>    		if (rc == OPAL_BUSY_EVENT)
>>>    			opal_poll_events(NULL);
>>
>> Current code does continuous poller here. May be we have small breathing time
>> here. What you say?
> 
> Yeah that's probably not a bad idea. I cc'ed skiboot list -- what's a
> reasonable delay between retries?

I think it depends on individual API. Like in case of dump retrival I've 20ms 
delay... as FSP takes time to copy data to host memory. But may be here we can 
have smaller delay.

> Linux has a bunch of similar kind of
> loops if you grep for opal_poll_events and OPAL_BUSY. It would be good
> to convert them all to a standard form with a standard delay as
> recommended by OPAL, and where specific calls have different delay
> for a good reason, that would be documented in the OPAL API docs.

Yes. We should update API documentation.

-Vasant

Re: [PATCH] powerpc/powernv/nvram: opal_nvram_write handle unknown OPAL errors

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:

> opal_nvram_write currently just assumes success if it encounters an
> error other than OPAL_BUSY or OPAL_BUSY_EVENT. Have it return -EIO
> on other errors instead.

Does that ever happen with current skiboot?

Even if it doesn't I think I'm inclined to tag this for stable.

cheers

> diff --git a/arch/powerpc/platforms/powernv/opal-nvram.c b/arch/powerpc/platforms/powernv/opal-nvram.c
> index 9db4398ded5d..13bf625dc3e8 100644
> --- a/arch/powerpc/platforms/powernv/opal-nvram.c
> +++ b/arch/powerpc/platforms/powernv/opal-nvram.c
> @@ -59,6 +59,8 @@ static ssize_t opal_nvram_write(char *buf, size_t count, loff_t *index)
>  		if (rc == OPAL_BUSY_EVENT)
>  			opal_poll_events(NULL);
>  	}
> +	if (rc)
> +		return -EIO;
>  	*index += count;
>  	return count;
>  }
> -- 
> 2.16.1

Re: [PATCH] powerpc/powernv/nvram: opal_nvram_write handle unknown OPAL errors

[Nicholas Piggin]

On Tue, 27 Mar 2018 23:13:00 +1100
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Nicholas Piggin <npiggin@gmail.com> writes:
> 
> > opal_nvram_write currently just assumes success if it encounters an
> > error other than OPAL_BUSY or OPAL_BUSY_EVENT. Have it return -EIO
> > on other errors instead.  
> 
> Does that ever happen with current skiboot?

I can now even using the mambo fake flash driver that never returns
failure, because skiboot will return OPAL_INTERNAL_ERROR if we try
to re-enter it. So I hit it when testing sreset-in-opal cases (the
crash path wants to write something to nvram).

Not sure about the skiboot flash layer. Aside from programming
errors, it looks like perhaps ECC and BMC failure or unresponsive
could cause errors to come back here.

> Even if it doesn't I think I'm inclined to tag this for stable.

It's turning some relatively minor types of errors into a system
hang, so it seems like it could go in stable. I've hit the -EIO case
in these basic tests and it hasn't had obvious bugs.

Thanks,
Nick

Re: [PATCH] powerpc/powernv/nvram: opal_nvram_write handle unknown OPAL errors

[T T]

[-- Attachment #1: Type: text/plain, Size: 1101 bytes --]

 unscribed me 

    On ‎Tuesday‎, ‎March‎ ‎27‎, ‎2018‎ ‎05‎:‎34‎:‎30‎ ‎AM‎ ‎PDT, Michael Ellerman <mpe@ellerman.id.au> wrote:  
 
 Nicholas Piggin <npiggin@gmail.com> writes:

> opal_nvram_write currently just assumes success if it encounters an
> error other than OPAL_BUSY or OPAL_BUSY_EVENT. Have it return -EIO
> on other errors instead.

Does that ever happen with current skiboot?

Even if it doesn't I think I'm inclined to tag this for stable.

cheers

> diff --git a/arch/powerpc/platforms/powernv/opal-nvram.c b/arch/powerpc/platforms/powernv/opal-nvram.c
> index 9db4398ded5d..13bf625dc3e8 100644
> --- a/arch/powerpc/platforms/powernv/opal-nvram.c
> +++ b/arch/powerpc/platforms/powernv/opal-nvram.c
> @@ -59,6 +59,8 @@ static ssize_t opal_nvram_write(char *buf, size_t count, loff_t *index)
>          if (rc == OPAL_BUSY_EVENT)
>              opal_poll_events(NULL);
>      }
> +    if (rc)
> +        return -EIO;
>      *index += count;
>      return count;
>  }
> -- 
> 2.16.1
  

[-- Attachment #2: Type: text/html, Size: 2646 bytes --]

Re: [PATCH] powerpc/powernv/nvram: opal_nvram_write handle unknown OPAL errors

[Stewart Smith]

Nicholas Piggin <npiggin@gmail.com> writes:
> opal_nvram_write currently just assumes success if it encounters an
> error other than OPAL_BUSY or OPAL_BUSY_EVENT. Have it return -EIO
> on other errors instead.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/platforms/powernv/opal-nvram.c | 2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Stewart Smith <stewart@linux.ibm.com>

-- 
Stewart Smith
OPAL Architect, IBM.

Re: powerpc/powernv/nvram: opal_nvram_write handle unknown OPAL errors

[Michael Ellerman]

On Mon, 2018-03-26 at 15:02:33 UTC, Nicholas Piggin wrote:
> opal_nvram_write currently just assumes success if it encounters an
> error other than OPAL_BUSY or OPAL_BUSY_EVENT. Have it return -EIO
> on other errors instead.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> Reviewed-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> Acked-by: Stewart Smith <stewart@linux.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/741de617661794246f84a21a02fc5e

cheers