Message ID | 20181025150452.25543-2-manoj.iyer@canonical.com |
---|---|
State | New |
Headers | show |
Series | ipmi:ssif: Add support for multi-part transmit messages > 2 parts | expand |
On 25.10.18 17:04, Manoj Iyer wrote: > From: Corey Minyard <cminyard@mvista.com> > > The spec was fairly confusing about how multi-part transmit messages > worked, so the original implementation only added support for two > part messages. But after talking about it with others and finding > something I missed, I think it makes more sense. > > The spec mentions smbus command 8 in a table at the end of the > section on SSIF support as the end transaction. If that works, > then all is good and as it should be. However, some implementations > seem to use a middle transaction <32 bytes tomark the end because of the > confusion in the spec, even though that is an SMBus violation if > the number of bytes is zero. > > So this change adds some tests, if command=8 works, it uses that, > otherwise if an empty end transaction works, it uses a middle > transaction <32 bytes to mark the end. If neither works, then > it limits the size to 63 bytes as it is now. > > BugLink: https://bugs.launchpad.net/bugs/1799794 > > Cc: Harri Hakkarainen <harri@cavium.com> > Cc: Bazhenov, Dmitry <dmitry.bazhenov@auriga.com> > Cc: Mach, Dat <Dat.Mach@cavium.com> > Signed-off-by: Corey Minyard <cminyard@mvista.com> > (cherry picked from commit 10042504ed92c06077b8a20a4edd67ba784847d4 > linux-next) > Signed-off-by: Manoj Iyer <manoj.iyer@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- Reading through the patch I believe it add enough checks to fall back to current behaviour if the additional methods fail. It is just about on the thin line between new feature and fixing a bug. However this made it into upstream as of v4.20-rc1 (same sha1), so we can drop the linux-next part of the s-o-b section. -Stefan > drivers/char/ipmi/ipmi_ssif.c | 209 ++++++++++++++++++++++++++-------- > 1 file changed, 159 insertions(+), 50 deletions(-) > > diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c > index 265d6a6583bc..783af69172c4 100644 > --- a/drivers/char/ipmi/ipmi_ssif.c > +++ b/drivers/char/ipmi/ipmi_ssif.c > @@ -60,6 +60,7 @@ > #define SSIF_IPMI_REQUEST 2 > #define SSIF_IPMI_MULTI_PART_REQUEST_START 6 > #define SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE 7 > +#define SSIF_IPMI_MULTI_PART_REQUEST_END 8 > #define SSIF_IPMI_RESPONSE 3 > #define SSIF_IPMI_MULTI_PART_RESPONSE_MIDDLE 9 > > @@ -271,6 +272,7 @@ struct ssif_info { > /* Info from SSIF cmd */ > unsigned char max_xmit_msg_size; > unsigned char max_recv_msg_size; > + bool cmd8_works; /* See test_multipart_messages() for details. */ > unsigned int multi_support; > int supports_pec; > > @@ -887,32 +889,33 @@ static void msg_written_handler(struct ssif_info *ssif_info, int result, > * in the SSIF_MULTI_n_PART case in the probe function > * for details on the intricacies of this. > */ > - int left; > + int left, to_write; > unsigned char *data_to_send; > + unsigned char cmd; > > ssif_inc_stat(ssif_info, sent_messages_parts); > > left = ssif_info->multi_len - ssif_info->multi_pos; > - if (left > 32) > - left = 32; > + to_write = left; > + if (to_write > 32) > + to_write = 32; > /* Length byte. */ > - ssif_info->multi_data[ssif_info->multi_pos] = left; > + ssif_info->multi_data[ssif_info->multi_pos] = to_write; > data_to_send = ssif_info->multi_data + ssif_info->multi_pos; > - ssif_info->multi_pos += left; > - if (left < 32) > - /* > - * Write is finished. Note that we must end > - * with a write of less than 32 bytes to > - * complete the transaction, even if it is > - * zero bytes. > - */ > + ssif_info->multi_pos += to_write; > + cmd = SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE; > + if (ssif_info->cmd8_works) { > + if (left == to_write) { > + cmd = SSIF_IPMI_MULTI_PART_REQUEST_END; > + ssif_info->multi_data = NULL; > + } > + } else if (to_write < 32) { > ssif_info->multi_data = NULL; > + } > > rv = ssif_i2c_send(ssif_info, msg_written_handler, > - I2C_SMBUS_WRITE, > - SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE, > - data_to_send, > - I2C_SMBUS_BLOCK_DATA); > + I2C_SMBUS_WRITE, cmd, > + data_to_send, I2C_SMBUS_BLOCK_DATA); > if (rv < 0) { > /* request failed, just return the error. */ > ssif_inc_stat(ssif_info, send_errors); > @@ -1244,6 +1247,24 @@ static int ssif_remove(struct i2c_client *client) > return 0; > } > > +static int read_response(struct i2c_client *client, unsigned char *resp) > +{ > + int ret = -ENODEV, retry_cnt = SSIF_RECV_RETRIES; > + > + while (retry_cnt > 0) { > + ret = i2c_smbus_read_block_data(client, SSIF_IPMI_RESPONSE, > + resp); > + if (ret > 0) > + break; > + msleep(SSIF_MSG_MSEC); > + retry_cnt--; > + if (retry_cnt <= 0) > + break; > + } > + > + return ret; > +} > + > static int do_cmd(struct i2c_client *client, int len, unsigned char *msg, > int *resp_len, unsigned char *resp) > { > @@ -1260,26 +1281,16 @@ static int do_cmd(struct i2c_client *client, int len, unsigned char *msg, > return -ENODEV; > } > > - ret = -ENODEV; > - retry_cnt = SSIF_RECV_RETRIES; > - while (retry_cnt > 0) { > - ret = i2c_smbus_read_block_data(client, SSIF_IPMI_RESPONSE, > - resp); > - if (ret > 0) > - break; > - msleep(SSIF_MSG_MSEC); > - retry_cnt--; > - if (retry_cnt <= 0) > - break; > - } > - > + ret = read_response(client, resp); > if (ret > 0) { > /* Validate that the response is correct. */ > if (ret < 3 || > (resp[0] != (msg[0] | (1 << 2))) || > (resp[1] != msg[1])) > ret = -EINVAL; > - else { > + else if (ret > IPMI_MAX_MSG_LENGTH) { > + ret = -E2BIG; > + } else { > *resp_len = ret; > ret = 0; > } > @@ -1391,6 +1402,121 @@ static int find_slave_address(struct i2c_client *client, int slave_addr) > return slave_addr; > } > > +static int start_multipart_test(struct i2c_client *client, > + unsigned char *msg, bool do_middle) > +{ > + int retry_cnt = SSIF_SEND_RETRIES, ret; > + > +retry_write: > + ret = i2c_smbus_write_block_data(client, > + SSIF_IPMI_MULTI_PART_REQUEST_START, > + 32, msg); > + if (ret) { > + retry_cnt--; > + if (retry_cnt > 0) > + goto retry_write; > + dev_err(&client->dev, "Could not write multi-part start, though the BMC said it could handle it. Just limit sends to one part.\n"); > + return ret; > + } > + > + if (!do_middle) > + return 0; > + > + ret = i2c_smbus_write_block_data(client, > + SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE, > + 32, msg + 32); > + if (ret) { > + dev_err(&client->dev, "Could not write multi-part middle, though the BMC said it could handle it. Just limit sends to one part.\n"); > + return ret; > + } > + > + return 0; > +} > + > +static void test_multipart_messages(struct i2c_client *client, > + struct ssif_info *ssif_info, > + unsigned char *resp) > +{ > + unsigned char msg[65]; > + int ret; > + bool do_middle; > + > + if (ssif_info->max_xmit_msg_size <= 32) > + return; > + > + do_middle = ssif_info->max_xmit_msg_size > 63; > + > + memset(msg, 0, sizeof(msg)); > + msg[0] = IPMI_NETFN_APP_REQUEST << 2; > + msg[1] = IPMI_GET_DEVICE_ID_CMD; > + > + /* > + * The specification is all messed up dealing with sending > + * multi-part messages. Per what the specification says, it > + * is impossible to send a message that is a multiple of 32 > + * bytes, except for 32 itself. It talks about a "start" > + * transaction (cmd=6) that must be 32 bytes, "middle" > + * transaction (cmd=7) that must be 32 bytes, and an "end" > + * transaction. The "end" transaction is shown as cmd=7 in > + * the text, but if that's the case there is no way to > + * differentiate between a middle and end part except the > + * length being less than 32. But there is a table at the far > + * end of the section (that I had never noticed until someone > + * pointed it out to me) that mentions it as cmd=8. > + * > + * After some thought, I think the example is wrong and the > + * end transaction should be cmd=8. But some systems don't > + * implement cmd=8, they use a zero-length end transaction, > + * even though that violates the SMBus specification. > + * > + * So, to work around this, this code tests if cmd=8 works. > + * If it does, then we use that. If not, it tests zero- > + * byte end transactions. If that works, good. If not, > + * we only allow 63-byte transactions max. > + */ > + > + ret = start_multipart_test(client, msg, do_middle); > + if (ret) > + goto out_no_multi_part; > + > + ret = i2c_smbus_write_block_data(client, > + SSIF_IPMI_MULTI_PART_REQUEST_END, > + 1, msg + 64); > + > + if (!ret) > + ret = read_response(client, resp); > + > + if (ret > 0) { > + /* End transactions work, we are good. */ > + ssif_info->cmd8_works = true; > + return; > + } > + > + ret = start_multipart_test(client, msg, do_middle); > + if (ret) { > + dev_err(&client->dev, "Second multipart test failed.\n"); > + goto out_no_multi_part; > + } > + > + ret = i2c_smbus_write_block_data(client, > + SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE, > + 0, msg + 64); > + if (!ret) > + ret = read_response(client, resp); > + if (ret > 0) > + /* Zero-size end parts work, use those. */ > + return; > + > + /* Limit to 63 bytes and use a short middle command to mark the end. */ > + if (ssif_info->max_xmit_msg_size > 63) > + ssif_info->max_xmit_msg_size = 63; > + return; > + > +out_no_multi_part: > + ssif_info->max_xmit_msg_size = 32; > + return; > +} > + > /* > * Global enables we care about. > */ > @@ -1477,26 +1603,7 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id) > break; > > case SSIF_MULTI_n_PART: > - /* > - * The specification is rather confusing at > - * this point, but I think I understand what > - * is meant. At least I have a workable > - * solution. With multi-part messages, you > - * cannot send a message that is a multiple of > - * 32-bytes in length, because the start and > - * middle messages are 32-bytes and the end > - * message must be at least one byte. You > - * can't fudge on an extra byte, that would > - * screw up things like fru data writes. So > - * we limit the length to 63 bytes. That way > - * a 32-byte message gets sent as a single > - * part. A larger message will be a 32-byte > - * start and the next message is always going > - * to be 1-31 bytes in length. Not ideal, but > - * it should work. > - */ > - if (ssif_info->max_xmit_msg_size > 63) > - ssif_info->max_xmit_msg_size = 63; > + /* We take whatever size given, but do some testing. */ > break; > > default: > @@ -1515,6 +1622,8 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id) > ssif_info->supports_pec = 0; > } > > + test_multipart_messages(client, ssif_info, resp); > + > /* Make sure the NMI timeout is cleared. */ > msg[0] = IPMI_NETFN_APP_REQUEST << 2; > msg[1] = IPMI_CLEAR_MSG_FLAGS_CMD; >
On 10/25/18 17:04, Manoj Iyer wrote: > From: Corey Minyard <cminyard@mvista.com> > > The spec was fairly confusing about how multi-part transmit messages > worked, so the original implementation only added support for two > part messages. But after talking about it with others and finding > something I missed, I think it makes more sense. > > The spec mentions smbus command 8 in a table at the end of the > section on SSIF support as the end transaction. If that works, > then all is good and as it should be. However, some implementations > seem to use a middle transaction <32 bytes tomark the end because of the > confusion in the spec, even though that is an SMBus violation if > the number of bytes is zero. > > So this change adds some tests, if command=8 works, it uses that, > otherwise if an empty end transaction works, it uses a middle > transaction <32 bytes to mark the end. If neither works, then > it limits the size to 63 bytes as it is now. > > BugLink: https://bugs.launchpad.net/bugs/1799794 > > Cc: Harri Hakkarainen <harri@cavium.com> > Cc: Bazhenov, Dmitry <dmitry.bazhenov@auriga.com> > Cc: Mach, Dat <Dat.Mach@cavium.com> > Signed-off-by: Corey Minyard <cminyard@mvista.com> > (cherry picked from commit 10042504ed92c06077b8a20a4edd67ba784847d4 > linux-next) > Signed-off-by: Manoj Iyer <manoj.iyer@canonical.com> As Stefan mentioned, it seem relatively safe wrt regression. Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > --- > drivers/char/ipmi/ipmi_ssif.c | 209 ++++++++++++++++++++++++++-------- > 1 file changed, 159 insertions(+), 50 deletions(-) > > diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c > index 265d6a6583bc..783af69172c4 100644 > --- a/drivers/char/ipmi/ipmi_ssif.c > +++ b/drivers/char/ipmi/ipmi_ssif.c > @@ -60,6 +60,7 @@ > #define SSIF_IPMI_REQUEST 2 > #define SSIF_IPMI_MULTI_PART_REQUEST_START 6 > #define SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE 7 > +#define SSIF_IPMI_MULTI_PART_REQUEST_END 8 > #define SSIF_IPMI_RESPONSE 3 > #define SSIF_IPMI_MULTI_PART_RESPONSE_MIDDLE 9 > > @@ -271,6 +272,7 @@ struct ssif_info { > /* Info from SSIF cmd */ > unsigned char max_xmit_msg_size; > unsigned char max_recv_msg_size; > + bool cmd8_works; /* See test_multipart_messages() for details. */ > unsigned int multi_support; > int supports_pec; > > @@ -887,32 +889,33 @@ static void msg_written_handler(struct ssif_info *ssif_info, int result, > * in the SSIF_MULTI_n_PART case in the probe function > * for details on the intricacies of this. > */ > - int left; > + int left, to_write; > unsigned char *data_to_send; > + unsigned char cmd; > > ssif_inc_stat(ssif_info, sent_messages_parts); > > left = ssif_info->multi_len - ssif_info->multi_pos; > - if (left > 32) > - left = 32; > + to_write = left; > + if (to_write > 32) > + to_write = 32; > /* Length byte. */ > - ssif_info->multi_data[ssif_info->multi_pos] = left; > + ssif_info->multi_data[ssif_info->multi_pos] = to_write; > data_to_send = ssif_info->multi_data + ssif_info->multi_pos; > - ssif_info->multi_pos += left; > - if (left < 32) > - /* > - * Write is finished. Note that we must end > - * with a write of less than 32 bytes to > - * complete the transaction, even if it is > - * zero bytes. > - */ > + ssif_info->multi_pos += to_write; > + cmd = SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE; > + if (ssif_info->cmd8_works) { > + if (left == to_write) { > + cmd = SSIF_IPMI_MULTI_PART_REQUEST_END; > + ssif_info->multi_data = NULL; > + } > + } else if (to_write < 32) { > ssif_info->multi_data = NULL; > + } > > rv = ssif_i2c_send(ssif_info, msg_written_handler, > - I2C_SMBUS_WRITE, > - SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE, > - data_to_send, > - I2C_SMBUS_BLOCK_DATA); > + I2C_SMBUS_WRITE, cmd, > + data_to_send, I2C_SMBUS_BLOCK_DATA); > if (rv < 0) { > /* request failed, just return the error. */ > ssif_inc_stat(ssif_info, send_errors); > @@ -1244,6 +1247,24 @@ static int ssif_remove(struct i2c_client *client) > return 0; > } > > +static int read_response(struct i2c_client *client, unsigned char *resp) > +{ > + int ret = -ENODEV, retry_cnt = SSIF_RECV_RETRIES; > + > + while (retry_cnt > 0) { > + ret = i2c_smbus_read_block_data(client, SSIF_IPMI_RESPONSE, > + resp); > + if (ret > 0) > + break; > + msleep(SSIF_MSG_MSEC); > + retry_cnt--; > + if (retry_cnt <= 0) > + break; > + } > + > + return ret; > +} > + > static int do_cmd(struct i2c_client *client, int len, unsigned char *msg, > int *resp_len, unsigned char *resp) > { > @@ -1260,26 +1281,16 @@ static int do_cmd(struct i2c_client *client, int len, unsigned char *msg, > return -ENODEV; > } > > - ret = -ENODEV; > - retry_cnt = SSIF_RECV_RETRIES; > - while (retry_cnt > 0) { > - ret = i2c_smbus_read_block_data(client, SSIF_IPMI_RESPONSE, > - resp); > - if (ret > 0) > - break; > - msleep(SSIF_MSG_MSEC); > - retry_cnt--; > - if (retry_cnt <= 0) > - break; > - } > - > + ret = read_response(client, resp); > if (ret > 0) { > /* Validate that the response is correct. */ > if (ret < 3 || > (resp[0] != (msg[0] | (1 << 2))) || > (resp[1] != msg[1])) > ret = -EINVAL; > - else { > + else if (ret > IPMI_MAX_MSG_LENGTH) { > + ret = -E2BIG; > + } else { > *resp_len = ret; > ret = 0; > } > @@ -1391,6 +1402,121 @@ static int find_slave_address(struct i2c_client *client, int slave_addr) > return slave_addr; > } > > +static int start_multipart_test(struct i2c_client *client, > + unsigned char *msg, bool do_middle) > +{ > + int retry_cnt = SSIF_SEND_RETRIES, ret; > + > +retry_write: > + ret = i2c_smbus_write_block_data(client, > + SSIF_IPMI_MULTI_PART_REQUEST_START, > + 32, msg); > + if (ret) { > + retry_cnt--; > + if (retry_cnt > 0) > + goto retry_write; > + dev_err(&client->dev, "Could not write multi-part start, though the BMC said it could handle it. Just limit sends to one part.\n"); > + return ret; > + } > + > + if (!do_middle) > + return 0; > + > + ret = i2c_smbus_write_block_data(client, > + SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE, > + 32, msg + 32); > + if (ret) { > + dev_err(&client->dev, "Could not write multi-part middle, though the BMC said it could handle it. Just limit sends to one part.\n"); > + return ret; > + } > + > + return 0; > +} > + > +static void test_multipart_messages(struct i2c_client *client, > + struct ssif_info *ssif_info, > + unsigned char *resp) > +{ > + unsigned char msg[65]; > + int ret; > + bool do_middle; > + > + if (ssif_info->max_xmit_msg_size <= 32) > + return; > + > + do_middle = ssif_info->max_xmit_msg_size > 63; > + > + memset(msg, 0, sizeof(msg)); > + msg[0] = IPMI_NETFN_APP_REQUEST << 2; > + msg[1] = IPMI_GET_DEVICE_ID_CMD; > + > + /* > + * The specification is all messed up dealing with sending > + * multi-part messages. Per what the specification says, it > + * is impossible to send a message that is a multiple of 32 > + * bytes, except for 32 itself. It talks about a "start" > + * transaction (cmd=6) that must be 32 bytes, "middle" > + * transaction (cmd=7) that must be 32 bytes, and an "end" > + * transaction. The "end" transaction is shown as cmd=7 in > + * the text, but if that's the case there is no way to > + * differentiate between a middle and end part except the > + * length being less than 32. But there is a table at the far > + * end of the section (that I had never noticed until someone > + * pointed it out to me) that mentions it as cmd=8. > + * > + * After some thought, I think the example is wrong and the > + * end transaction should be cmd=8. But some systems don't > + * implement cmd=8, they use a zero-length end transaction, > + * even though that violates the SMBus specification. > + * > + * So, to work around this, this code tests if cmd=8 works. > + * If it does, then we use that. If not, it tests zero- > + * byte end transactions. If that works, good. If not, > + * we only allow 63-byte transactions max. > + */ > + > + ret = start_multipart_test(client, msg, do_middle); > + if (ret) > + goto out_no_multi_part; > + > + ret = i2c_smbus_write_block_data(client, > + SSIF_IPMI_MULTI_PART_REQUEST_END, > + 1, msg + 64); > + > + if (!ret) > + ret = read_response(client, resp); > + > + if (ret > 0) { > + /* End transactions work, we are good. */ > + ssif_info->cmd8_works = true; > + return; > + } > + > + ret = start_multipart_test(client, msg, do_middle); > + if (ret) { > + dev_err(&client->dev, "Second multipart test failed.\n"); > + goto out_no_multi_part; > + } > + > + ret = i2c_smbus_write_block_data(client, > + SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE, > + 0, msg + 64); > + if (!ret) > + ret = read_response(client, resp); > + if (ret > 0) > + /* Zero-size end parts work, use those. */ > + return; > + > + /* Limit to 63 bytes and use a short middle command to mark the end. */ > + if (ssif_info->max_xmit_msg_size > 63) > + ssif_info->max_xmit_msg_size = 63; > + return; > + > +out_no_multi_part: > + ssif_info->max_xmit_msg_size = 32; > + return; > +} > + > /* > * Global enables we care about. > */ > @@ -1477,26 +1603,7 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id) > break; > > case SSIF_MULTI_n_PART: > - /* > - * The specification is rather confusing at > - * this point, but I think I understand what > - * is meant. At least I have a workable > - * solution. With multi-part messages, you > - * cannot send a message that is a multiple of > - * 32-bytes in length, because the start and > - * middle messages are 32-bytes and the end > - * message must be at least one byte. You > - * can't fudge on an extra byte, that would > - * screw up things like fru data writes. So > - * we limit the length to 63 bytes. That way > - * a 32-byte message gets sent as a single > - * part. A larger message will be a 32-byte > - * start and the next message is always going > - * to be 1-31 bytes in length. Not ideal, but > - * it should work. > - */ > - if (ssif_info->max_xmit_msg_size > 63) > - ssif_info->max_xmit_msg_size = 63; > + /* We take whatever size given, but do some testing. */ > break; > > default: > @@ -1515,6 +1622,8 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id) > ssif_info->supports_pec = 0; > } > > + test_multipart_messages(client, ssif_info, resp); > + > /* Make sure the NMI timeout is cleared. */ > msg[0] = IPMI_NETFN_APP_REQUEST << 2; > msg[1] = IPMI_CLEAR_MSG_FLAGS_CMD;
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c index 265d6a6583bc..783af69172c4 100644 --- a/drivers/char/ipmi/ipmi_ssif.c +++ b/drivers/char/ipmi/ipmi_ssif.c @@ -60,6 +60,7 @@ #define SSIF_IPMI_REQUEST 2 #define SSIF_IPMI_MULTI_PART_REQUEST_START 6 #define SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE 7 +#define SSIF_IPMI_MULTI_PART_REQUEST_END 8 #define SSIF_IPMI_RESPONSE 3 #define SSIF_IPMI_MULTI_PART_RESPONSE_MIDDLE 9 @@ -271,6 +272,7 @@ struct ssif_info { /* Info from SSIF cmd */ unsigned char max_xmit_msg_size; unsigned char max_recv_msg_size; + bool cmd8_works; /* See test_multipart_messages() for details. */ unsigned int multi_support; int supports_pec; @@ -887,32 +889,33 @@ static void msg_written_handler(struct ssif_info *ssif_info, int result, * in the SSIF_MULTI_n_PART case in the probe function * for details on the intricacies of this. */ - int left; + int left, to_write; unsigned char *data_to_send; + unsigned char cmd; ssif_inc_stat(ssif_info, sent_messages_parts); left = ssif_info->multi_len - ssif_info->multi_pos; - if (left > 32) - left = 32; + to_write = left; + if (to_write > 32) + to_write = 32; /* Length byte. */ - ssif_info->multi_data[ssif_info->multi_pos] = left; + ssif_info->multi_data[ssif_info->multi_pos] = to_write; data_to_send = ssif_info->multi_data + ssif_info->multi_pos; - ssif_info->multi_pos += left; - if (left < 32) - /* - * Write is finished. Note that we must end - * with a write of less than 32 bytes to - * complete the transaction, even if it is - * zero bytes. - */ + ssif_info->multi_pos += to_write; + cmd = SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE; + if (ssif_info->cmd8_works) { + if (left == to_write) { + cmd = SSIF_IPMI_MULTI_PART_REQUEST_END; + ssif_info->multi_data = NULL; + } + } else if (to_write < 32) { ssif_info->multi_data = NULL; + } rv = ssif_i2c_send(ssif_info, msg_written_handler, - I2C_SMBUS_WRITE, - SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE, - data_to_send, - I2C_SMBUS_BLOCK_DATA); + I2C_SMBUS_WRITE, cmd, + data_to_send, I2C_SMBUS_BLOCK_DATA); if (rv < 0) { /* request failed, just return the error. */ ssif_inc_stat(ssif_info, send_errors); @@ -1244,6 +1247,24 @@ static int ssif_remove(struct i2c_client *client) return 0; } +static int read_response(struct i2c_client *client, unsigned char *resp) +{ + int ret = -ENODEV, retry_cnt = SSIF_RECV_RETRIES; + + while (retry_cnt > 0) { + ret = i2c_smbus_read_block_data(client, SSIF_IPMI_RESPONSE, + resp); + if (ret > 0) + break; + msleep(SSIF_MSG_MSEC); + retry_cnt--; + if (retry_cnt <= 0) + break; + } + + return ret; +} + static int do_cmd(struct i2c_client *client, int len, unsigned char *msg, int *resp_len, unsigned char *resp) { @@ -1260,26 +1281,16 @@ static int do_cmd(struct i2c_client *client, int len, unsigned char *msg, return -ENODEV; } - ret = -ENODEV; - retry_cnt = SSIF_RECV_RETRIES; - while (retry_cnt > 0) { - ret = i2c_smbus_read_block_data(client, SSIF_IPMI_RESPONSE, - resp); - if (ret > 0) - break; - msleep(SSIF_MSG_MSEC); - retry_cnt--; - if (retry_cnt <= 0) - break; - } - + ret = read_response(client, resp); if (ret > 0) { /* Validate that the response is correct. */ if (ret < 3 || (resp[0] != (msg[0] | (1 << 2))) || (resp[1] != msg[1])) ret = -EINVAL; - else { + else if (ret > IPMI_MAX_MSG_LENGTH) { + ret = -E2BIG; + } else { *resp_len = ret; ret = 0; } @@ -1391,6 +1402,121 @@ static int find_slave_address(struct i2c_client *client, int slave_addr) return slave_addr; } +static int start_multipart_test(struct i2c_client *client, + unsigned char *msg, bool do_middle) +{ + int retry_cnt = SSIF_SEND_RETRIES, ret; + +retry_write: + ret = i2c_smbus_write_block_data(client, + SSIF_IPMI_MULTI_PART_REQUEST_START, + 32, msg); + if (ret) { + retry_cnt--; + if (retry_cnt > 0) + goto retry_write; + dev_err(&client->dev, "Could not write multi-part start, though the BMC said it could handle it. Just limit sends to one part.\n"); + return ret; + } + + if (!do_middle) + return 0; + + ret = i2c_smbus_write_block_data(client, + SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE, + 32, msg + 32); + if (ret) { + dev_err(&client->dev, "Could not write multi-part middle, though the BMC said it could handle it. Just limit sends to one part.\n"); + return ret; + } + + return 0; +} + +static void test_multipart_messages(struct i2c_client *client, + struct ssif_info *ssif_info, + unsigned char *resp) +{ + unsigned char msg[65]; + int ret; + bool do_middle; + + if (ssif_info->max_xmit_msg_size <= 32) + return; + + do_middle = ssif_info->max_xmit_msg_size > 63; + + memset(msg, 0, sizeof(msg)); + msg[0] = IPMI_NETFN_APP_REQUEST << 2; + msg[1] = IPMI_GET_DEVICE_ID_CMD; + + /* + * The specification is all messed up dealing with sending + * multi-part messages. Per what the specification says, it + * is impossible to send a message that is a multiple of 32 + * bytes, except for 32 itself. It talks about a "start" + * transaction (cmd=6) that must be 32 bytes, "middle" + * transaction (cmd=7) that must be 32 bytes, and an "end" + * transaction. The "end" transaction is shown as cmd=7 in + * the text, but if that's the case there is no way to + * differentiate between a middle and end part except the + * length being less than 32. But there is a table at the far + * end of the section (that I had never noticed until someone + * pointed it out to me) that mentions it as cmd=8. + * + * After some thought, I think the example is wrong and the + * end transaction should be cmd=8. But some systems don't + * implement cmd=8, they use a zero-length end transaction, + * even though that violates the SMBus specification. + * + * So, to work around this, this code tests if cmd=8 works. + * If it does, then we use that. If not, it tests zero- + * byte end transactions. If that works, good. If not, + * we only allow 63-byte transactions max. + */ + + ret = start_multipart_test(client, msg, do_middle); + if (ret) + goto out_no_multi_part; + + ret = i2c_smbus_write_block_data(client, + SSIF_IPMI_MULTI_PART_REQUEST_END, + 1, msg + 64); + + if (!ret) + ret = read_response(client, resp); + + if (ret > 0) { + /* End transactions work, we are good. */ + ssif_info->cmd8_works = true; + return; + } + + ret = start_multipart_test(client, msg, do_middle); + if (ret) { + dev_err(&client->dev, "Second multipart test failed.\n"); + goto out_no_multi_part; + } + + ret = i2c_smbus_write_block_data(client, + SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE, + 0, msg + 64); + if (!ret) + ret = read_response(client, resp); + if (ret > 0) + /* Zero-size end parts work, use those. */ + return; + + /* Limit to 63 bytes and use a short middle command to mark the end. */ + if (ssif_info->max_xmit_msg_size > 63) + ssif_info->max_xmit_msg_size = 63; + return; + +out_no_multi_part: + ssif_info->max_xmit_msg_size = 32; + return; +} + /* * Global enables we care about. */ @@ -1477,26 +1603,7 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id) break; case SSIF_MULTI_n_PART: - /* - * The specification is rather confusing at - * this point, but I think I understand what - * is meant. At least I have a workable - * solution. With multi-part messages, you - * cannot send a message that is a multiple of - * 32-bytes in length, because the start and - * middle messages are 32-bytes and the end - * message must be at least one byte. You - * can't fudge on an extra byte, that would - * screw up things like fru data writes. So - * we limit the length to 63 bytes. That way - * a 32-byte message gets sent as a single - * part. A larger message will be a 32-byte - * start and the next message is always going - * to be 1-31 bytes in length. Not ideal, but - * it should work. - */ - if (ssif_info->max_xmit_msg_size > 63) - ssif_info->max_xmit_msg_size = 63; + /* We take whatever size given, but do some testing. */ break; default: @@ -1515,6 +1622,8 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id) ssif_info->supports_pec = 0; } + test_multipart_messages(client, ssif_info, resp); + /* Make sure the NMI timeout is cleared. */ msg[0] = IPMI_NETFN_APP_REQUEST << 2; msg[1] = IPMI_CLEAR_MSG_FLAGS_CMD;