[Skiboot] [PATCH V2 3/6] opal/errorlog : Properly raising/clearing the event variable OPAL_EVENT_ERROR_LOG_AVAIL
Vasant Hegde
hegdevasant at linux.vnet.ibm.com
Fri Jun 10 18:57:26 AEST 2016
On 06/09/2016 07:24 PM, Mukesh Ojha wrote:
> When the OPAL_EVENT_ERROR_LOG_AVAIL raised from the write path to host, It is
> never cleared from the write path to host code. So, even after writing of all
> the OPAL errorlog list to host, the OPAL_EVENT_ERROR_LOG_AVAIL remain raised.
> It is never cleared from write path.
>
> This patch removes this bug of raising/clearing the variable
> OPAL_EVENT_ERROR_LOG_AVAIL by the implementation of general routine
> 'elog_set_head_state' and two static variables 'platform_state' and 'opal_state'.
>
> This 'elog_set_head_state' routine takes cares of the raising/clearing of
> OPAL_EVENT_ERROR_LOG_AVAIL variable.
>
> 'platform_state' and 'opal_state' variable are used to check any other errorlog
> are awaiting to be read by the host kernel either from the FSP (In the current
> scenario) or from the OPAL.
>
> So, the OPAL_EVENT_ERROR_LOG_AVAIL remain raised, if any of the errorlogs from
> OPAL or FSP is pending to get read and it will be cleared only when errorlogs
> list of both of them get empty.
Overall patch looks good. Few minor comments.
>
> Signed-off-by: Mukesh Ojha <mukesh02 at linux.vnet.ibm.com>
> Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
>
> ---
> Changes in V2:
> - Separated it from generalise read path patch 2/3 in V1.
> - Changed the implementation logic.
>
> core/elog-host.c | 52 +++++++++++++++++++++++++++++++++++++--------
> hw/fsp/fsp-elog-read.c | 11 +---------
> include/errorlog.h | 16 ++++++++++++++
> include/fsp-elog.h | 17 ++-------------
> platforms/ibm-fsp/firenze.c | 2 +-
> 5 files changed, 63 insertions(+), 35 deletions(-)
>
> diff --git a/core/elog-host.c b/core/elog-host.c
> index 9fb855b..0315055 100644
> --- a/core/elog-host.c
> +++ b/core/elog-host.c
> @@ -34,6 +34,40 @@ void *elog_write_to_host_buffer;
> /* Manipulate this only with write_lock held */
> static enum elog_head_state elog_write_to_host_head_state = ELOG_STATE_NONE;
>
> +void elog_set_head_state(bool platform_log, enum elog_head_state old_state,
> + enum elog_head_state new_state)
> +{
> + static enum elog_head_state platform_state = ELOG_STATE_NONE;
> + static enum elog_head_state opal_state = ELOG_STATE_NONE;
> +
> + if (platform_log) {
> + platform_state = new_state;
> +
> + if (opal_state == ELOG_STATE_FETCHED_DATA)
> + return;
> + } else {
> + opal_state = new_state;
> +
> + if (platform_state == ELOG_STATE_FETCHED_DATA)
> + return;
> + }
> +
> + if (new_state == ELOG_STATE_FETCHED_DATA &&
> + old_state != ELOG_STATE_FETCHED_DATA)
> + opal_update_pending_evt(OPAL_EVENT_ERROR_LOG_AVAIL,
> + OPAL_EVENT_ERROR_LOG_AVAIL);
> +
> + if (new_state != ELOG_STATE_FETCHED_DATA &&
> + old_state == ELOG_STATE_FETCHED_DATA)
> + opal_update_pending_evt(OPAL_EVENT_ERROR_LOG_AVAIL, 0);
> +}
> +
> +static inline void opal_elog_set_head_state(enum elog_head_state state)
> +{
> + elog_set_head_state(false, elog_write_to_host_head_state, state);
> + elog_write_to_host_head_state = state;
> +}
> +
> bool opal_elog_info(uint64_t *opal_elog_id, uint64_t *opal_elog_size)
> {
> struct errorlog *head;
> @@ -46,14 +80,15 @@ bool opal_elog_info(uint64_t *opal_elog_id, uint64_t *opal_elog_size)
> if (!head) {
> prlog(PR_ERR, "%s: Inconsistent internal list state !\n"
> , __func__);
> - elog_write_to_host_head_state = ELOG_STATE_NONE;
> + opal_elog_set_head_state(ELOG_STATE_NONE);
> } else {
> *opal_elog_id = head->plid;
> *opal_elog_size = head->log_size;
> - elog_write_to_host_head_state = ELOG_STATE_FETCHED_INFO;
> + opal_elog_set_head_state(ELOG_STATE_FETCHED_INFO);
> rc = true;
> }
> }
> +
> unlock(&elog_write_to_host_lock);
> return rc;
> }
> @@ -70,9 +105,7 @@ void opal_commit_elog_in_host(void)
> buf->log_size = create_pel_log(buf,
> (char *)elog_write_to_host_buffer,
> ELOG_WRITE_TO_HOST_BUFFER_SIZE);
> - elog_write_to_host_head_state = ELOG_STATE_FETCHED_DATA;
> - opal_update_pending_evt(OPAL_EVENT_ERROR_LOG_AVAIL,
> - OPAL_EVENT_ERROR_LOG_AVAIL);
> + opal_elog_set_head_state(ELOG_STATE_FETCHED_DATA);
> }
> unlock(&elog_write_to_host_lock);
> }
> @@ -88,7 +121,7 @@ bool opal_elog_read(uint64_t *buffer, uint64_t opal_elog_size,
> log_data = list_top(&elog_write_to_host_pending,
> struct errorlog, link);
> if (!log_data) {
> - elog_write_to_host_head_state = ELOG_STATE_NONE;
> + opal_elog_set_head_state(ELOG_STATE_NONE);
> unlock(&elog_write_to_host_lock);
> return rc;
> }
> @@ -103,7 +136,7 @@ bool opal_elog_read(uint64_t *buffer, uint64_t opal_elog_size,
> opal_elog_size);
> list_del(&log_data->link);
> list_add(&elog_write_to_host_processed, &log_data->link);
> - elog_write_to_host_head_state = ELOG_STATE_NONE;
> + opal_elog_set_head_state(ELOG_STATE_NONE);
> rc = true;
> }
> unlock(&elog_write_to_host_lock);
> @@ -134,7 +167,7 @@ bool opal_elog_ack(uint64_t ack_id)
> log_data = list_top(&elog_write_to_host_pending,
> struct errorlog, link);
> if (ack_id == log_data->plid)
> - elog_write_to_host_head_state = ELOG_STATE_NONE;
> + opal_elog_set_head_state(ELOG_STATE_NONE);
>
> list_for_each_safe(&elog_write_to_host_pending, record,
> next_record, link) {
> @@ -165,7 +198,8 @@ void opal_resend_pending_logs(void)
> struct errorlog, link);
> list_add_tail(&elog_write_to_host_pending, &record->link);
> }
> - elog_write_to_host_head_state = ELOG_STATE_NONE;
> +
> + opal_elog_set_head_state(ELOG_STATE_NONE);
> unlock(&elog_write_to_host_lock);
> opal_commit_elog_in_host();
> }
> diff --git a/hw/fsp/fsp-elog-read.c b/hw/fsp/fsp-elog-read.c
> index 213d9ec..9c3f843 100644
> --- a/hw/fsp/fsp-elog-read.c
> +++ b/hw/fsp/fsp-elog-read.c
> @@ -149,17 +149,8 @@ static void fsp_elog_check_and_fetch_head(void)
> /* This function should be called with the lock held */
> static void fsp_elog_set_head_state(enum elog_head_state state)
> {
> - enum elog_head_state old_state = elog_read_from_fsp_head_state;
> -
> + elog_set_head_state(true, elog_read_from_fsp_head_state, state);
> elog_read_from_fsp_head_state = state;
> - if (state == ELOG_STATE_FETCHED_DATA &&
> - old_state != ELOG_STATE_FETCHED_DATA)
> - opal_update_pending_evt(OPAL_EVENT_ERROR_LOG_AVAIL,
> - OPAL_EVENT_ERROR_LOG_AVAIL);
> -
> - if (state != ELOG_STATE_FETCHED_DATA &&
> - old_state == ELOG_STATE_FETCHED_DATA)
> - opal_update_pending_evt(OPAL_EVENT_ERROR_LOG_AVAIL, 0);
> }
>
> /*
> diff --git a/include/errorlog.h b/include/errorlog.h
> index d013433..3dff2cf 100644
> --- a/include/errorlog.h
> +++ b/include/errorlog.h
> @@ -22,6 +22,19 @@
> #include <stdint.h>
> #include <ccan/list/list.h>
>
> +/* Following variables are used to indicate state of the
> + * head log entry which is being fetched from FSP/OPAL and
> + * these variables are not overwritten until next log is
> + * retrieved from FSP/OPAL.
> + */
> +enum elog_head_state {
> + ELOG_STATE_FETCHING, /*In the process of reading log from FSP. */
> + ELOG_STATE_FETCHED_INFO,/* Indicates reading log info is completed */
> + ELOG_STATE_FETCHED_DATA,/* Indicates reading log is completed */
> + ELOG_STATE_NONE, /* Indicates to fetch next log */
> + ELOG_STATE_REJECTED, /* resend all pending logs to linux */
> +};
> +
> /* Classification of error/events type reported on OPAL */
> /* Platform Events/Errors: Report Machine Check Interrupt */
> #define OPAL_PLATFORM_ERR_EVT 0x01
> @@ -358,7 +371,10 @@ void log_commit(struct errorlog *elog);
> void opal_elog_complete(struct errorlog *elog, bool success);
>
> void opal_elog_init(void);
> +void elog_set_head_state(bool platform_log, enum elog_head_state old_state,
> + enum elog_head_state new_state);
> void opal_commit_elog_in_host(void);
> +bool opal_check_elog_pending_write_to_host(void);
I think you have to remove above function definition.
> void elog_append_write_to_host(struct errorlog *buf);
> int elog_init(void);
>
> diff --git a/include/fsp-elog.h b/include/fsp-elog.h
> index 1522b18..812c593 100644
> --- a/include/fsp-elog.h
> +++ b/include/fsp-elog.h
> @@ -13,28 +13,15 @@
> * See the License for the specific language governing permissions and
> * limitations under the License.
> */
> +#ifndef __ELOG_H
> +#define __ELOG_H
> #include <opal.h>
> #include <errorlog.h>
> #include <pel.h>
> -#ifndef __ELOG_H
> -#define __ELOG_H
>
> #define ELOG_TYPE_PEL 0
> #define MAX_RETRIES 3
>
> -/* Following variables are used to indicate state of the
> - * head log entry which is being fetched from FSP/OPAL and
> - * these variables are not overwritten until next log is
> - * retrieved from FSP/OPAL.
> - */
> -enum elog_head_state {
> - ELOG_STATE_FETCHING, /*In the process of reading log from FSP. */
> - ELOG_STATE_FETCHED_INFO,/* Indicates reading log info is completed */
> - ELOG_STATE_FETCHED_DATA,/* Indicates reading log is completed */
> - ELOG_STATE_NONE, /* Indicates to fetch next log */
> - ELOG_STATE_REJECTED, /* resend all pending logs to linux */
> -};
> -
> /* Generate src from opal reason code (src_comp) */
> #define generate_src_from_comp(src_comp) (OPAL_SRC_TYPE_ERROR << 24 | \
> OPAL_FAILING_SUBSYSTEM << 16 | src_comp)
> diff --git a/platforms/ibm-fsp/firenze.c b/platforms/ibm-fsp/firenze.c
> index 75f566c..3ef62a4 100644
> --- a/platforms/ibm-fsp/firenze.c
> +++ b/platforms/ibm-fsp/firenze.c
> @@ -566,4 +566,4 @@ DECLARE_PLATFORM(firenze) = {
> .resource_loaded = fsp_resource_loaded,
> .sensor_read = ibm_fsp_sensor_read,
> .terminate = ibm_fsp_terminate,
> -} ;
> +};
Remove above change as its not related to this patch.
-Vasant
More information about the Skiboot
mailing list