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

[Christophe Lombard]

Le 12/04/2023 à 17:11, Frederic Barrat a écrit :
>
>
> 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?
>
>

ok, will do. Thanks

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

yep. Thanks for this info

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