Re: [PATCH v6 1/4] ipmi: ssif_bmc: Add SSIF BMC driver

From: Quan Nguyen
Date: Fri Apr 08 2022 - 00:41:29 EST



On 17/03/2022 20:13, Corey Minyard wrote:
snip...
+
+static void response_timeout(struct timer_list *t)
+{
+ struct ssif_bmc_ctx *ssif_bmc = from_timer(ssif_bmc, t, response_timer);
+ unsigned long flags;
+

Is there a possible race here? The timeout can happen at the same time
as a received message, will something bad happen if that's the case?


Thanks Corey,
I think I need extra comment here.

The purpose of this timeout is to make sure ssif_bmc will recover from busy
state in case the upper stack does not provide the response.
Hence, the response timeout is set as 500ms, twice the time of max
Request-to-Response in spec as the code below. Should it be longer?

That's not what I was asking. I know what the timer is for. But what
happens if the response comes in at the same time this timer goes off?
What will keep the data from getting messed up?


Dear Corey, thanks for pointing this out.

In case the response comes in at the same time this timer goes off, both timeout handler and the ssif_bmc_write must first win to acquire the lock first, eg: ssif_bmc->lock

If timeout handler wins it firstly test ssif_bmc->response_in_progress to make sure if the ssif_bmc_write() is succeeded. If not, ssif_bmc->response_in_progress is false, then set the ssif_bmc->busy and ssif_bmc->response_timer_inited flags to false, and set the flag ssif_bmc->aborting = true to start aborting the current response.

If ssif_bmc_write() wins, it then first test if the timer not yet goes off, ie: ssif_bmc->response_timer_inited is true, if so, del the timer and let the response ready to send back to host. If not, make ssif_bmc_write() to return -EINVAL as the driver had already aborted the response and wait for the new request.

This will be included in my next version.


As per spec, the max Request-to-Respose would not exceed 250ms.

I put the comment in ssif_bmc.h as below:
+/*
+ * IPMI 2.0 Spec, section 12.7 SSIF Timing,
+ * Request-to-Response Time is T6max(250ms) - T1max(20ms) - 3ms = 227ms
+ * Recover ssif_bmc from busy state if it takes upto 500ms
+ */
+#define RESPONSE_TIMEOUT 500 /* ms */


+ spin_lock_irqsave(&ssif_bmc->lock, flags);
+
+ /* Recover ssif_bmc from busy */
+ ssif_bmc->busy = false;
+ del_timer(&ssif_bmc->response_timer);

You don't need to delete the timer, it's in the timeout.


Will remove this redundant code in next version

+ ssif_bmc->response_timer_inited = false;
+
+ spin_unlock_irqrestore(&ssif_bmc->lock, flags);
+}
+
+/* Called with ssif_bmc->lock held. */
+static void handle_request(struct ssif_bmc_ctx *ssif_bmc)
+{
+ /* set ssif_bmc to busy waiting for response */
+ ssif_bmc->busy = true;
+
+ /* Request message is available to process */
+ ssif_bmc->request_available = true;
+
+ /* Clean old response buffer */
+ memset(&ssif_bmc->response, 0, sizeof(struct ssif_msg));
+
+ /* This is the new READ request.*/
+ wake_up_all(&ssif_bmc->wait_queue);
+
+ /* Armed timer to recover slave from busy state in case of no response */
+ if (!ssif_bmc->response_timer_inited) {
+ timer_setup(&ssif_bmc->response_timer, response_timeout, 0);
+ ssif_bmc->response_timer_inited = true;
+ }
+ mod_timer(&ssif_bmc->response_timer, jiffies + msecs_to_jiffies(RESPONSE_TIMEOUT));
+}
+
+static void set_multipart_response_buffer(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
+{
+ u8 response_len = 0;
+ int idx = 0;
+ u8 data_len;
+
+ data_len = ssif_bmc->response.len;
+ switch (ssif_bmc->smbus_cmd) {
+ case SSIF_IPMI_MULTIPART_READ_START:
+ /*
+ * Read Start length is 32 bytes.
+ * Read Start transfer first 30 bytes of IPMI response
+ * and 2 special code 0x00, 0x01.
+ */
+ *val = MAX_PAYLOAD_PER_TRANSACTION;
+ ssif_bmc->remain_len = data_len - MAX_IPMI_DATA_PER_START_TRANSACTION;
+ ssif_bmc->block_num = 0;

Do you need to validate the data length before using this?
This applies for lots of places through here.


set_multipart_response_buffer() is called only when ->is_singlepart_read is
false, which is determined by the below code under the *_write()

ssif_bmc->is_singlepart_read = (ssif_msg_len(&msg) <=
MAX_PAYLOAD_PER_TRANSACTION + 1);

So, I think the len is validated and could be use in this functions.

Ah, I had things backwards. "Read" here is when you are writing to
the other side. I understand now.


+
+ ssif_bmc->response_buf[idx++] = 0x00; /* Start Flag */
+ ssif_bmc->response_buf[idx++] = 0x01; /* Start Flag */
+ ssif_bmc->response_buf[idx++] = ssif_bmc->response.netfn_lun;
+ ssif_bmc->response_buf[idx++] = ssif_bmc->response.cmd;
+ ssif_bmc->response_buf[idx++] = ssif_bmc->response.payload[0];
+
+ response_len = MAX_PAYLOAD_PER_TRANSACTION - idx;
+
+ memcpy(&ssif_bmc->response_buf[idx], &ssif_bmc->response.payload[1],
+ response_len);
+ break;
+
+ case SSIF_IPMI_MULTIPART_READ_MIDDLE:
+ /*
+ * IPMI READ Middle or READ End messages can carry up to 31 bytes
+ * IPMI data plus block number byte.
+ */
+ if (ssif_bmc->remain_len < MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION) {
+ /*
+ * This is READ End message
+ * Return length is the remaining response data length
+ * plus block number
+ * Block number 0xFF is to indicate this is last message
+ *
+ */
+ *val = ssif_bmc->remain_len + 1;
+ ssif_bmc->block_num = 0xFF;
+ ssif_bmc->response_buf[idx++] = ssif_bmc->block_num;
+ response_len = ssif_bmc->remain_len;
+ /* Clean the buffer */
+ memset(&ssif_bmc->response_buf[idx], 0, MAX_PAYLOAD_PER_TRANSACTION - idx);
+ } else {
+ /*
+ * This is READ Middle message
+ * Response length is the maximum SMBUS transfer length
+ * Block number byte is incremented
+ * Return length is maximum SMBUS transfer length
+ */
+ *val = MAX_PAYLOAD_PER_TRANSACTION;
+ ssif_bmc->remain_len -= MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION;
+ response_len = MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION;
+ ssif_bmc->response_buf[idx++] = ssif_bmc->block_num;
+ ssif_bmc->block_num++;
+ }
+
+ memcpy(&ssif_bmc->response_buf[idx],
+ ssif_bmc->response.payload + 1 + ssif_bmc->nbytes_processed,
+ response_len);
+ break;
+
+ default:
+ /* Do not expect to go to this case */
+ dev_err(&ssif_bmc->client->dev,
+ "%s: Unexpected SMBus command 0x%x\n",
+ __func__, ssif_bmc->smbus_cmd);
+ break;
+ }
+
+ ssif_bmc->nbytes_processed += response_len;
+}
+
+/* Process the IPMI response that will be read by master */
+static void handle_read_processed(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
+{
+ u8 *buf;
+ u8 pec_len, addr, len;
+ u8 pec = 0;
+
+ pec_len = ssif_bmc->pec_support ? 1 : 0;
+ /* PEC - Start Read Address */
+ addr = GET_8BIT_ADDR(ssif_bmc->client->addr);
+ pec = i2c_smbus_pec(pec, &addr, 1);
+ /* PEC - SSIF Command */
+ pec = i2c_smbus_pec(pec, &ssif_bmc->smbus_cmd, 1);
+ /* PEC - Restart Write Address */
+ addr = addr | 0x01;
+ pec = i2c_smbus_pec(pec, &addr, 1);
+
+ if (ssif_bmc->is_singlepart_read) {
+ /* Single-part Read processing */
+ buf = (u8 *)&ssif_bmc->response;
+
+ if (ssif_bmc->response.len && ssif_bmc->msg_idx < ssif_bmc->response.len) {
+ ssif_bmc->msg_idx++;
+ *val = buf[ssif_bmc->msg_idx];
+ } else if (ssif_bmc->response.len && ssif_bmc->msg_idx == ssif_bmc->response.len) {
+ ssif_bmc->msg_idx++;
+ *val = i2c_smbus_pec(pec, buf, ssif_msg_len(&ssif_bmc->response));
+ } else {

I thought for a second that this was wrong, but I think it's correct.
Supply zero if you don't have data.

+ *val = 0;
+ }
+ /* Invalidate response buffer to denote it is sent */
+ if (ssif_bmc->msg_idx + 1 >= (ssif_msg_len(&ssif_bmc->response) + pec_len))
+ complete_response(ssif_bmc);
+ } else {
+ /* Multi-part Read processing */

You don't check the length here like you did above. I think that's
required.


As per my explanation above, the ->is_singlepart_read is determined by
testing the length, so it is validated as I assumed.

+ switch (ssif_bmc->smbus_cmd) {
+ case SSIF_IPMI_MULTIPART_READ_START:
+ case SSIF_IPMI_MULTIPART_READ_MIDDLE:
+ buf = (u8 *)&ssif_bmc->response_buf;
+ *val = buf[ssif_bmc->msg_idx];
+ ssif_bmc->msg_idx++;
+ break;
+ default:
+ /* Do not expect to go to this case */
+ dev_err(&ssif_bmc->client->dev,
+ "%s: Unexpected SMBus command 0x%x\n",
+ __func__, ssif_bmc->smbus_cmd);
+ break;
+ }
+
+ len = (ssif_bmc->block_num == 0xFF) ?
+ ssif_bmc->remain_len + 1 : MAX_PAYLOAD_PER_TRANSACTION;
+ if (ssif_bmc->msg_idx == (len + 1)) {
+ pec = i2c_smbus_pec(pec, &len, 1);
+ *val = i2c_smbus_pec(pec, ssif_bmc->response_buf, len);
+ }
+ /* Invalidate response buffer to denote last response is sent */
+ if (ssif_bmc->block_num == 0xFF &&
+ ssif_bmc->msg_idx > (ssif_bmc->remain_len + pec_len)) {
+ complete_response(ssif_bmc);
+ }
+ }
+}
+
+static void handle_write_received(struct ssif_bmc_ctx *ssif_bmc, u8 *val)
+{
+ u8 *buf = (u8 *)&ssif_bmc->request;
+
+ if (ssif_bmc->msg_idx >= sizeof(struct ssif_msg))
+ return;

I don't think this check is valid. I believe the msg_idx only covers
the current message, but ssif_msg is a full multi-part message. It
covers the single-part message, I think but not the multi-part ones.
Also, abort the operation and log on bad data.


Yes, thank you for this catch.

Will change in next version.

+
+ switch (ssif_bmc->smbus_cmd) {
+ case SSIF_IPMI_SINGLEPART_WRITE:
+ buf[ssif_bmc->msg_idx - 1] = *val;
+ ssif_bmc->msg_idx++;
+
+ break;
+ case SSIF_IPMI_MULTIPART_WRITE_START:
+ if (ssif_bmc->msg_idx == 1)
+ ssif_bmc->request.len = 0;
+
+ fallthrough;
+ case SSIF_IPMI_MULTIPART_WRITE_MIDDLE:
+ /* The len should always be 32 */
+ if (ssif_bmc->msg_idx == 1 && *val != MAX_PAYLOAD_PER_TRANSACTION)
+ dev_warn(&ssif_bmc->client->dev,
+ "Warn: Invalid Multipart Write len");

You should abort the operation here. Don't deliver obviously bad data.
Same in the code just below.

This will require that you add a message aborted type of state to just
ignore everything that comes in until the full sequence ends or a new
message starts.


Will introduce the abort state which will ignore everything until the new request comes to handle those invalid cases.

+
+ fallthrough;
+ case SSIF_IPMI_MULTIPART_WRITE_END:
+ /* Multi-part write, 2nd byte received is length */
+ if (ssif_bmc->msg_idx == 1) {
+ if (*val > MAX_PAYLOAD_PER_TRANSACTION)
+ dev_warn(&ssif_bmc->client->dev,
+ "Warn: Invalid Multipart Write End len");
+
+ ssif_bmc->request.len += *val;
+ ssif_bmc->recv_len = *val;
+
+ /* request len should never exceeded 255 bytes */
+ if (ssif_bmc->request.len > 255)
+ dev_warn(&ssif_bmc->client->dev,
+ "Warn: Invalid request len");
+
+ } else {

You check msg_idx above, but I'm not sure that check will cover this
operation.

That check is to make sure the length (*val) must always be strictly 32
bytes in case of MULTIPART_WRITE_START/MIDDLE. And this check allows the
length is up to 32 bytes in MULTIPART_WRITE_END.

Now that I have read and write straight, this is where the previous
comments apply.

You are trusting the the length sent by the remote end in the second
byte is correct, but there is no guarantee of this. The remote end can
send as many bytes as it likes. You need to check that you don't
overflow buf here and that it actually sends the number of bytes that it
said it was going to send to avoid underflow.


Will include in next version. The request which is exceeded the 255 bytes should be aborted.


+ buf[ssif_bmc->msg_idx - 1 +
+ ssif_bmc->request.len - ssif_bmc->recv_len] = *val;

This computation is fairly complicated and hard to understand.
Calculations like this are asking for trouble.

It would be easier to understand had request.len be the current length
of what is in request.payload and increment it on every incoming byte.
Then request.len could be used to add data to the buffer, like

if (ssif_bmc->request.len >= sizeof(ssif_bmc->payload))
error...
ssif_bmc->payload[ssif_bmc->request.len++] = *val;

If you renamed msg_idx to curr_recv_idx and recv_len to curr_recv_len,
it would be more clear that these are related and operate on the current
incoming message.

It would also be nice to get rid of the casts from ssif_msg to a buffer
array and just index directly into request.payload[].


Really appreciate for these comments, Corey.
I have rechecked the code and there will be, definitely, changes to refactor this code in my next version.

In thinking about this further, I have a few more observations...

There is no need to have the netfn and cmd in ssif_msg. They are just
the first and second bytes of the message. You don't care what they
are in this code.

Agree. Will change in next version

Why do you deliver the length as part of the message to the user? The
length is returned by the system call. You have all these +1 and -1
things around the message length, which is error-prone. Removing the
length from the message would get rid of all of that. And using packed
structures is generally not the best, anyway.


Will avoid those +1, -1 in next version.
About the packed structures, I think it is needed because we want to just copy the whole request/response struct from/to user space.

The PEC calculations remove one byte from the maximum message length.
Since they are not included in the length byte, it's kind of unnatural
to do this the way you are doing it. Instead, it might be best to say
if you receive a byte and curr_recv_idx == curr_recv_len, process it
as PEC. That way the PEC never hits the buffer.

There is no need for msg_idx, or cur_recv_idx, to be size_t.

I need to look at this some more, but I'll need to see the rewrite.

-corey

Thanks Corey,

Will address these suggestions on next version.

- Quan