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

Groups > linux.kernel > #1435638 > unrolled thread

[PATCH 1/3] ACPI: fix incorrect counts returned by acpi_parse_entries_array()

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

  [PATCH 1/3] ACPI: fix incorrect counts returned by acpi_parse_entries_array() Al Stone <ahs3@redhat.com> - 2016-07-01 23:30 +0200
    Re: [PATCH 1/3] ACPI: fix incorrect counts returned by acpi_parse_entries_array() "Rafael J. Wysocki" <rafael@kernel.org> - 2016-07-01 23:30 +0200
      Re: [PATCH 1/3] ACPI: fix incorrect counts returned by  acpi_parse_entries_array() Al Stone <ahs3@redhat.com> - 2016-07-01 23:40 +0200
        Re: [PATCH 1/3] ACPI: fix incorrect counts returned by acpi_parse_entries_array() "Rafael J. Wysocki" <rafael@kernel.org> - 2016-07-01 23:50 +0200
          Re: [PATCH 1/3] ACPI: fix incorrect counts returned by  acpi_parse_entries_array() Al Stone <ahs3@redhat.com> - 2016-07-02 00:00 +0200
            Re: [PATCH 1/3] ACPI: fix incorrect counts returned by acpi_parse_entries_array() "Rafael J. Wysocki" <rafael@kernel.org> - 2016-07-02 00:10 +0200
              Re: [PATCH 1/3] ACPI: fix incorrect counts returned by  acpi_parse_entries_array() Al Stone <ahs3@redhat.com> - 2016-07-02 00:40 +0200

#1435638 — [PATCH 1/3] ACPI: fix incorrect counts returned by acpi_parse_entries_array()

The static function acpi_parse_entries_array() is provided an array of
type struct acpi_subtable_proc that has a callback function and a count.
The count should reflect how many times the callback has been successfully
called.  However, the current code only increments the 0th element of the
array, regardless of the number of entries in the array, or which callback
has been invoked.  The fix is to use the index into the array, instead of
a pointer to the beginning of the array.

Signed-off-by: Al Stone <ahs3@redhat.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
---
 drivers/acpi/tables.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 9f0ad6e..3e167b4 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -281,7 +281,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
 			     proc[i].handler(entry, table_end))
 				return -EINVAL;
 
-			proc->count++;
+			proc[i].count++;
 			break;
 		}
 		if (i != proc_num)
@@ -416,7 +416,7 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler)
 		return -ENODEV;
 }
 
-/* 
+/*
  * The BIOS is supposed to supply a single APIC/MADT,
  * but some report two.  Provide a knob to use either.
  * (don't you wish instance 0 and 1 were not the same?)
-- 
2.7.4

[toc] | [next] | [standalone]

#1435641

On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
> The static function acpi_parse_entries_array() is provided an array of
> type struct acpi_subtable_proc that has a callback function and a count.
> The count should reflect how many times the callback has been successfully
> called.  However, the current code only increments the 0th element of the
> array, regardless of the number of entries in the array, or which callback
> has been invoked.  The fix is to use the index into the array, instead of
> a pointer to the beginning of the array.

OK, so it would be good to say what the consequences of the problem are too.

> Signed-off-by: Al Stone <ahs3@redhat.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> ---
>  drivers/acpi/tables.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 9f0ad6e..3e167b4 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -281,7 +281,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
>                              proc[i].handler(entry, table_end))
>                                 return -EINVAL;
>
> -                       proc->count++;
> +                       proc[i].count++;
>                         break;
>                 }
>                 if (i != proc_num)
> @@ -416,7 +416,7 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler)
>                 return -ENODEV;
>  }
>
> -/*
> +/*
>   * The BIOS is supposed to supply a single APIC/MADT,
>   * but some report two.  Provide a knob to use either.
>   * (don't you wish instance 0 and 1 were not the same?)
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

#1435643 — Re: [PATCH 1/3] ACPI: fix incorrect counts returned by acpi_parse_entries_array()

On 07/01/2016 03:25 PM, Rafael J. Wysocki wrote:
> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>> The static function acpi_parse_entries_array() is provided an array of
>> type struct acpi_subtable_proc that has a callback function and a count.
>> The count should reflect how many times the callback has been successfully
>> called.  However, the current code only increments the 0th element of the
>> array, regardless of the number of entries in the array, or which callback
>> has been invoked.  The fix is to use the index into the array, instead of
>> a pointer to the beginning of the array.
> 
> OK, so it would be good to say what the consequences of the problem are too.
> 

Hrm.  So replace the last sentence with something like:

   The fix is to use the index into the array, instead of
   a pointer to the beginning of the array, so that the count
   for each element in the array in incremented by the
   corresponding callback.

That feels a little clunky but is it closer to what you were
thinking?

Thanks for the review!

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

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

#1435657

On Fri, Jul 1, 2016 at 11:36 PM, Al Stone <ahs3@redhat.com> wrote:
> On 07/01/2016 03:25 PM, Rafael J. Wysocki wrote:
>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>> The static function acpi_parse_entries_array() is provided an array of
>>> type struct acpi_subtable_proc that has a callback function and a count.
>>> The count should reflect how many times the callback has been successfully
>>> called.  However, the current code only increments the 0th element of the
>>> array, regardless of the number of entries in the array, or which callback
>>> has been invoked.  The fix is to use the index into the array, instead of
>>> a pointer to the beginning of the array.
>>
>> OK, so it would be good to say what the consequences of the problem are too.
>>
>
> Hrm.  So replace the last sentence with something like:
>
>    The fix is to use the index into the array, instead of
>    a pointer to the beginning of the array, so that the count
>    for each element in the array in incremented by the
>    corresponding callback.
>
> That feels a little clunky but is it closer to what you were
> thinking?

Well, not really.

The code is arguably incorrect, but is there anything that does not
work as expected as a result?  Any functional breakage?  Any
misleading messages printed?

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

#1435660 — Re: [PATCH 1/3] ACPI: fix incorrect counts returned by acpi_parse_entries_array()

On 07/01/2016 03:44 PM, Rafael J. Wysocki wrote:
> On Fri, Jul 1, 2016 at 11:36 PM, Al Stone <ahs3@redhat.com> wrote:
>> On 07/01/2016 03:25 PM, Rafael J. Wysocki wrote:
>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>> The static function acpi_parse_entries_array() is provided an array of
>>>> type struct acpi_subtable_proc that has a callback function and a count.
>>>> The count should reflect how many times the callback has been successfully
>>>> called.  However, the current code only increments the 0th element of the
>>>> array, regardless of the number of entries in the array, or which callback
>>>> has been invoked.  The fix is to use the index into the array, instead of
>>>> a pointer to the beginning of the array.
>>>
>>> OK, so it would be good to say what the consequences of the problem are too.
>>>
>>
>> Hrm.  So replace the last sentence with something like:
>>
>>    The fix is to use the index into the array, instead of
>>    a pointer to the beginning of the array, so that the count
>>    for each element in the array in incremented by the
>>    corresponding callback.
>>
>> That feels a little clunky but is it closer to what you were
>> thinking?
> 
> Well, not really.
> 
> The code is arguably incorrect, but is there anything that does not
> work as expected as a result?  Any functional breakage?  Any
> misleading messages printed?
> 

That's the odd thing; there is no breakage.  Of any sort.

But, no one relies on those values for anything at this point.  I've got a
couple of ideas I'm working on that are easier if it does work right, however.


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

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

#1435665

On Fri, Jul 1, 2016 at 11:50 PM, Al Stone <ahs3@redhat.com> wrote:
> On 07/01/2016 03:44 PM, Rafael J. Wysocki wrote:
>> On Fri, Jul 1, 2016 at 11:36 PM, Al Stone <ahs3@redhat.com> wrote:
>>> On 07/01/2016 03:25 PM, Rafael J. Wysocki wrote:
>>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>>> The static function acpi_parse_entries_array() is provided an array of
>>>>> type struct acpi_subtable_proc that has a callback function and a count.
>>>>> The count should reflect how many times the callback has been successfully
>>>>> called.  However, the current code only increments the 0th element of the
>>>>> array, regardless of the number of entries in the array, or which callback
>>>>> has been invoked.  The fix is to use the index into the array, instead of
>>>>> a pointer to the beginning of the array.
>>>>
>>>> OK, so it would be good to say what the consequences of the problem are too.
>>>>
>>>
>>> Hrm.  So replace the last sentence with something like:
>>>
>>>    The fix is to use the index into the array, instead of
>>>    a pointer to the beginning of the array, so that the count
>>>    for each element in the array in incremented by the
>>>    corresponding callback.
>>>
>>> That feels a little clunky but is it closer to what you were
>>> thinking?
>>
>> Well, not really.
>>
>> The code is arguably incorrect, but is there anything that does not
>> work as expected as a result?  Any functional breakage?  Any
>> misleading messages printed?
>>
>
> That's the odd thing; there is no breakage.  Of any sort.
>
> But, no one relies on those values for anything at this point.  I've got a
> couple of ideas I'm working on that are easier if it does work right, however.

That's information that should go into the changelog too.

"There are no functional consequences of the issue, but fixing it is
necessary for future work."

Or similar.

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

#1435671 — Re: [PATCH 1/3] ACPI: fix incorrect counts returned by acpi_parse_entries_array()

On 07/01/2016 03:56 PM, Rafael J. Wysocki wrote:
> On Fri, Jul 1, 2016 at 11:50 PM, Al Stone <ahs3@redhat.com> wrote:
>> On 07/01/2016 03:44 PM, Rafael J. Wysocki wrote:
>>> On Fri, Jul 1, 2016 at 11:36 PM, Al Stone <ahs3@redhat.com> wrote:
>>>> On 07/01/2016 03:25 PM, Rafael J. Wysocki wrote:
>>>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>>>> The static function acpi_parse_entries_array() is provided an array of
>>>>>> type struct acpi_subtable_proc that has a callback function and a count.
>>>>>> The count should reflect how many times the callback has been successfully
>>>>>> called.  However, the current code only increments the 0th element of the
>>>>>> array, regardless of the number of entries in the array, or which callback
>>>>>> has been invoked.  The fix is to use the index into the array, instead of
>>>>>> a pointer to the beginning of the array.
>>>>>
>>>>> OK, so it would be good to say what the consequences of the problem are too.
>>>>>
>>>>
>>>> Hrm.  So replace the last sentence with something like:
>>>>
>>>>    The fix is to use the index into the array, instead of
>>>>    a pointer to the beginning of the array, so that the count
>>>>    for each element in the array in incremented by the
>>>>    corresponding callback.
>>>>
>>>> That feels a little clunky but is it closer to what you were
>>>> thinking?
>>>
>>> Well, not really.
>>>
>>> The code is arguably incorrect, but is there anything that does not
>>> work as expected as a result?  Any functional breakage?  Any
>>> misleading messages printed?
>>>
>>
>> That's the odd thing; there is no breakage.  Of any sort.
>>
>> But, no one relies on those values for anything at this point.  I've got a
>> couple of ideas I'm working on that are easier if it does work right, however.
> 
> That's information that should go into the changelog too.
> 
> "There are no functional consequences of the issue, but fixing it is
> necessary for future work."
> 
> Or similar.
> 

Will do in v2.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web