[Skiboot] [PATCH V6 09/21] core/pldm: Implement PLDM requester

[Frederic Barrat]

On 13/09/2022 12:26, Christophe Lombard wrote:

> diff --git a/core/pldm/pldm-requester.c b/core/pldm/pldm-requester.c
> new file mode 100644
> index 00000000..412d495a
> --- /dev/null
> +++ b/core/pldm/pldm-requester.c
> @@ -0,0 +1,331 @@
> +// 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 <stdio.h>
> +#include <string.h>
> +#include <timebase.h>
> +#include <ast.h>
> +#include <pldm/libpldm/utils.h>
> +#include "pldm.h"
> +
> +struct pldm_request {
> +	struct list_node link;
> +
> +	/* originating request params */
> +	int dest;
> +	int instance;
> +	int cmd_code;
> +	int type;
> +
> +	/* messages requested */
> +	void *request_msg;
> +	int request_len;
> +
> +	/* timeout handling */
> +	struct timer timeout;
> +	uint64_t timeout_ms;
> +	uint64_t start_time;
> +
> +	/* completion callback */
> +	void (*complete)(struct pldm_rx_data *rx, void *data);
> +	void *complete_data;
> +};
> +
> +struct pldm_response {
> +	void **response_msg;
> +	size_t *response_len;
> +	bool done;
> +	int rc;
> +};
> +
> +/* pldm requests queue */
> +static LIST_HEAD(list_pldm_requests);
> +struct lock list_lock;
> +struct timer requests_timer;


Make those 2 struct static.
'list_lock' is pretty generic. Can we rename or at least have a comment 
to say what it's protecting?



> +/*
> + * Handle PLDM message received from the PLDM terminus over MCTP
> + */
> +int pldm_requester_handle_response(struct pldm_rx_data *rx)
> +{
> +	/* check the message received */
> +	if (active_request == NULL) {
> +		prlog(PR_ERR, "%s: No active request. "
> +			      "Response received t:%d c:%d i:%d\n",
> +			      __func__,
> +			      rx->hdrinf.pldm_type,
> +			      rx->hdrinf.command,
> +			      rx->hdrinf.instance);
> +		return OPAL_WRONG_STATE;
> +	}
> +
> +	if (!matches_request(rx, active_request)) {
> +		prlog(PR_ERR, "%s: Unexpected response! t:%d c:%d i:%d want %d,%d,%d\n",
> +			      __func__,
> +			      rx->hdrinf.pldm_type,
> +			      rx->hdrinf.command,
> +			      rx->hdrinf.instance,
> +			      active_request->type,
> +			      active_request->cmd_code,
> +			      active_request->instance
> +			);
> +		return OPAL_WRONG_STATE;
> +	}
> +
> +	/* The expected message seems correct */
> +	handle_response(rx);
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +/*
> + * Send the PLDM request
> + */
> +static void requests_poller(struct timer *t __unused,
> +			    void *data __unused,
> +			    uint64_t now __unused)
> +{


Could probably also be converted to an opal poller and avoid the timer 
management.


> +	int rc = OPAL_SUCCESS;
> +
> +	lock(&list_lock);
> +
> +	/* no new request to handle */
> +	if (list_empty(&list_pldm_requests))
> +		goto out;
> +
> +	/* wait for the end of the processing of the current request */
> +	if (active_request)
> +		goto out;
> +
> +	/* remove the first entry in a list */
> +	active_request = list_pop(&list_pldm_requests,
> +				  struct pldm_request,
> +				  link);


I think we can shorten the duration of holding the 'list_lock' and 
release it here.

   Fred