[Skiboot] [PATCH V6 10/21] core/pldm: PLDM for Platform Monitoring and Control Specification

[Christophe Lombard]

Le 12/04/2023 à 17:24, Frederic Barrat a écrit :
>
>
> On 13/09/2022 12:26, Christophe Lombard wrote:
>
>> diff --git a/core/pldm/pldm-platform-requests.c 
>> b/core/pldm/pldm-platform-requests.c
>> new file mode 100644
>> index 00000000..965820c8
>> --- /dev/null
>> +++ b/core/pldm/pldm-platform-requests.c
>> @@ -0,0 +1,254 @@
>> +// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
>> +// Copyright 2022 IBM Corp.
>> +
>> +#define pr_fmt(fmt) "PLDM: " fmt
>> +
>> +#include <cpu.h>
>> +#include <opal.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <timebase.h>
>> +#include <inttypes.h>
>> +#include <pldm/libpldm/entity.h>
>> +#include <pldm/libpldm/state_set.h>
>> +#include <pldm/libpldm/platform.h>
>> +#include "pldm.h"
>> +
>> +#define NO_MORE_PDR_HANDLES 0
>> +
>> +static pldm_pdr *pdrs_repo;
>> +static bool pdr_ready;
>> +
>> +struct pldm_pdrs {
>> +    uint32_t record_hndl;
>> +    bool done;
>> +    int rc;
>> +};
>> +
>> +static void pdr_init_complete(bool success)
>> +{
>> +    /* Read not successful, error out and free the buffer */
>> +    if (!success) {
>> +        pdr_ready = false;
>> +
>> +        pldm_pdr_destroy(pdrs_repo);
>> +        return;
>> +    }
>> +
>> +    /* Mark ready */
>> +    pdr_ready = true;
>> +}
>> +
>> +struct get_pdr_response {
>> +    uint8_t completion_code;
>> +    uint32_t next_record_hndl;
>> +    uint32_t next_data_transfer_hndl;
>> +    uint8_t transfer_flag;
>> +    uint16_t resp_cnt;
>> +    uint8_t *record_data;
>> +    size_t record_data_length;
>> +    uint8_t transfer_crc;
>> +};
>> +
>> +static int encode_and_queue_get_pdr_req(struct pldm_pdrs *pdrs);
>> +
>> +static void get_pdr_req_complete(struct pldm_rx_data *rx,
>> +                 void *data)
>> +{
>> +    struct pldm_pdrs *pdrs = (struct pldm_pdrs *)data;
>> +    uint32_t record_hndl = pdrs->record_hndl;
>> +    size_t payload_len;
>> +    int rc, i;
>> +
>
>
> Superfluous empty line in the middle of the variable declarations.
>

right !

>
>> +    struct get_pdr_response response;
>> +
>> +    prlog(PR_DEBUG, "%s - record_hndl: %d\n", __func__, record_hndl);
>> +
>> +    /* Decode the message twice; the first time, the payload buffer
>> +     * will be null so that the decoder will simply tell us how big
>> +     * the buffer should be. Then we create a suitable payload
>> +     * buffer and call the decoder again, this time with the real
>> +     * buffer so that it can fill it with data from the message.
>> +     *
>> +     * transfer_crc is not used in case of PLDM_START_AND_END.
>> +     */
>> +    payload_len = rx->msg_len - sizeof(struct pldm_msg_hdr);
>
>
> In case of a timeout, rx can be NULL and the line above will go badly.
>
>

good catch !

>> +    response.record_data_length = 0;
>> +    response.record_data = NULL;
>> +
>> +    for (i = 0; i < 2; i++) {
>> +        rc = decode_get_pdr_resp(
>> +                rx->msg, payload_len,
>> +                &response.completion_code,
>> +                &response.next_record_hndl,
>> +                &response.next_data_transfer_hndl,
>> +                &response.transfer_flag,
>> +                &response.resp_cnt,
>> +                response.record_data,
>> +                response.record_data_length,
>> +                &response.transfer_crc);
>> +
>> +        if (rc != PLDM_SUCCESS || response.completion_code != 
>> PLDM_SUCCESS) {
>> +            /* Message decoding failed */
>> +            prlog(PR_ERR, "Decode GetPDRResp Error (rc: %d, cc: %d)\n",
>> +                      rc, response.completion_code);
>> +
>> +            /* BMC is not ready, try again */
>> +            if (response.completion_code == PLDM_ERROR_NOT_READY) {
>> +                time_wait_ms(500);
>> +                encode_and_queue_get_pdr_req(pdrs);
>> +                return;
>> +            }
>
>
> Why are we singling out that case and retry? The pattern 
> encode/send/decode is seen several times, but that's the only time 
> where we try to handle PLDM_ERROR_NOT_READY, so it looks odd.
>
>

When the bmc restarts, or a specific event is received (repository 
change), and the server is still on, we need to destroy the current repo 
and collect all PDRs again.
In case the BMC is restarting, it may happen that this one is not ready 
for these requests.

>
>> +
>> +            pdrs->rc = OPAL_PARAMETER;
>> +            pdrs->done = true;
>> +            return;
>> +        }
>> +
>> +        if (response.record_data == NULL) {
>> +            response.record_data_length = response.resp_cnt;
>> +            response.record_data = zalloc(response.resp_cnt);
>> +        }
>> +    }
>> +
>> +    /* we do not support multipart transfer */
>> +    if (response.transfer_flag != PLDM_START_AND_END)
>> +        prlog(PR_ERR, "Transfert GetPDRResp not complete, 
>> transfer_flag: %d\n",
>> +                  response.transfer_flag);
>> +
>> +    prlog(PR_DEBUG, "%s - record_hndl: %d, next_record_hndl: %d, 
>> resp_cnt: %d\n",
>> +            __func__, record_hndl,
>> +            response.next_record_hndl,
>> +            response.resp_cnt);
>> +
>> +    /* Add a PDR record to a PDR repository */
>> +    pldm_pdr_add(pdrs_repo,
>> +             response.record_data,
>> +             response.resp_cnt,
>> +             record_hndl,
>> +             false);
>> +
>> +    free(response.record_data);
>> +
>> +    if (response.next_record_hndl != NO_MORE_PDR_HANDLES) {
>> +        pdrs->record_hndl = response.next_record_hndl;
>> +        encode_and_queue_get_pdr_req(pdrs);
>> +    } else {
>> +        pdr_init_complete(true);
>
>
> Is this call necessary? It looks like it will done again from 
> pldm_platform_init()

This call is necessary when we collect all pdrs due to BMC restart.

>
>
>
>> +int pldm_platform_init(void)
>> +{
>> +    int rc;
>> +
>> +    /* retrieve all PDRs */
>> +    rc = pdrs_init();
>> +    if (rc) {
>> +        pdr_init_complete(false);
>> +        prlog(PR_ERR, "%s - done, rc: %d\n", __func__, rc);
>
>
> Maybe a more descriptive error message?
>

ok

> Fred
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/skiboot/attachments/20230413/e1f1ee06/attachment-0001.htm>