[Skiboot] [PATCH v3 1/9] core/pldm/test : Implement PLDM self test common api

[Christophe Lombard]

Le 18/05/2022 à 09:42, Abhishek Singh Tomar a écrit :
> The patch bypasses hardware-dependent implementation for
> PLDM test. This patch provides API that will bypass the
> need to exchange messages with BMC over the LPC bus whereas
> messages are handled by the same program for self test.
>
> It also contains common functions and declarations that
> are used to implement the PLDM self test.
>
> Signed-off-by: Abhishek Singh Tomar<abhishek at linux.ibm.com>
> ---
>   core/pldm/test/common/test-pldm-common.c | 190 +++++++++++++++++++++++
>   1 file changed, 190 insertions(+)
>   create mode 100644 core/pldm/test/common/test-pldm-common.c

why did you create a subfolder "common" ?

> diff --git a/core/pldm/test/common/test-pldm-common.c b/core/pldm/test/common/test-pldm-common.c
> new file mode 100644
> index 00000000..4acbfd35
> --- /dev/null
> +++ b/core/pldm/test/common/test-pldm-common.c
> @@ -0,0 +1,190 @@
> +// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
> +// Copyright 2022 IBM Corp.
> +
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdarg.h>
> +#include <stdbool.h>
> +#include <types.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <time.h>
> +#include <timer.h>
> +#include <ccan/list/list.h>
> +#include <ccan/short_types/short_types.h>
> +
> +#define __LITTLE_ENDIAN_BITFIELD
> +#define __TEST__
> +#define __SKIBOOT__
> +#define zalloc(bytes) calloc((bytes), 1)
> +static inline unsigned long mftb(void);
> +#include <timebase.h>
> +#include <op-panel.h>
> +#include <include/platform.h>
> +#include "../../pldm.h"
> +#include <include/pldm.h>
> +#include <ast.h>
> +#ifdef ARRAY_SIZE
> +#undef ARRAY_SIZE
> +#endif
> +
> +
> +#include <pldm/libpldm/bios_table.h>
> +#include <pldm/libpldm/bios_table.c>
> +#undef pr_fmt
> +#include "../../pldm-bios-requests.c"
> +#include <pldm/libpldm/base.c>
> +#include <pldm/libpldm/pdr.c>
> +#include <pldm/libpldm/bios.c>
> +#include <pldm/libpldm/platform.c>
> +#include <pldm/libpldm/utils.c>
> +#include <pldm/ibm/libpldm/file_io.c>
> +#include <pldm/libpldm/fru.c>
> +#include "../../pldm-file-io-requests.c"
> +#include "../../pldm-requester.c"
> +#include "../../pldm-common.c"
> +#include "../../pldm-responder.c"
> +#include "../../pldm-base-requests.c"
> +#include "../../pldm-watchdog.c"
> +#include "../../pldm-fru-requests.c"
> +#include "../../pldm-platform-requests.c"
> +#include "../../../device.c"
> +
> +
> +
> +char __rodata_start[1], __rodata_end[1];
> +unsigned long tb_hz = 512000000;
> +struct dt_node *dt_root;
> +struct debug_descriptor debug_descriptor;
> +struct platform platform;
> +
> +int pldm_test_reply_request(void *request_msg, size_t request_len,
> +		void **response_msg, size_t *response_len);
> +int pldm_test_verify_response(void *response_msg, size_t response_len);
> +

I am not so fan about the global functions. So, rather than declaring
these functions in each test file, we could instead have a common list,
each test would record its type and the functions to call.

> +void time_wait_ms(unsigned long ms)
> +{
> +	usleep(ms * 1000);
> +}
> +void init_timer(struct timer *t, timer_func_t expiry, void *data)
> +{
> +	t->link.next = t->link.prev = NULL;
> +	t->target = 0;
> +	t->expiry = expiry;
> +	t->user_data = data;
> +	t->running = NULL;
> +}
> +uint64_t schedule_timer(struct timer *t, uint64_t how_long)
> +{
> +	if (t != NULL)
> +		return how_long;
> +	return 0;
> +}
> +void cancel_timer(struct timer *t)
> +{
> +	t->link.next = t->link.prev = NULL;
> +}
> +
> +static inline unsigned long mftb(void)
> +{
> +	unsigned long clk;
> +
> +	clk = clock();
> +	return clk;
> +}
> +
> +int ast_mctp_init(void (*fn)(uint8_t src_eid, bool tag_owner, uint8_t msg_tag, void *data,
> +			void *msg, size_t len))
> +{
> +	if (fn != NULL)
> +		return OPAL_SUCCESS;
> +	return OPAL_PARAMETER;
> +}
> +
> +int ast_mctp_message_tx(uint8_t eid, uint8_t *msg, int len)
> +{
> +	int ret;
> +	uint8_t *pldm_received_msg = msg+1;
> +	void *response_msg;
> +	char *vmsg;
> +	size_t response_len;
> +
> +	/* TEST eid is BMC_ID */
> +	if (eid != BMC_EID)
> +		return OPAL_PARAMETER;
> +
> +	/* TEST Message TYPE: PLDM = 0x01 (000_0001b) as per MCTP - DSP0240 */
> +	if (msg[0] != 0x01) {
> +		printf("TEST : %s : request MCTP message type not set for PLDM\n", __func__);
> +		return OPAL_PARAMETER;
> +	}
> +
> +	if (((struct pldm_msg *)pldm_received_msg)->hdr.request == PLDM_RESPONSE) {
> +		ret = pldm_test_verify_response(pldm_received_msg, len-1);
> +		if (ret != OPAL_SUCCESS)
> +			return ret;
> +	}
> +

You could simplify the code:

     request = ((struct pldm_msg *)pldm_received_msg)->hdr.request;

     switch (request) {
     case PLDM_RESPONSE:
         return pldm_test_verify_response(pldm_received_msg, len-1);
         break;

     case PLDM_REQUEST:
         ....

> +	/* Reply to requests */
> +	else if (((struct pldm_msg *)pldm_received_msg)->hdr.request == PLDM_REQUEST) {
> +		ret = pldm_test_reply_request(pldm_received_msg, len-1,
> +				&response_msg, &response_len);

The name of the function is not very explicit.
pldm_test_reply_request() seems to build a response to the Skiboot request.
The response is sent afterwards.

> +		if (ret != OPAL_SUCCESS)
> +			return ret;
> +

blanck is not necessary.

> +
> +		/*
> +		 * If response length > 0 then response back
> +		 * else if response length == 0 then no need to response
> +		 */
> +		if (response_len > 0) {

Why do you need to test response_len ?
If response_len is equal to 0 or <0, so pldm_test_reply_request will 
return an error.

> +			vmsg = malloc(response_len+1);
> +			/*  TYPE: PLDM = 0x01 (000_0001b) as per MCTP - DSP0240 */
> +			vmsg[0] = 1;
> +			memcpy(vmsg + 1, response_msg, response_len);
> +			free(response_msg);
> +			pldm_rx_message(BMC_EID, 0, 0, NULL, vmsg, response_len+1);
> +			free(vmsg);
> +		}
> +	}
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +void ast_mctp_exit(void)
> +{
> +}
> +
> +void lock_caller(struct lock *l, const char *caller)
> +{
> +	(void)caller;
> +	assert(!l->lock_val);
> +	l->lock_val++;
> +}
> +
> +int _opal_queue_msg(enum opal_msg_type msg_type, void *data,
> +		void (*consumed)(void *data, int status),
> +		size_t params_size, const void *params)
> +{
> +	(void)msg_type;
> +	if (data != NULL || consumed != NULL)
> +		return OPAL_PARAMETER;
> +	if (params != NULL && params_size > 0)
> +		return OPAL_PARAMETER;
> +	return OPAL_SUCCESS;
> +
> +}
> +
> +
> +void unlock(struct lock *l)
> +{
> +	assert(l->lock_val);
> +	l->lock_val = 0;
> +}
> +
> +void prd_occ_reset(uint32_t proc)
> +{
> +	(void)proc;
> +}
> +
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/skiboot/attachments/20220523/1bdf170f/attachment-0001.htm>