[Skiboot] [PATCH v2 5/6] FSP/ELOG: Fix possible event notifier hangs

[Vasant Hegde]

On 07/13/2016 02:53 PM, Vasant Hegde wrote:
> On 07/04/2016 03:18 PM, Mukesh Ojha wrote:
>> Few questions.
>>
>> On Saturday 02 July 2016 09:03 PM, Vasant Hegde wrote:
>>> In some corner cases host may send acknowledgement without
>>> reading actual data (fsp_opal_elog_info -> fsp_opal_elog_ack).
>>> Because of this elog_read_from_fsp_head_state may be stuck in
>>> wrong state (ELOG_STATE_HOST_INFO) and not able to send remaining
>>> ELOG's to host. Hence reset ELOG state and start sending remaining
>>> ELOG's.
>>>
>>> Also in normal case we will ACK the logs which are already processed
>>> (elog_read_processed). Hence rearrange the code such that we go
>>> through elog_read_processed first.
>>>
>>> Finally return OPAL_PARAMETER if we are not able to find ELOG ID.
>>>
>>> Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
>>> ---
>>>   hw/fsp/fsp-elog-read.c  | 18 +++++++++++++++---
>>>   hw/fsp/fsp-elog-write.c |  3 +++
>>>   2 files changed, 18 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/fsp/fsp-elog-read.c b/hw/fsp/fsp-elog-read.c
>>> index b606236..8c01a1c 100644
>>> --- a/hw/fsp/fsp-elog-read.c
>>> +++ b/hw/fsp/fsp-elog-read.c
>>> @@ -416,21 +416,33 @@ static int64_t fsp_opal_elog_ack(uint64_t ack_id)
>>>           return rc;
>>>       }
>>>       lock(&elog_read_lock);
>>> -    list_for_each_safe(&elog_read_pending, record, next_record, link) {
>>> +    list_for_each_safe(&elog_read_processed, record, next_record, link) {
>>>           if (record->log_id != ack_id)
>>>               continue;
>>>           list_del(&record->link);
>>>           list_add(&elog_read_free, &record->link);
>>> +        unlock(&elog_read_lock);
>>> +        return rc;
>>>       }
>>> -    list_for_each_safe(&elog_read_processed, record, next_record, link) {
>>> +    list_for_each_safe(&elog_read_pending, record, next_record, link) {
>>
>> Is it really necessary to check the ack_id in the pending list?
>> As this function will be called only for id which is already in the processed
>> list.
>> Same we do it for opal errorlogs.
>
> Its required. If host ACKs without reading it.. then it will be in pending list.
>
>
>>>           if (record->log_id != ack_id)
>>>               continue;
>>> +        /* It means host has sent ACK without reading actual data.
>>> +         * Because of this elog_read_from_fsp_head_state may be
>>> +         * stuck in wrong state (ELOG_STATE_HOST_INFO) and not able
>>> +         * to send remaning ELOG's to host. Hence reset ELOG state
>>
>> s/remaning/remaining
>
> Ah yes.. will fix .

@Stewart,
   If you don't have any other comment/suggestions, can you fix this spell 
mistake before merging ?

-Vasant