[Skiboot] [PATCH 2/2] Add SBE driver support
Vasant Hegde
hegdevasant at linux.vnet.ibm.com
Sun Apr 15 18:45:41 AEST 2018
On 04/13/2018 02:47 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2018-04-03 at 17:25 +0530, Vasant Hegde wrote:
>> SBE (Self Boot Engine) on P9 has two different jobs:
>> - Boot the chip up to the point the core is functional
>> - Provide various services like timer, scom, stash MPIPL, etc., at runtime
>>
.../...
>> +
>> +struct p9_sbe {
>> + /* Chip ID to send message */
>> + u32 chip_id;
>> +
>> + /* List to hold SBE queue messages */
>> + struct list_head msg_list;
>> +
>> + struct lock lock;
>> +
>> + enum p9_sbe_mbox_state state;
>> +
>> + /* SBE MBOX message sequence number */
>> + u16 cur_seq;
>> +
>> + /*
>> + * Sending messages to SBE is serialized. We can use
>> + * single buffer per SBE to hold response message.
>> + */
>> + struct p9_sbe_msg *resp;
>
> Not fan of that. You return the response to the user, you've dropped
> the lock, the buffer can be re-used by another user. That's bad.
Yeah. Will fix in next iteration.
>
>> +};
>> +
>> +/* Default SBE chip ID */
>> +static int sbe_default_chip_id = -1;
>> +
>> +#define SBE_STATUS_PRI_SHIFT 0x30
>> +#define SBE_STATUS_SEC_SHIFT 0x20
>> +#define SBE_FFDC_PRESENT PPC_BIT(1)
>> +
>> +static void p9_sbe_timeout_poll(void *user_data __unused);
>> +
>> +static inline bool p9_sbe_ffdc_present(struct p9_sbe_msg *resp)
>> +{
>> + return (resp->reg[0] & SBE_FFDC_PRESENT);
>> +}
>> +
>> +/*
>> + * bit 0 : PCBPIB status present in response
>> + * bit 1 : FFDC package(s) present in response
>> + * bit 2-3 : Reserved
>> + * bit 4-15 : Primary status code
>> + */
>> +static inline u16 p9_sbe_get_primary_rc(struct p9_sbe_msg *resp)
>> +{
>> + return ((resp->reg[0] >> SBE_STATUS_PRI_SHIFT) & 0xfff);
>> +}
>> +
>> +static inline void p9_sbe_set_primary_rc(struct p9_sbe_msg *resp, u64 rc)
>> +{
>> + resp->reg[0] |= (rc << SBE_STATUS_PRI_SHIFT);
>> +}
>> +
>> +static u64 p9_sbe_rreg(u32 chip_id, u64 reg)
>> +{
>> + u64 data = 0;
>> + int rc;
>> +
>> + rc = xscom_read(chip_id, reg, &data);
>> + if (rc != OPAL_SUCCESS) {
>> + prlog(PR_DEBUG, "XSCOM error %d reading reg 0x%llx\n", rc, reg);
>> + return 0;
>> + }
>
> Traditionally, on errors, we return all 1's
Oh yes. I should make it as 0xffffffff.
>
>> +
>> + return be64_to_cpu(data);
>> +}
>> +
>> +static void p9_sbe_reg_dump(u32 chip_id)
>> +{
>> +#define SBE_DUMP_REG_ONE(chip_id, x) \
>> + prlog(PR_DEBUG, " %20s: %016llx\n", #x, p9_sbe_rreg(chip_id, x))
>> +
>> + prlog(PR_DEBUG, "MBOX register dump for chip : %x\n", chip_id);
>> + SBE_DUMP_REG_ONE(chip_id, PSU_SBE_DOORBELL_REG_RW);
>> + SBE_DUMP_REG_ONE(chip_id, PSU_HOST_SBE_MBOX_REG0);
>> + SBE_DUMP_REG_ONE(chip_id, PSU_HOST_SBE_MBOX_REG1);
>> + SBE_DUMP_REG_ONE(chip_id, PSU_HOST_SBE_MBOX_REG2);
>> + SBE_DUMP_REG_ONE(chip_id, PSU_HOST_SBE_MBOX_REG3);
>> + SBE_DUMP_REG_ONE(chip_id, PSU_HOST_DOORBELL_REG_RW);
>> + SBE_DUMP_REG_ONE(chip_id, PSU_HOST_SBE_MBOX_REG4);
>> + SBE_DUMP_REG_ONE(chip_id, PSU_HOST_SBE_MBOX_REG5);
>> + SBE_DUMP_REG_ONE(chip_id, PSU_HOST_SBE_MBOX_REG6);
>> + SBE_DUMP_REG_ONE(chip_id, PSU_HOST_SBE_MBOX_REG7);
>> +}
>> +
>> +void p9_sbe_freemsg(struct p9_sbe_msg *msg)
>> +{
>> + free(msg);
>> +}
>> +
>> +static void p9_sbe_fillmsg(struct p9_sbe_msg *msg, u16 cmd,
>> + u16 ctrl_flag, u64 reg1, u64 reg2, u64 reg3)
>> +{
>> + bool response = !!(ctrl_flag & SBE_CMD_CTRL_RESP_REQ);
>> + u16 flag;
>> +
>> + /*
>> + * Always set ack required flag. SBE will interrupt OPAL once it read
>> + * message from mailbox register. If OPAL is expecting response, then
>> + * it will update message timeout, otherwise it will send next message.
>> + */
>> + flag = ctrl_flag | SBE_CMD_CTRL_ACK_REQ;
>> +
>> + /* Seqence ID is filled by p9_sbe_queue_msg() */
>> + msg->reg[0] = ((u64)flag << 32) | cmd;
>> + msg->reg[1] = reg1;
>> + msg->reg[2] = reg2;
>> + msg->reg[3] = reg3;
>> +
>> + msg->state = sbe_msg_unused;
>> + msg->response = response;
>> +}
>> +
>> +/*
>> + * Handles "command with direct data" format only.
>> + *
>> + * Note: All mbox messages of our interest uses direct data format. If we need
>> + * indirect data format then we may have to enhance this function.
>> + */
>> +struct p9_sbe_msg *p9_sbe_mkmsg(u16 cmd, u16 ctrl_flag,
>> + u64 reg1, u64 reg2, u64 reg3)
>> +{
>> + struct p9_sbe_msg *msg;
>> +
>> + msg = zalloc(sizeof(struct p9_sbe_msg));
>> + if (!msg) {
>> + prlog(PR_ERR, "Failed to allocate memory for SBE message\n");
>> + return NULL;
>> + }
>> +
>> + p9_sbe_fillmsg(msg, cmd, ctrl_flag, reg1, reg2, reg3);
>
> So you aren't setting "resp" here, thus it can be NULL. I'd rather you
> allocated a response here or left it NULL if none is needed, I don't
> like the shared response buffer see below
Sure. I will add separate response message and allocate it only when message
expects response.
>
>> + return msg;
>> +}
>> +
>> +static inline bool p9_sbe_mbox_busy(struct p9_sbe *sbe)
>> +{
>> + return sbe->state != sbe_mbox_idle ? true : false;
>> +}
>> +
>> +static inline bool p9_sbe_msg_busy(struct p9_sbe_msg *msg)
>> +{
>> + switch (msg->state) {
>> + case sbe_msg_unused:
>> + /* fall through */
>> + case sbe_msg_done:
>> + case sbe_msg_timeout:
>> + return false;
>> + default: /* sbe_msg_queued, sbe_msg_sent, sbe_msg_wresp */
>> + break;
>> + }
>> + return true;
>> +}
>> +
>> +static int p9_sbe_msg_send(u32 chip_id, struct p9_sbe_msg *msg)
>> +{
>> + int rc, i;
>> + u64 addr, *data;
>> +
>> + addr = PSU_HOST_SBE_MBOX_REG0;
>> + data = &msg->reg[0];
>> +
>> + for (i = 0; i < NR_HOST_SBE_MBOX_REG; i++) {
>> + rc = xscom_write(chip_id, addr, *data);
>> + if (rc)
>> + return rc;
>> +
>> + addr++;
>> + data++;
>> + }
>> +
>> + rc = xscom_write(chip_id, PSU_SBE_DOORBELL_REG_OR,
>> + HOST_SBE_MSG_WAITING);
>> + return rc;
>> +}
>> +
>> +static int p9_sbe_msg_receive(u32 chip_id, struct p9_sbe_msg *resp)
>> +{
>> + int i;
>> + int rc = OPAL_SUCCESS;
>> + u64 addr, *data;
>> +
>> + addr = PSU_HOST_SBE_MBOX_REG4;
>> + data = &resp->reg[0];
>> +
>> + for (i = 0; i < NR_HOST_SBE_MBOX_REG; i++) {
>> + rc = xscom_read(chip_id, addr, data);
>> + if (rc)
>> + return rc;
>> +
>> + addr++;
>> + data++;
>> + }
>> +
>> + return rc;
>> +}
>> +
>> +/* WARNING: This will drop sbe->lock */
>> +static void p9_sbe_msg_complete(struct p9_sbe *sbe, struct p9_sbe_msg *msg)
>> +{
>> + prlog(PR_TRACE, "Completing msg [chip id = %x], reg0 : 0x%llx\n",
>> + sbe->chip_id, msg->reg[0]);
>> +
>> + if (msg->response)
>> + msg->resp = sbe->resp;
>
> Right so you set the above, then you drop the lock below. At that point
> any other message can start processing and override the response from
> another CPU while the owner of this message tries to read it. Bad.
>
> You really need to allocate a separate response buffer per message.
> What you can do is only allocate if if a response is needed for the
> given message (typically via an argument to mkmsg) but you need to
> null check in various places then.
Fixed.
>
>> +
>> + list_del_from(&sbe->msg_list, &msg->link);
>
> How does that differ from just list_del ? checks the list ?
>
>> + if (msg->complete) {
>> + unlock(&sbe->lock);
>> + msg->complete(msg);
>> + lock(&sbe->lock);
>> + }
>> +}
>> +
>> +/* WARNING: This will drop sbe->lock */
>> +static void p9_sbe_send_complete(struct p9_sbe *sbe)
>> +{
>> + struct p9_sbe_msg *msg;
>> +
>> + if (list_empty(&sbe->msg_list))
>> + return;
>> +
>> + msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
>> + /* Need response */
>> + if (msg->response) {
>> + msg->state = sbe_msg_wresp;
>> + } else {
>> + msg->state = sbe_msg_done;
>> + p9_sbe_msg_complete(sbe, msg);
>> + sbe->state = sbe_mbox_idle;
>> + }
>> +}
>> +
>> +/* WARNING: This will drop sbe->lock */
>> +static void p9_sbe_process_queue(struct p9_sbe *sbe)
>> {
>> int rc;
>> - u64 data;
>> + struct p9_sbe_msg *msg;
>> +
>> + if (p9_sbe_mbox_busy(sbe))
>> + return;
>> +
>> + if (list_empty(&sbe->msg_list))
>> + return;
>> +
>> + msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
>> + /* Send message */
>> + rc = p9_sbe_msg_send(sbe->chip_id, msg);
>> + if (rc) {
>> + prlog(PR_ERR, "Failed to send message to SBE "
>> + "[chip id = %x]\n", sbe->chip_id);
>> + memset(sbe->resp, 0, sizeof(struct p9_sbe_msg));
>
> So that should be msg->resp along with a NULL check
Fixed.
>
>> + p9_sbe_set_primary_rc(sbe->resp, SBE_STATUS_PRI_GENERIC_ERR);
>> + msg->state = sbe_msg_timeout;
>
> This is not a timeout though. Should you have a more generic "error"
> state name instead ?
I will add separate "sbe_msg_error".
>
>> + p9_sbe_msg_complete(sbe, msg);
>> +
>> + /* Try to send next message */
>> + p9_sbe_process_queue(sbe);
>
> Not fan of the recursion here, I would rather have a loop.
Its in error path. Only when first message sending fails we enter recursion to
send second message. Do you really want to convert this to loop?
>
>> + return;
>> + }
>> +
>> + prlog(PR_TRACE, "Message queued [chip id = 0x%x]:\n\t Reg0 : %016llx"
>> + "\n\t Reg1 : %016llx\n\t Reg2 : %016llx\n\t Reg3 : %016llx\n",
>> + sbe->chip_id, msg->reg[0], msg->reg[1], msg->reg[2], msg->reg[3]);
>> +
>> + msg->timeout = mftb() + msecs_to_tb(SBE_CMD_TIMEOUT_MAX);
>> + sbe->state = sbe_mbox_send;
>> + msg->state = sbe_msg_sent;
>> +}
>> +
>> +/*
>> + * WARNING:
>> + * Only one command is accepted in the command buffer until response
>> + * to the command is enqueued in the response buffer by SBE.
>> + *
>> + * Head of msg_list contains in-flight message. Hence we should always
>> + * add new message to tail of the list.
>> + */
>> +int p9_sbe_queue_msg(u32 chip_id, struct p9_sbe_msg *msg,
>> + void (*comp)(struct p9_sbe_msg *msg))
>> +{
>> struct proc_chip *chip;
>> + struct p9_sbe *sbe;
>> +
>> + if (!msg)
>> + return OPAL_PARAMETER;
>> +
>> + if (sbe_default_chip_id == -1)
>> + return OPAL_HARDWARE;
>
> Why ? I would move the above test inside the if (chip_id == -1)
> below...
Agree.
>
>> +
>> + /* Default to SBE on master chip */
>> + if (chip_id == -1)
>> + chip = get_chip(sbe_default_chip_id);
>> + else
>> + chip = get_chip(chip_id);
>> +
>> + if (chip == NULL || chip->sbe == NULL)
>> + return OPAL_INTERNAL_ERROR;
>> +
>> + sbe = chip->sbe;
>> + lock(&sbe->lock);
>> + /* Set completion and update sequence number */
>> + msg->complete = comp;
>> + msg->state = sbe_msg_queued;
>> + msg->reg[0] = msg->reg[0] | ((u64)sbe->cur_seq << 16);
>> + sbe->cur_seq++;
>> +
>> + /* Reset sequence number */
>> + if (sbe->cur_seq == 0xffff)
>> + sbe->cur_seq = 1;
>
> So never 0 ?
We can start with 0. But generally when message allocated its set to 0. So I
started from 1.
>
> Also how are the sequence numbers used by the SBE ? (Are they at all ?)
> because cancelling a message means creating a "hole" in the sequence
> number chain, is that ok with the SBE ?
Sequence number is used to identify the message. When OPAL expects ACK/response
from SBE they will pass back the sequence number we had sent.
SBE don't really check whether we had hole or not. They just need sequence
number. So we are good here.
>
>> +
>> + /* Add message to queue */
>> + list_add_tail(&sbe->msg_list, &msg->link);
>> + p9_sbe_process_queue(sbe);
>> + unlock(&sbe->lock);
>> +
>> + return OPAL_SUCCESS;
>> +}
>> +
>> +int p9_sbe_sync_msg(u32 chip_id, struct p9_sbe_msg *msg, bool autofree)
>> +{
>> + int rc;
>> +
>> + rc = p9_sbe_queue_msg(chip_id, msg, NULL);
>> + if (rc)
>> + goto free_msg;
>> +
>> + while (p9_sbe_msg_busy(msg)) {
>> + cpu_relax();
>> + p9_sbe_timeout_poll(NULL);
>
> So I'm thinking we should have an p9_sbe_poll_one() that takes the chip
> as an argument, unless you really want to poll all sbes in that loop ?
Here we just need to poll only one SBE not all.
Added p9_sbe_poll_one(struct p9_sbe *msg) function.
>
> Another option is to run all pollers like we do for the FSP driver, but
> that's more heavy handed (though will allow other things to work while
> we wait for the SBE).
>
>> + }
>> +
>> + if (msg->state == sbe_msg_done)
>> + rc = 0;
>> + else
>> + rc = -1;
>
> Any better error code here ? Maybe have a function that convert the SBE
> errors into nice OPAL errors.
Converted to SBE error messages.
>
>> +
>> + if (msg->response && msg->resp)
>> + rc = p9_sbe_get_primary_rc(msg->resp);
>
> Same deal, should we translate the RC or pass the "raw" SBE RC up ?
> Either works but in any case, the "-1" above should be some constant.
Fixed.
>
>> +free_msg:
>> + if (autofree)
>> + p9_sbe_freemsg(msg);
>> +
>> + return rc;
>> +}
>> +
>> +/* Remove SBE message from queue. It will not remove inflight message */
>> +int p9_sbe_cancelmsg(u32 chip_id, struct p9_sbe_msg *msg)
>> +{
>> + struct proc_chip *chip;
>> + struct p9_sbe *sbe;
>> +
>> + if (chip_id == -1)
>> + chip = get_chip(sbe_default_chip_id);
>> + else
>> + chip = get_chip(chip_id);
>> + if (chip == NULL || chip->sbe == NULL)
>> + return OPAL_PARAMETER;
>> +
>> + sbe = chip->sbe;
>> +
>> + lock(&sbe->lock);
>> + if (msg->state != sbe_msg_queued) {
>> + unlock(&sbe->lock);
>> + return OPAL_BUSY;
>> + }
>> +
>> + list_del_from(&sbe->msg_list, &msg->link);
>> + msg->state = sbe_msg_done;
>> + unlock(&sbe->lock);
>> +
>> + return OPAL_SUCCESS;
>> +}
>> +
>> +static void p9_sbe_handle_response(struct p9_sbe *sbe, struct p9_sbe_msg *msg)
>> +{
>> + u16 send_seq, resp_seq;
>> + int rc;
>> +
>> + if (msg == NULL)
>> + return;
>> +
>> + /* Clear response message */
>> + memset(sbe->resp, 0, sizeof(struct p9_sbe_msg));
>
> Don't use sbe->resp, use msg->resp and null check as described earlier
Fixed.
>
>> +
>> + rc = p9_sbe_msg_receive(sbe->chip_id, sbe->resp);
>> + if (rc != OPAL_SUCCESS) {
>> + prlog(PR_ERR, "Failed to read response message "
>> + "[chip id = %x]\n", sbe->chip_id);
>> + p9_sbe_set_primary_rc(sbe->resp, SBE_STATUS_PRI_GENERIC_ERR);
>> + return;
>> + }
>> +
>> + /* Validate sequence number */
>> + send_seq = (msg->reg[0] >> 16) & 0xffff;
>> + resp_seq = (sbe->resp->reg[0] >> 16) & 0xffff;
>> + if (send_seq != resp_seq) {
>> + /*
>> + * XXX Handle SBE R/R.
>> + * Lets send sequence error to caller until SBE reset works.
>> + */
>> + prlog(PR_ERR, "Invalid sequence id [chip id = %x]\n",
>> + sbe->chip_id);
>> + p9_sbe_set_primary_rc(sbe->resp, SBE_STATUS_PRI_SEQ_ERR);
>> + return;
>> + }
>> +}
>> +
>> +void p9_sbe_interrupt(uint32_t chip_id)
>> +{
>> + int rc;
>> + u64 data = 0, val;
>> + struct proc_chip *chip;
>> + struct p9_sbe *sbe;
>> + struct p9_sbe_msg *msg = NULL;
>>
>> chip = get_chip(chip_id);
>> - if (chip == NULL)
>> + if (chip == NULL || chip->sbe == NULL)
>> return;
>> + sbe = chip->sbe;
>>
>> + lock(&sbe->lock);
>> /* Read doorbell register */
>> rc = xscom_read(chip_id, PSU_HOST_DOORBELL_REG_RW, &data);
>> if (rc) {
>> prlog(PR_ERR, "Failed to read SBE to Host doorbell register "
>> "[chip id = %x]\n", chip_id);
>> - goto clr_interrupt;
>> + p9_sbe_reg_dump(chip_id);
>> + unlock(&sbe->lock);
>> + return;
>> + }
>> +
>> + /* Called from poller path (p9_sbe_timeout_poll), nothing to process */
>> + if (!data) {
>> + unlock(&sbe->lock);
>> + return;
>> }
>>
>> - /* SBE passtrhough command, call prd handler */
>> + /* SBE passthrough command, call prd handler */
>> if (data & SBE_HOST_PASSTHROUGH) {
>> prd_sbe_passthrough(chip_id);
>> }
>>
>> + /* SBE came back from reset */
>> + if (data & SBE_HOST_RESET) {
>> + prlog(PR_NOTICE, "Back from reset [chip id = %x]\n", chip_id);
>> + /* Reset SBE MBOX state */
>> + sbe->state = sbe_mbox_idle;
>> +
>> + /* Reset message state */
>> + if (!list_empty(&sbe->msg_list)) {
>> + msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
>> + msg->state = sbe_msg_queued;
>> + }
>> + goto clr_interrupt;
>> + }
>> +
>> + /* SBE read message from mailbox register */
>> + if (data & SBE_HOST_MSG_READ) {
>> + p9_sbe_send_complete(sbe);
>> + }
>> +
>> + /* Read SBE response before clearing doorbell register */
>> + if (data & SBE_HOST_RESPONSE_WAITING) {
>> + if (!list_empty(&sbe->msg_list)) {
>> + msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
>> + p9_sbe_handle_response(sbe, msg);
>> + msg->state = sbe_msg_done;
>> + p9_sbe_msg_complete(sbe, msg);
>> + }
>> + sbe->state = sbe_mbox_idle;
>> + }
>
> Nervous here... msg_complete drops the lock, so all sort of things can
> have happened. We should clear the interrupt first I think, before we
> process things, and go straight to process_queue at the bottom after
> having called msg_complete. Also see my comment in the timeout code
> about when you set mbox_idle. Makes me nervous here too.
Yeah. I will read response data, clear interrupt and then process interrupt bits.
>
>> clr_interrupt:
>> - /* Clears all the bits */
>> - rc = xscom_write(chip_id, PSU_HOST_DOORBELL_REG_AND,
>> - SBE_HOST_RESPONSE_CLEAR);
>> + /* Clear doorbell register */
>> + val = SBE_HOST_RESPONSE_MASK & ~data;
>> + rc = xscom_write(chip_id, PSU_HOST_DOORBELL_REG_AND, val);
>> if (rc) {
>> prlog(PR_ERR, "Failed to clear SBE to Host doorbell "
>> - "register [chip id = %x]\n", chip_id);
>> + "interrupt [chip id = %x]\n", chip_id);
>> + unlock(&sbe->lock);
>> + return;
>> + }
>> +
>> + p9_sbe_process_queue(sbe);
>> + unlock(&sbe->lock);
>> +}
>> +
>> +static void p9_sbe_timeout_poll(void *user_data __unused)
>> +{
>> + struct p9_sbe *sbe;
>> + struct p9_sbe_msg *msg;
>> + struct proc_chip *chip;
>> +
>> + for_each_chip(chip) {
>> + if (chip->sbe == NULL)
>> + continue;
>> + sbe = chip->sbe;
>> +
>> + if (list_empty(&sbe->msg_list))
>> + continue;
>
> This is racy when running in debug mode, we've hit that with the timers
> too, use list_empty_nocheck (see check_timers)
Fixed.
>
>> +
>> + /*
>> + * In some cases there will be a delay in calling OPAL interrupt
>> + * handler routine (opal_handle_interrupt). In such cases its
>> + * possible that SBE has responded, but OPAL didn't act on that.
>> + * Hence check for SBE response.
>> + */
>> + p9_sbe_interrupt(sbe->chip_id);
>> +
>> + lock(&sbe->lock);
>
> This is unfortunate as sbe_interrupt takes and releases the lock, maybe
> use a __p9_sbe_interrupt() that doesn't (and takes a sbe pointer rather
> than a chip as an argument since you have that already) and call *that*
> from here, and p9_sbe_interrupt() is just a wrapper. something like:
Yeah.. Too many places I've added unlock().
>
> void p9_sbe_interrupt(uint32_t chip_id)
> {
> struct proc_chip *chip;
> struct p9_sbe *sbe;
>
> chip = get_chip(chip_id);
> f (chip == NULL || chip->sbe == NULL)
> return;
> sbe = chip->sbe;
> lock(&sbe->lock);
> __p9_sbe_interrupt(sbe);
> unlock(&sbe->lock);
> }
Yep. Added separate function like above.
>
>> + if (list_empty(&sbe->msg_list)) {
>> + unlock(&sbe->lock);
>> + continue;
>> + }
>
> If you had a per-chip poll routine without an outer wrapper that
> iterate all chips, all of these constructs could be simple goto out;
> with a single unlock.
Fixed.
>
>> +
>> + msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
>> + if (tb_compare(mftb(), msg->timeout) != TB_AAFTERB) {
>> + unlock(&sbe->lock);
>> + continue;
>> + }
>> +
>> + /* Message timeout */
>> + prlog(PR_ERR, "Message timeout [chip id = %x], "
>> + "cmd = %llx, subcmd = %llx\n", sbe->chip_id,
>> + (msg->reg[0] >> 8) & 0xff, msg->reg[0] & 0xff);
>> + p9_sbe_reg_dump(sbe->chip_id);
>> + memset(sbe->resp, 0, sizeof(struct p9_sbe_msg));
>
> Usual comment about resp...
Fixed.
>
>> + p9_sbe_set_primary_rc(sbe->resp, SBE_STATUS_PRI_GENERIC_ERR);
>> + msg->state = sbe_msg_timeout;
>> + p9_sbe_msg_complete(sbe, msg);
>> +
>> + /* XXX Handle SBE R/R. Reset SBE state until SBE R/R works. */
>> + sbe->state = sbe_mbox_idle;
>
> This makes me nervous. If for any reason the message is top of the
> queue but hasn't started sending yet, you could be already in idle
> state before you call p9_sbe_msg_complete(). If that's the case, the
> unlock inside p9_sbe_msg_complete() means somebody else can restart
> the queue with another message, thus making the above state change to
> idle a bug.
>
> Now it's possible you don't have such a reason where the state would go
> idle while there's something in the queue and drop the lock, but as I
> said, it makes me somewhat nervous.
>
> I would much rather have the idle set before we call msg_complete to
> make sure that at the point of unlock inside it, we are completely
> in sync between not having the message and being idle. Also comment :-)
I will set sbe->state before calling msg_complete().
Thanks!
-Vasant
More information about the Skiboot
mailing list