[Skiboot] [PATCH V2 3/6] opal/errorlog : Properly raising/clearing the event variable OPAL_EVENT_ERROR_LOG_AVAIL
Mukesh Ojha
mukesh02 at linux.vnet.ibm.com
Mon Jun 13 06:56:38 AEST 2016
Hi Vasant,
Thanks for your time in reviewing the patches.
All the suggested changes has been taken care in V3.
Regards,
Mukesh
On Friday 10 June 2016 02:27 PM, Vasant Hegde wrote:
> 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