lkml.org 
[lkml]   [2017]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v2] ipmi: Fix kernel panic at ipmi_ssif_thread()
From
Date
On 03/27/2017 02:31 AM, Joeseph Chang wrote:
> From: Joeseph Chang <joechang@codeaurora.org>
>
> Since patch v1 touch ssif_info->multi_pos and ssif_info->multi_data
> after ssif_i2c_send(). msg_written_handler can be called at any time
> after ssif_i2c_send(). There is possible to have concurrent access to
> ssif_info->multi_pos or ssif_info->multi_data at msg_written_handler.
>
> Revert patch v1 and add new local pointer "i2c_data" which point to
> i2c data and size that will be sent out to avoid concurrent access in
> msg_written_handler().
>
> Signed-off-by: Joeseph Chang <joechang@codeaurora.org>
> ---
> drivers/char/ipmi/ipmi_ssif.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index 39346ee..f36f018 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -852,6 +852,7 @@ static void msg_written_handler(struct ssif_info *ssif_info, int result,
> unsigned char *data, unsigned int len)
> {
> int rv;
> + unsigned char *i2c_data;

The variable should be in the block where it is used. The basic rule is
to limit scope as much as possible.

The name is also a little vague, though not terrible.

>
> /* We are single-threaded here, so no need for a lock. */
> if (result < 0) {
> @@ -899,13 +900,22 @@ static void msg_written_handler(struct ssif_info *ssif_info, int result,
> left = 32;
> /* Length byte. */
> ssif_info->multi_data[ssif_info->multi_pos] = left;
> + i2c_data = 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_data = NULL;

Can you make this change not relative to the previous change? Just do a
"git rebase -i ...." and squash the
changes.

I saw your previous emails, but I can't take those because of the
comments at the top and the text from
the reply. You can add comments to a submission that don't go into git,
just put them after the '---" in
the header.

Thanks,

-corey

>
> rv = ssif_i2c_send(ssif_info, msg_written_handler,
> I2C_SMBUS_WRITE,
> SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE,
> - ssif_info->multi_data + ssif_info->multi_pos,
> + i2c_data,
> I2C_SMBUS_BLOCK_DATA);
> -
> if (rv < 0) {
> /* request failed, just return the error. */
> ssif_inc_stat(ssif_info, send_errors);
> @@ -914,16 +924,6 @@ static void msg_written_handler(struct ssif_info *ssif_info, int result,
> pr_info("Error from i2c_non_blocking_op(3)\n");
> msg_done_handler(ssif_info, -EIO, NULL, 0);
> }
> -
> - 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_data = NULL;
> } else {
> /* Ready to request the result. */
> unsigned long oflags, *flags;


\
 
 \ /
  Last update: 2017-03-27 14:34    [W:0.242 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site