Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1610268 > unrolled thread

Re: [PATCH v2] ipmi: Fix kernel panic at ipmi_ssif_thread()

Started byjoechang@codeaurora.org
First post2017-03-28 04:40 +0200
Last post2017-04-07 19:30 +0200
Articles 4 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] ipmi: Fix kernel panic at ipmi_ssif_thread() joechang@codeaurora.org - 2017-03-28 04:40 +0200
    Re: [Openipmi-developer] [PATCH v2] ipmi: Fix kernel panic at  ipmi_ssif_thread() Corey Minyard <minyard@acm.org> - 2017-03-28 14:20 +0200
      Re: [Openipmi-developer] [PATCH v2] ipmi: Fix kernel panic at ipmi_ssif_thread() Timur Tabi <timur@codeaurora.org> - 2017-04-07 18:20 +0200
        Re: [Openipmi-developer] [PATCH v2] ipmi: Fix kernel panic at  ipmi_ssif_thread() Corey Minyard <minyard@acm.org> - 2017-04-07 19:30 +0200

#1610268 — Re: [PATCH v2] ipmi: Fix kernel panic at ipmi_ssif_thread()

Fromjoechang@codeaurora.org
Date2017-03-28 04:40 +0200
SubjectRe: [PATCH v2] ipmi: Fix kernel panic at ipmi_ssif_thread()
Message-ID<tpPd8-VD-13@gated-at.bofh.it>
Hi Corey,

I really appreciate your patience and detail code review.
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;

[toc] | [next] | [standalone]

#1610630 — Re: [Openipmi-developer] [PATCH v2] ipmi: Fix kernel panic at ipmi_ssif_thread()

FromCorey Minyard <minyard@acm.org>
Date2017-03-28 14:20 +0200
SubjectRe: [Openipmi-developer] [PATCH v2] ipmi: Fix kernel panic at ipmi_ssif_thread()
Message-ID<tpYgp-7p2-5@gated-at.bofh.it>
In reply to#1610268
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

[toc] | [prev] | [next] | [standalone]

#1618938 — Re: [Openipmi-developer] [PATCH v2] ipmi: Fix kernel panic at ipmi_ssif_thread()

FromTimur Tabi <timur@codeaurora.org>
Date2017-04-07 18:20 +0200
SubjectRe: [Openipmi-developer] [PATCH v2] ipmi: Fix kernel panic at ipmi_ssif_thread()
Message-ID<ttEMa-18X-9@gated-at.bofh.it>
In reply to#1610630
On Tue, Mar 28, 2017 at 7:18 AM, Corey Minyard <minyard@acm.org> wrote:
>
> 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.

So I'm a little confused with the status of this patch.  Is this the
final commit that will go into 4.12?

https://github.com/cminyard/linux-ipmi/commit/495658a9888dc213fafeed1284050a0b347d90fd

I don't see a patch description.  What happened to it?

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1618986 — Re: [Openipmi-developer] [PATCH v2] ipmi: Fix kernel panic at ipmi_ssif_thread()

FromCorey Minyard <minyard@acm.org>
Date2017-04-07 19:30 +0200
SubjectRe: [Openipmi-developer] [PATCH v2] ipmi: Fix kernel panic at ipmi_ssif_thread()
Message-ID<ttFRU-1SP-3@gated-at.bofh.it>
In reply to#1618938
On 04/07/2017 11:17 AM, Timur Tabi wrote:
> On Tue, Mar 28, 2017 at 7:18 AM, Corey Minyard <minyard@acm.org> wrote:
>> 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.
> So I'm a little confused with the status of this patch.  Is this the
> final commit that will go into 4.12?
>
> https://github.com/cminyard/linux-ipmi/commit/495658a9888dc213fafeed1284050a0b347d90fd
>
> I don't see a patch description.  What happened to it?
>
That's strange.  I just did a normal git am.  I'll fix it.

Thanks,

-corey

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web