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

[Frederic Barrat]

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.


> +	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.


> +	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.



> +
> +			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()



> +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?

   Fred