lkml.org 
[lkml]   [2017]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [Openipmi-developer] [PATCH v2] ipmi: Fix kernel panic at ipmi_ssif_thread()
From
Date
On 03/27/2017 09:29 PM, joechang@codeaurora.org wrote:
> Hi Corey,
>
> I really appreciate your patience and detail code review.

Not a problem at all, thanks for sticking with this.

This will be queued for the next release, and I'll request backports to
stable trees.

Thanks,

-corey

> I re-generate new patch and send new patch to you.
> Please feel free to feedback to me if something need to modify from the
> new patch..
>
> Thank you,
> Joseph.
>
> Corey Minyard 於 2017-03-27 20:32 寫到:
>> 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;
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Openipmi-developer mailing list
> Openipmi-developer@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openipmi-developer


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