From: Joeseph Chang <[email protected]>
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 <[email protected]>
---
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;
/* 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;
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;
--
1.9.1
On 03/27/2017 02:31 AM, Joeseph Chang wrote:
> From: Joeseph Chang <[email protected]>
>
> 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 <[email protected]>
> ---
> 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;
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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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;
On 03/27/2017 09:29 PM, [email protected] 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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/openipmi-developer
On Tue, Mar 28, 2017 at 7:18 AM, Corey Minyard <[email protected]> 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.
On 04/07/2017 11:17 AM, Timur Tabi wrote:
> On Tue, Mar 28, 2017 at 7:18 AM, Corey Minyard <[email protected]> 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