Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751915AbdC1MSq (ORCPT ); Tue, 28 Mar 2017 08:18:46 -0400 Received: from mail-oi0-f68.google.com ([209.85.218.68]:34774 "EHLO mail-oi0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751496AbdC1MSo (ORCPT ); Tue, 28 Mar 2017 08:18:44 -0400 Reply-To: minyard@acm.org Subject: Re: [Openipmi-developer] [PATCH v2] ipmi: Fix kernel panic at ipmi_ssif_thread() References: <1490599902-45215-1-git-send-email-joechang@qti.qualcomm.com> To: joechang@codeaurora.org Cc: anjiandi@codeaurora.org, openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org, Corey Minyard From: Corey Minyard Message-ID: <5cd886a8-5fa8-2828-ce48-c7d507b6b571@acm.org> Date: Tue, 28 Mar 2017 07:18:40 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4339 Lines: 122 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 >>> >>> 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 >>> --- >>> 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