<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<br>
<br>
<div class="moz-cite-prefix">Le 12/04/2023 à 17:24, Frederic Barrat
a écrit :<br>
</div>
<blockquote type="cite"
cite="mid:b688092c-b59a-00c2-47c1-00a2c96fbc4c@linux.ibm.com">
<br>
<br>
On 13/09/2022 12:26, Christophe Lombard wrote:
<br>
<br>
<blockquote type="cite">diff --git
a/core/pldm/pldm-platform-requests.c
b/core/pldm/pldm-platform-requests.c
<br>
new file mode 100644
<br>
index 00000000..965820c8
<br>
--- /dev/null
<br>
+++ b/core/pldm/pldm-platform-requests.c
<br>
@@ -0,0 +1,254 @@
<br>
+// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
<br>
+// Copyright 2022 IBM Corp.
<br>
+
<br>
+#define pr_fmt(fmt) "PLDM: " fmt
<br>
+
<br>
+#include <cpu.h>
<br>
+#include <opal.h>
<br>
+#include <stdio.h>
<br>
+#include <string.h>
<br>
+#include <timebase.h>
<br>
+#include <inttypes.h>
<br>
+#include <pldm/libpldm/entity.h>
<br>
+#include <pldm/libpldm/state_set.h>
<br>
+#include <pldm/libpldm/platform.h>
<br>
+#include "pldm.h"
<br>
+
<br>
+#define NO_MORE_PDR_HANDLES 0
<br>
+
<br>
+static pldm_pdr *pdrs_repo;
<br>
+static bool pdr_ready;
<br>
+
<br>
+struct pldm_pdrs {
<br>
+ uint32_t record_hndl;
<br>
+ bool done;
<br>
+ int rc;
<br>
+};
<br>
+
<br>
+static void pdr_init_complete(bool success)
<br>
+{
<br>
+ /* Read not successful, error out and free the buffer */
<br>
+ if (!success) {
<br>
+ pdr_ready = false;
<br>
+
<br>
+ pldm_pdr_destroy(pdrs_repo);
<br>
+ return;
<br>
+ }
<br>
+
<br>
+ /* Mark ready */
<br>
+ pdr_ready = true;
<br>
+}
<br>
+
<br>
+struct get_pdr_response {
<br>
+ uint8_t completion_code;
<br>
+ uint32_t next_record_hndl;
<br>
+ uint32_t next_data_transfer_hndl;
<br>
+ uint8_t transfer_flag;
<br>
+ uint16_t resp_cnt;
<br>
+ uint8_t *record_data;
<br>
+ size_t record_data_length;
<br>
+ uint8_t transfer_crc;
<br>
+};
<br>
+
<br>
+static int encode_and_queue_get_pdr_req(struct pldm_pdrs
*pdrs);
<br>
+
<br>
+static void get_pdr_req_complete(struct pldm_rx_data *rx,
<br>
+ void *data)
<br>
+{
<br>
+ struct pldm_pdrs *pdrs = (struct pldm_pdrs *)data;
<br>
+ uint32_t record_hndl = pdrs->record_hndl;
<br>
+ size_t payload_len;
<br>
+ int rc, i;
<br>
+
<br>
</blockquote>
<br>
<br>
Superfluous empty line in the middle of the variable declarations.
<br>
<br>
</blockquote>
<br>
right !<br>
<br>
<blockquote type="cite"
cite="mid:b688092c-b59a-00c2-47c1-00a2c96fbc4c@linux.ibm.com">
<br>
<blockquote type="cite">+ struct get_pdr_response response;
<br>
+
<br>
+ prlog(PR_DEBUG, "%s - record_hndl: %d\n", __func__,
record_hndl);
<br>
+
<br>
+ /* Decode the message twice; the first time, the payload
buffer
<br>
+ * will be null so that the decoder will simply tell us how
big
<br>
+ * the buffer should be. Then we create a suitable payload
<br>
+ * buffer and call the decoder again, this time with the
real
<br>
+ * buffer so that it can fill it with data from the
message.
<br>
+ *
<br>
+ * transfer_crc is not used in case of PLDM_START_AND_END.
<br>
+ */
<br>
+ payload_len = rx->msg_len - sizeof(struct pldm_msg_hdr);
<br>
</blockquote>
<br>
<br>
In case of a timeout, rx can be NULL and the line above will go
badly.
<br>
<br>
<br>
</blockquote>
<br>
good catch !<br>
<br>
<blockquote type="cite"
cite="mid:b688092c-b59a-00c2-47c1-00a2c96fbc4c@linux.ibm.com">
<blockquote type="cite">+ response.record_data_length = 0;
<br>
+ response.record_data = NULL;
<br>
+
<br>
+ for (i = 0; i < 2; i++) {
<br>
+ rc = decode_get_pdr_resp(
<br>
+ rx->msg, payload_len,
<br>
+ &response.completion_code,
<br>
+ &response.next_record_hndl,
<br>
+ &response.next_data_transfer_hndl,
<br>
+ &response.transfer_flag,
<br>
+ &response.resp_cnt,
<br>
+ response.record_data,
<br>
+ response.record_data_length,
<br>
+ &response.transfer_crc);
<br>
+
<br>
+ if (rc != PLDM_SUCCESS || response.completion_code !=
PLDM_SUCCESS) {
<br>
+ /* Message decoding failed */
<br>
+ prlog(PR_ERR, "Decode GetPDRResp Error (rc: %d, cc:
%d)\n",
<br>
+ rc, response.completion_code);
<br>
+
<br>
+ /* BMC is not ready, try again */
<br>
+ if (response.completion_code ==
PLDM_ERROR_NOT_READY) {
<br>
+ time_wait_ms(500);
<br>
+ encode_and_queue_get_pdr_req(pdrs);
<br>
+ return;
<br>
+ }
<br>
</blockquote>
<br>
<br>
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.
<br>
<br>
<br>
</blockquote>
<br>
<span class="HwtZe" lang="en"><span class="jCAhz ChMk0b"><span
class="ryNqvb">When the bmc restarts, or a specific event is
received (repository change), and the server is still on, we
need to destroy the current repo and collect all PDRs again.<br>
In case the BMC is restarting, </span></span></span><span
class="HwtZe" lang="en"><span class="jCAhz ChMk0b"><span
class="ryNqvb"><span class="HwtZe" lang="en"><span
class="jCAhz ChMk0b"><span class="ryNqvb">it may happen
that this one is not ready for these requests. <br>
<br>
</span></span></span></span></span></span>
<blockquote type="cite"
cite="mid:b688092c-b59a-00c2-47c1-00a2c96fbc4c@linux.ibm.com">
<br>
<blockquote type="cite">+
<br>
+ pdrs->rc = OPAL_PARAMETER;
<br>
+ pdrs->done = true;
<br>
+ return;
<br>
+ }
<br>
+
<br>
+ if (response.record_data == NULL) {
<br>
+ response.record_data_length = response.resp_cnt;
<br>
+ response.record_data = zalloc(response.resp_cnt);
<br>
+ }
<br>
+ }
<br>
+
<br>
+ /* we do not support multipart transfer */
<br>
+ if (response.transfer_flag != PLDM_START_AND_END)
<br>
+ prlog(PR_ERR, "Transfert GetPDRResp not complete,
transfer_flag: %d\n",
<br>
+ response.transfer_flag);
<br>
+
<br>
+ prlog(PR_DEBUG, "%s - record_hndl: %d, next_record_hndl:
%d, resp_cnt: %d\n",
<br>
+ __func__, record_hndl,
<br>
+ response.next_record_hndl,
<br>
+ response.resp_cnt);
<br>
+
<br>
+ /* Add a PDR record to a PDR repository */
<br>
+ pldm_pdr_add(pdrs_repo,
<br>
+ response.record_data,
<br>
+ response.resp_cnt,
<br>
+ record_hndl,
<br>
+ false);
<br>
+
<br>
+ free(response.record_data);
<br>
+
<br>
+ if (response.next_record_hndl != NO_MORE_PDR_HANDLES) {
<br>
+ pdrs->record_hndl = response.next_record_hndl;
<br>
+ encode_and_queue_get_pdr_req(pdrs);
<br>
+ } else {
<br>
+ pdr_init_complete(true);
<br>
</blockquote>
<br>
<br>
Is this call necessary? It looks like it will done again from
pldm_platform_init()
<br>
</blockquote>
<br>
This call is necessary when we collect all pdrs due to BMC restart.<br>
<br>
<blockquote type="cite"
cite="mid:b688092c-b59a-00c2-47c1-00a2c96fbc4c@linux.ibm.com">
<br>
<br>
<br>
<blockquote type="cite">+int pldm_platform_init(void)
<br>
+{
<br>
+ int rc;
<br>
+
<br>
+ /* retrieve all PDRs */
<br>
+ rc = pdrs_init();
<br>
+ if (rc) {
<br>
+ pdr_init_complete(false);
<br>
+ prlog(PR_ERR, "%s - done, rc: %d\n", __func__, rc);
<br>
</blockquote>
<br>
<br>
Maybe a more descriptive error message?
<br>
<br>
</blockquote>
<br>
ok<br>
<br>
<blockquote type="cite"
cite="mid:b688092c-b59a-00c2-47c1-00a2c96fbc4c@linux.ibm.com">
Fred
<br>
</blockquote>
<br>
</body>
</html>