Skip to contentSkip to conversation

powerpc/powernv/nvram: opal_nvram_write handle unknown OPAL errors

PATCH Applied Nicholas Piggin 6 participants 9 messages Mar 26, 2018 – Mar 31, 2018 PowerPC

lore
Share & tools
lore

Nicholas Piggin

author reviewer
#0 lore raw

PATCH 1/1 powerpc/powernv/nvram: opal_nvram_write handle unknown OPAL errors

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 <[email protected]>
---
 arch/powerpc/platforms/powernv/opal-nvram.c | 2 ++
 1 file changed, 2 insertions(+)
arch/powerpc/platforms/powernv/opal-nvram.c +2 -0
index 9db4398ded5d..13bf625dc3e8 100644
@@ -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;
 }
Signature · 1 line
2.16.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 14 lines · 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 <[email protected]>
---
  arch/powerpc/platforms/powernv/opal-nvram.c | 2 ++
  1 file changed, 2 insertions(+)
arch/powerpc/platforms/powernv/opal-nvram.c +0 -0
index 9db4398ded5d..13bf625dc3e8 100644

arch/powerpc/platforms/powernv/opal-nvram.c static ssize_t opal_nvram_write(char *buf, size_t count, loff_t *index) · line 60

Quoted 2 lines
arch/powerpc/platforms/powernv/opal-nvram.c +0 -0
@@ -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?

arch/powerpc/platforms/powernv/opal-nvram.c static ssize_t opal_nvram_write(char *buf, size_t count, loff_t *index) · line 63

Quoted 3 lines
arch/powerpc/platforms/powernv/opal-nvram.c +2 -0
  	}
+	if (rc)
+		return -EIO;

Good catch. Thanks!

Reviewed-by Vasant Hegde <[email protected]>

-Vasant

author reviewer
#2 lore raw
Quoted 10 lines · On Tue, 27 Mar 2018 12:47:31 +0530 Vasant Hegde…
Quoted 9 lines · 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 <[email protected]>
---
  arch/powerpc/platforms/powernv/opal-nvram.c | 2 ++
  1 file changed, 2 insertions(+)
Quoted 10 lines

arch/powerpc/platforms/powernv/opal-nvram.c static ssize_t opal_nvram_write(char *buf, size_t count, loff_t *index) · line 60

Quoted 7 lines
arch/powerpc/platforms/powernv/opal-nvram.c +0 -0
index 9db4398ded5d..13bf625dc3e8 100644
@@ -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

reviewer
#4 lore raw
In reply to Nicholas Piggin
Quoted 3 lines · 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.

Does that ever happen with current skiboot?

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

cheers

Quoted 15 lines
arch/powerpc/platforms/powernv/opal-nvram.c +2 -0
index 9db4398ded5d..13bf625dc3e8 100644
@@ -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;
 }
Signature · 1 line
2.16.1
author reviewer
#5 lore raw
Quoted 7 lines · On Tue, 27 Mar 2018 23:13:00 +1100 Michael Ellerman <[email protected]> wrote:
Quoted 3 lines · 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.

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.

Quoted 1 line

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

#6 lore raw
In reply to Michael Ellerman

unscribed me

On ‎Tuesday‎, ‎March‎ ‎27‎, ‎2018‎ ‎05‎:‎34‎:‎30‎ ‎AM‎ ‎PDT, Michael Ellerman <[email protected]> wrote:

Quoted 3 lines · 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.

Does that ever happen with current skiboot?

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

cheers

Quoted 15 lines
arch/powerpc/platforms/powernv/opal-nvram.c +2 -0
index 9db4398ded5d..13bf625dc3e8 100644
@@ -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;
   }
Signature · 1 line
2.16.1
#3 lore raw
In reply to Nicholas Piggin
Quoted 24 lines · Nicholas Piggin
Quoted 10 lines · On Tue, 27 Mar 2018 12:47:31 +0530 Vasant Hegde…
Quoted 9 lines · 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 <[email protected]>
---
   arch/powerpc/platforms/powernv/opal-nvram.c | 2 ++
   1 file changed, 2 insertions(+)
Quoted 10 lines

arch/powerpc/platforms/powernv/opal-nvram.c static ssize_t opal_nvram_write(char *buf, size_t count, loff_t *index) · line 60

Quoted 7 lines
arch/powerpc/platforms/powernv/opal-nvram.c +0 -0
index 9db4398ded5d..13bf625dc3e8 100644
@@ -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?

Quoted 2 lines

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.

Quoted 5 lines

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

Acked-by on v1 1/1
#7 lore raw
In reply to Nicholas Piggin
Quoted 8 lines · 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 <[email protected]>
---
 arch/powerpc/platforms/powernv/opal-nvram.c | 2 ++
 1 file changed, 2 insertions(+)
Acked-by Stewart Smith <[email protected]>
Signature · 2 lines
Stewart Smith
OPAL Architect, IBM.
Applied
#8 lore raw
In reply to Nicholas Piggin
Quoted 7 lines · 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 <[email protected]>
Reviewed-by Vasant Hegde <[email protected]>
Acked-by Stewart Smith <[email protected]>

Applied to powerpc next, thanks.

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

cheers