| Message ID | 1495695955-30718-9-git-send-email-jk@ozlabs.org |
|---|---|
| State | Superseded |
| Headers | show |
On 05/25/2017 12:35 PM, Jeremy Kerr wrote: > With the introductuion of the opaque firmware channel, we want to > support variable-sized messages. Rather than expecting to read an > entire 'struct opal_prd_msg' in one read() call, we can split this > over mutiple reads, potentially expanding our message buffer. > > Signed-off-by: Jeremy Kerr <jk@ozlabs.org> > --- > external/opal-prd/opal-prd.c | 67 ++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 55 insertions(+), 12 deletions(-) > > diff --git a/external/opal-prd/opal-prd.c b/external/opal-prd/opal-prd.c > index c7cdc61..ebd6c0f 100644 > --- a/external/opal-prd/opal-prd.c > +++ b/external/opal-prd/opal-prd.c > @@ -80,6 +80,8 @@ struct opal_prd_ctx { > char *hbrt_file_name; > bool use_syslog; > bool expert_mode; > + struct opal_prd_msg *msg; > + size_t msg_alloc_len; > void (*vlog)(int, const char *, va_list); > }; > > @@ -1225,41 +1227,76 @@ static int handle_msg_occ_reset(struct opal_prd_ctx *ctx, > > static int handle_prd_msg(struct opal_prd_ctx *ctx) > { > - struct opal_prd_msg msg; > + struct opal_prd_msg *msg; > int size; > int rc; > > - rc = read(ctx->fd, &msg, sizeof(msg)); > + msg = ctx->msg; > + > + rc = read(ctx->fd, msg, ctx->msg_alloc_len); > if (rc < 0 && errno == EAGAIN) > return -1; > > - if (rc != sizeof(msg)) { > - pr_log(LOG_WARNING, "FW: Error reading events from OPAL: %m"); > + /* we need at least enough for the message header... */ > + if (rc < 0) { > + pr_log(LOG_WARNING, "FW: error reading from firmware: %m"); > + return -1; > + } > + > + if (rc < sizeof(msg->hdr)) { > + pr_log(LOG_WARNING, "FW: short message read from firmware"); > return -1; > } > > - size = htobe16(msg.hdr.size); > - if (size < sizeof(msg)) { > + /* ... and for the reported message size to be sane */ > + size = htobe16(msg->hdr.size); > + if (size < sizeof(msg->hdr)) { > pr_log(LOG_ERR, "FW: Mismatched message size " > "between opal-prd and firmware " > "(%d from FW, %zd expected)", > - size, sizeof(msg)); > + size, sizeof(msg->hdr)); > return -1; > } > > - switch (msg.hdr.type) { > + /* expand our message buffer if necessary... */ > + if (size > ctx->msg_alloc_len) { > + ctx->msg = msg = realloc(ctx->msg, size); Validate memory allocation ? > + ctx->msg_alloc_len = size; > + } > + > + /* ... and complete the read */ > + if (size > rc) { > + size_t pos; > + > + for (pos = rc; pos < size;) { > + rc = read(ctx->fd, msg + pos, size - pos); > + > + if (rc < 0 && errno == EAGAIN) > + continue; > + > + if (rc <= 0) { > + pr_log(LOG_WARNING, > + "FW: error reading from firmware: %m"); > + return -1; > + } > + > + pos += rc; > + } > + } > + > + switch (msg->hdr.type) { > case OPAL_PRD_MSG_TYPE_ATTN: > - rc = handle_msg_attn(ctx, &msg); > + rc = handle_msg_attn(ctx, msg); > break; > case OPAL_PRD_MSG_TYPE_OCC_RESET: > - rc = handle_msg_occ_reset(ctx, &msg); > + rc = handle_msg_occ_reset(ctx, msg); > break; > case OPAL_PRD_MSG_TYPE_OCC_ERROR: > - rc = handle_msg_occ_error(ctx, &msg); > + rc = handle_msg_occ_error(ctx, msg); > break; > default: > pr_log(LOG_WARNING, "Invalid incoming message type 0x%x", > - msg.hdr.type); > + msg->hdr.type); > return -1; > } > > @@ -1621,6 +1658,10 @@ static int run_prd_daemon(struct opal_prd_ctx *ctx) > ctx->fd = -1; > ctx->socket = -1; > > + /* set up our message buffer */ > + ctx->msg_alloc_len = sizeof(*ctx->msg); > + ctx->msg = malloc(ctx->msg_alloc_len); Validate malloc return value ? -Vasant
Hi Vasant, >> This change provides the facility to invoke HBRT's reset_pm_complex, in > > So is pm_complete replacement to occ_reset? Yes, Dan's reply has details :) >> +static int pm_complex_reset(uint64_t chip) >> +{ >> + int rc; >> + >> + if (hservice_runtime->reset_pm_complex(chip)) { > > if (hservice_runtime->reset_pm_complex) { ? Ah, that would have been a nasty one to debug. Thanks, fixed. > >> - if (!strcmp(str, "occ")) { >> + if (!strcmp(str, "occ") || !strcmp(str, "pm-complex")) { > > Help says only reset option is supported. But this will enable other > options like enable, disable. Yep, that's somewhat intentional at the moment; while enable & disable would work, I don't want to advertise that as such at the moment, as we don't call into the pm_complex equivalents yet. Cheers, Jeremy
On 05/26/2017 05:04 AM, Jeremy Kerr wrote: > Hi Vasant, > >>> This change provides the facility to invoke HBRT's reset_pm_complex, in >> >> So is pm_complete replacement to occ_reset? > > Yes, Dan's reply has details :) Thanks. got it. > >>> +static int pm_complex_reset(uint64_t chip) >>> +{ >>> + int rc; >>> + >>> + if (hservice_runtime->reset_pm_complex(chip)) { >> >> if (hservice_runtime->reset_pm_complex) { ? > > Ah, that would have been a nasty one to debug. Thanks, fixed. > >> >>> - if (!strcmp(str, "occ")) { >>> + if (!strcmp(str, "occ") || !strcmp(str, "pm-complex")) { >> >> Help says only reset option is supported. But this will enable other >> options like enable, disable. > > Yep, that's somewhat intentional at the moment; while enable & disable > would work, I don't want to advertise that as such at the moment, as we > don't call into the pm_complex equivalents yet. > Makes sense. Thanks for the clarification. -Vasant
diff --git a/external/opal-prd/opal-prd.c b/external/opal-prd/opal-prd.c index c7cdc61..ebd6c0f 100644 --- a/external/opal-prd/opal-prd.c +++ b/external/opal-prd/opal-prd.c @@ -80,6 +80,8 @@ struct opal_prd_ctx { char *hbrt_file_name; bool use_syslog; bool expert_mode; + struct opal_prd_msg *msg; + size_t msg_alloc_len; void (*vlog)(int, const char *, va_list); }; @@ -1225,41 +1227,76 @@ static int handle_msg_occ_reset(struct opal_prd_ctx *ctx, static int handle_prd_msg(struct opal_prd_ctx *ctx) { - struct opal_prd_msg msg; + struct opal_prd_msg *msg; int size; int rc; - rc = read(ctx->fd, &msg, sizeof(msg)); + msg = ctx->msg; + + rc = read(ctx->fd, msg, ctx->msg_alloc_len); if (rc < 0 && errno == EAGAIN) return -1; - if (rc != sizeof(msg)) { - pr_log(LOG_WARNING, "FW: Error reading events from OPAL: %m"); + /* we need at least enough for the message header... */ + if (rc < 0) { + pr_log(LOG_WARNING, "FW: error reading from firmware: %m"); + return -1; + } + + if (rc < sizeof(msg->hdr)) { + pr_log(LOG_WARNING, "FW: short message read from firmware"); return -1; } - size = htobe16(msg.hdr.size); - if (size < sizeof(msg)) { + /* ... and for the reported message size to be sane */ + size = htobe16(msg->hdr.size); + if (size < sizeof(msg->hdr)) { pr_log(LOG_ERR, "FW: Mismatched message size " "between opal-prd and firmware " "(%d from FW, %zd expected)", - size, sizeof(msg)); + size, sizeof(msg->hdr)); return -1; } - switch (msg.hdr.type) { + /* expand our message buffer if necessary... */ + if (size > ctx->msg_alloc_len) { + ctx->msg = msg = realloc(ctx->msg, size); + ctx->msg_alloc_len = size; + } + + /* ... and complete the read */ + if (size > rc) { + size_t pos; + + for (pos = rc; pos < size;) { + rc = read(ctx->fd, msg + pos, size - pos); + + if (rc < 0 && errno == EAGAIN) + continue; + + if (rc <= 0) { + pr_log(LOG_WARNING, + "FW: error reading from firmware: %m"); + return -1; + } + + pos += rc; + } + } + + switch (msg->hdr.type) { case OPAL_PRD_MSG_TYPE_ATTN: - rc = handle_msg_attn(ctx, &msg); + rc = handle_msg_attn(ctx, msg); break; case OPAL_PRD_MSG_TYPE_OCC_RESET: - rc = handle_msg_occ_reset(ctx, &msg); + rc = handle_msg_occ_reset(ctx, msg); break; case OPAL_PRD_MSG_TYPE_OCC_ERROR: - rc = handle_msg_occ_error(ctx, &msg); + rc = handle_msg_occ_error(ctx, msg); break; default: pr_log(LOG_WARNING, "Invalid incoming message type 0x%x", - msg.hdr.type); + msg->hdr.type); return -1; } @@ -1621,6 +1658,10 @@ static int run_prd_daemon(struct opal_prd_ctx *ctx) ctx->fd = -1; ctx->socket = -1; + /* set up our message buffer */ + ctx->msg_alloc_len = sizeof(*ctx->msg); + ctx->msg = malloc(ctx->msg_alloc_len); + i2c_init(); #ifdef DEBUG_I2C @@ -1708,6 +1749,8 @@ out_close: close(ctx->fd); if (ctx->socket != -1) close(ctx->socket); + if (ctx->msg) + free(ctx->msg); return rc; }
With the introductuion of the opaque firmware channel, we want to support variable-sized messages. Rather than expecting to read an entire 'struct opal_prd_msg' in one read() call, we can split this over mutiple reads, potentially expanding our message buffer. Signed-off-by: Jeremy Kerr <jk@ozlabs.org> --- external/opal-prd/opal-prd.c | 67 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 55 insertions(+), 12 deletions(-)