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

[joechang@codeaurora.org]

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;