| Message ID | 1467408081-7418-2-git-send-email-ahs3@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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 -- 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
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!
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? -- 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
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.
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. -- 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
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.
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?)
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(-)