[Skiboot] [PATCH V6 17/21] core/pldm: Update "bmc-firmware-version" device-tree field

[Christophe Lombard]

Le 12/04/2023 à 17:41, Frederic Barrat a écrit :
>
>
> On 13/09/2022 12:27, Christophe Lombard wrote:
>> Use the GetFruRecordByOptionReq command to retrieve the bmc information
>> with: "FRU Field Type": Version
>>        "FRU Record Set Identifier": 1,
>>        "FRU Record Type": "General(1)"
>> and update the "bmc-firmware-version" device-tree field.
>>
>> Signed-off-by: Christophe Lombard <clombard at linux.vnet.ibm.com>
>> ---
>>   core/pldm/pldm-fru-requests.c | 60 +++++++++++++++++++++++++++++++++++
>>   core/pldm/pldm.h              |  1 +
>>   include/pldm.h                |  5 +++
>>   3 files changed, 66 insertions(+)
>>
>> diff --git a/core/pldm/pldm-fru-requests.c 
>> b/core/pldm/pldm-fru-requests.c
>> index 02c9b968..b07a7b1a 100644
>> --- a/core/pldm/pldm-fru-requests.c
>> +++ b/core/pldm/pldm-fru-requests.c
>> @@ -15,6 +15,7 @@ static void *fru_record_table;
>>   static size_t fru_record_length;
>>     static bool fru_ready;
>> +static char *bmc_version = NULL;
>>     static void fru_init_complete(bool success)
>>   {
>> @@ -33,6 +34,16 @@ static void fru_init_complete(bool success)
>>       fru_ready = true;
>>   }
>>   +int pldm_fru_get_bmc_version(void *bv)
>
>
> That's a dangerous API as we don't know what has been allocated and 
> we're going to write over it an unknown size. Should at the minimum 
> take an extra parameter specifying the buffer size.
>

You are right. We have to new parameters to control the memcpy 
operation. Thanks

>
>> +{
>> +    if (bmc_version == NULL)
>> +        return OPAL_HARDWARE;
>> +
>> +    memcpy(bv, bmc_version, strlen(bmc_version) + 1); > +
>> +    return OPAL_SUCCESS;
>> +}
>> +
>>   static int get_fru_record_table_req(void **record_table_data,
>>                       size_t *record_table_length)
>>   {
>> @@ -115,6 +126,55 @@ static int get_fru_record_table_req(void 
>> **record_table_data,
>>       return OPAL_SUCCESS;
>>   }
>>   +int pldm_fru_dt_add_bmc_version(void)
>> +{
>> +    struct pldm_fru_record_data_format *data;
>> +    struct pldm_fru_record_tlv *tlv;
>> +    struct dt_node *dt_fw_version;
>> +    uint8_t *record_table;
>> +    size_t record_size;
>> +
>> +    dt_fw_version = dt_find_by_name(dt_root, "ibm,firmware-versions");
>> +    if (!dt_fw_version)
>> +        return OPAL_SUCCESS;
>> +
>> +    /* retrieve the bmc information with
>> +     * "FRU Record Set Identifier": 1,
>> +     * "FRU Record Type": "General(1)"
>> +     * "FRU Field Type": Version
>> +     *
>> +     * we can not know size of the record table got by options
>> +     * in advance, but it must be less than the source table. So
>> +     * it's safe to use sizeof the source table.
>> +     */
>> +    record_table = zalloc(fru_record_length);
>
>
> We need to verify that the init went fine and 
> fru_record_table/fru_record_length are defined. It's a general pattern 
> in all those functions with static data. It's the only time I've seen 
> where you don't verify it first, but it's probably worth 
> double-checking all of them.
>
>

That's true. We need to check that we got successfully the pldm fru data 
using fru_ready. thanks

>> +    record_size = fru_record_length;
>> +    get_fru_record_by_option(
>> +            fru_record_table,
>> +            fru_record_length - 
>> PLDM_GET_FRU_RECORD_TABLE_MIN_RESP_BYTES,
>
>
> PLDM_GET_FRU_RECORD_TABLE_MIN_RESP_BYTES was already deducted in 
> decode_get_fru_record_table_resp(), so fru_record_length should be the 
> correct length. At least, that's what I understand :)
>

Really, I don't remember. You should certainly right, but I need to 
dobble check it. Thanks

>
>> +            record_table,
>> +            &record_size,
>> +            1,
>> +            PLDM_FRU_RECORD_TYPE_GENERAL,
>> +            PLDM_FRU_FIELD_TYPE_VERSION);
>> +
>> +    /* get tlv value */
>
>
> We need to check 'record_size' and make sure we have (at least) one 
> entry in the record table.
>

will do. thanks

> Fred