| Message ID | 1479441508-18489-1-git-send-email-stewart@linux.vnet.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
On 11/18/2016 09:28 AM, Stewart Smith wrote: > If we were to terminate in a poller, we'd call op_display() which > called pollers which hit the recursive poller warning, which ended > in not much fun at all. > > This patch will skip the running of pollers and instead run > the FSP poller to set the op-panel display before attn. > > Signed-off-by: Stewart Smith <stewart@linux.vnet.ibm.com> > --- > hw/fsp/fsp-op-panel.c | 8 +++++++- > hw/fsp/fsp.c | 31 +++++++++++++++++++++++++++++++ > include/fsp.h | 7 +++++++ > 3 files changed, 45 insertions(+), 1 deletion(-) > > diff --git a/hw/fsp/fsp-op-panel.c b/hw/fsp/fsp-op-panel.c > index eb61e8d..7063cbb 100644 > --- a/hw/fsp/fsp-op-panel.c > +++ b/hw/fsp/fsp-op-panel.c > @@ -40,7 +40,13 @@ static void fsp_op_display_fatal(uint32_t w0, uint32_t w1) > > fsp_fillmsg(&op_msg, FSP_CMD_DISP_SRC_DIRECT, 3, 1, w0, w1); > > - fsp_sync_msg(&op_msg, false); > + /* > + * A special way to send a message: it doesn't run pollers. > + * This means we can call it while in a poller, which we may > + * well be in when we're terminating (and thus displaying a *fatal* > + * message on the op-panel). > + */ > + fsp_fatal_msg(&op_msg); So we call this function for FATAL errors only.. That means most likely we are in PANIC path (like abort). Do we really need to wait till we send message to FSP. Can't we just queue and proceed? Of course we have a side effect of message not completing .. Assuming we really want this message to complete, I see below function is duplicate of fsp_sync_message except that you are calling fsp poller here.. May be we should abstract it something like below ? __fsp_sync_msg(...., bool opal_poller) { .... if (opal_poller) call default opal poller else call fsp_poller } fsp_sync_msg(...) { return __fsp_sync_msg(...., true); } -Vasant
On 11/18/2016 03:56 PM, Vasant Hegde wrote: > On 11/18/2016 09:28 AM, Stewart Smith wrote: >> If we were to terminate in a poller, we'd call op_display() which >> called pollers which hit the recursive poller warning, which ended >> in not much fun at all. >> >> This patch will skip the running of pollers and instead run >> the FSP poller to set the op-panel display before attn. Stewart, Unrelated comment ... op_display is FSP specific feature and inside FSP code.But its called from many places including common code.. So in future if we want to introduce platform specific CONFIG then we have to reorganize this code.. Or may be introduce platform specific hook for this ? -Vasant
Vasant Hegde <hegdevasant@linux.vnet.ibm.com> writes: > On 11/18/2016 03:56 PM, Vasant Hegde wrote: >> On 11/18/2016 09:28 AM, Stewart Smith wrote: >>> If we were to terminate in a poller, we'd call op_display() which >>> called pollers which hit the recursive poller warning, which ended >>> in not much fun at all. >>> >>> This patch will skip the running of pollers and instead run >>> the FSP poller to set the op-panel display before attn. > > Stewart, > > Unrelated comment ... op_display is FSP specific feature and inside FSP code.But > its called from many places including common code.. So in future if we want to > introduce platform specific CONFIG then we have to reorganize this code.. Or may > be introduce platform specific hook for this ? I think a hook. You *may* want to display such a thing on a BMC web UI or something, or query over IPMI. Although we may want to put something more sensible on that display than the incomprehensible jumble of hex we do now.
Stewart Smith <stewart@linux.vnet.ibm.com> writes: > If we were to terminate in a poller, we'd call op_display() which > called pollers which hit the recursive poller warning, which ended > in not much fun at all. > > This patch will skip the running of pollers and instead run > the FSP poller to set the op-panel display before attn. > > Signed-off-by: Stewart Smith <stewart@linux.vnet.ibm.com> merged to master as of 9fcb109218b1374a8caa3cac62e83fbedb1f7f2f Also cherry-picked back to stable: [skiboot-5.1.x b4bde3b] [skiboot-5.3.x 8b9ec81] [skiboot-5.4.x 1a0cd0a]
diff --git a/hw/fsp/fsp-op-panel.c b/hw/fsp/fsp-op-panel.c index eb61e8d..7063cbb 100644 --- a/hw/fsp/fsp-op-panel.c +++ b/hw/fsp/fsp-op-panel.c @@ -40,7 +40,13 @@ static void fsp_op_display_fatal(uint32_t w0, uint32_t w1) fsp_fillmsg(&op_msg, FSP_CMD_DISP_SRC_DIRECT, 3, 1, w0, w1); - fsp_sync_msg(&op_msg, false); + /* + * A special way to send a message: it doesn't run pollers. + * This means we can call it while in a poller, which we may + * well be in when we're terminating (and thus displaying a *fatal* + * message on the op-panel). + */ + fsp_fatal_msg(&op_msg); } void op_display(enum op_severity sev, enum op_module mod, uint16_t code) diff --git a/hw/fsp/fsp.c b/hw/fsp/fsp.c index 58219a0..c25e42c 100644 --- a/hw/fsp/fsp.c +++ b/hw/fsp/fsp.c @@ -1689,6 +1689,7 @@ void fsp_interrupt(void) unlock(&fsp_lock); } + int fsp_sync_msg(struct fsp_msg *msg, bool autofree) { int rc; @@ -1977,6 +1978,36 @@ static void fsp_opal_poll(void *data __unused) } } +int fsp_fatal_msg(struct fsp_msg *msg) +{ + int rc = 0; + + rc = fsp_queue_msg(msg, NULL); + if (rc) + return rc; + + while(fsp_msg_busy(msg)) { + cpu_relax(); + fsp_opal_poll(NULL); + } + + switch(msg->state) { + case fsp_msg_done: + rc = 0; + break; + case fsp_msg_timeout: + rc = -1; /* XXX to improve */ + break; + default: + rc = -1; /* Should not happen... (assert ?) */ + } + + if (msg->resp) + rc = (msg->resp->word1 >> 8) & 0xff; + + return rc; +} + static bool fsp_init_one(const char *compat) { struct dt_node *fsp_node; diff --git a/include/fsp.h b/include/fsp.h index 7ea162d..6142ca3 100644 --- a/include/fsp.h +++ b/include/fsp.h @@ -697,6 +697,13 @@ extern void fsp_cancelmsg(struct fsp_msg *msg); extern int fsp_queue_msg(struct fsp_msg *msg, void (*comp)(struct fsp_msg *msg)) __warn_unused_result; +/* Send a fatal message to FSP + * + * This will *not* run pollers. + * Use only when attempting to get the word out about how we died. + */ +extern int fsp_fatal_msg(struct fsp_msg *msg); + /* Synchronously send a command. If there's a response, the status is * returned as a positive number. A negative result means an error * sending the message.
If we were to terminate in a poller, we'd call op_display() which called pollers which hit the recursive poller warning, which ended in not much fun at all. This patch will skip the running of pollers and instead run the FSP poller to set the op-panel display before attn. Signed-off-by: Stewart Smith <stewart@linux.vnet.ibm.com> --- hw/fsp/fsp-op-panel.c | 8 +++++++- hw/fsp/fsp.c | 31 +++++++++++++++++++++++++++++++ include/fsp.h | 7 +++++++ 3 files changed, 45 insertions(+), 1 deletion(-)