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

Groups > linux.kernel > #1435634 > unrolled thread

[PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables

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 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables Al Stone <ahs3@redhat.com> - 2016-07-01 23:30 +0200
    Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses  all subtables "Rafael J. Wysocki" <rafael@kernel.org> - 2016-07-01 23:40 +0200
      Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses  all subtables Al Stone <ahs3@redhat.com> - 2016-07-01 23:50 +0200
        Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses  all subtables "Rafael J. Wysocki" <rafael@kernel.org> - 2016-07-01 23:50 +0200
          Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses  all subtables Al Stone <ahs3@redhat.com> - 2016-07-02 00:00 +0200
            Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses  all subtables "Rafael J. Wysocki" <rafael@kernel.org> - 2016-07-02 00:10 +0200
              Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses  all subtables Al Stone <ahs3@redhat.com> - 2016-07-02 01:10 +0200
                Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses  all subtables "Rafael J. Wysocki" <rafael@kernel.org> - 2016-07-02 01:30 +0200

#1435634 — [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables

Without this patch, the acpi_parse_entries_array() function will return
the very first time there is any error found in either the array of
callback functions or if one of the callbacks returns an non-zero value.
However, the array of callbacks could still have valid entries further
on in the array, or the callbacks may be able to process subsequent
subtables without error.  The change here makes the function consistent
with its description so that it will properly return the sum of all
matching entries for all proc handlers, instead of stopping abruptly
as it does today.

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 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 3e167b4..76c07ed 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -246,6 +246,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
 	struct acpi_subtable_header *entry;
 	unsigned long table_end;
 	int count = 0;
+	int errs_found = 0;
 	int i;
 
 	if (acpi_disabled)
@@ -278,8 +279,10 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
 			if (entry->type != proc[i].id)
 				continue;
 			if (!proc[i].handler ||
-			     proc[i].handler(entry, table_end))
-				return -EINVAL;
+			     proc[i].handler(entry, table_end)) {
+				errs_found++;
+				continue;
+			}
 
 			proc[i].count++;
 			break;
@@ -305,7 +308,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
 			id, proc->id, count - max_entries, count);
 	}
 
-	return count;
+	return (errs_found) ? -EINVAL : count;
 }
 
 int __init
-- 
2.7.4

[toc] | [next] | [standalone]

#1435647 — Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables

On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
> Without this patch, the acpi_parse_entries_array() function will return
> the very first time there is any error found in either the array of
> callback functions or if one of the callbacks returns an non-zero value.
> However, the array of callbacks could still have valid entries further
> on in the array, or the callbacks may be able to process subsequent
> subtables without error.  The change here makes the function consistent
> with its description so that it will properly return the sum of all
> matching entries for all proc handlers, instead of stopping abruptly
> as it does today.

I'm not sure I follow.

You seem to be saying that the function should process all of the
subtables etc even though errors have been found for some of them, but
it still will return an error in the end if there are any errors.  How
exactly does it help to continue processing in case of an error, then?

> 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 | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 3e167b4..76c07ed 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -246,6 +246,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
>         struct acpi_subtable_header *entry;
>         unsigned long table_end;
>         int count = 0;
> +       int errs_found = 0;
>         int i;
>
>         if (acpi_disabled)
> @@ -278,8 +279,10 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
>                         if (entry->type != proc[i].id)
>                                 continue;
>                         if (!proc[i].handler ||
> -                            proc[i].handler(entry, table_end))
> -                               return -EINVAL;
> +                            proc[i].handler(entry, table_end)) {
> +                               errs_found++;
> +                               continue;
> +                       }
>
>                         proc[i].count++;
>                         break;
> @@ -305,7 +308,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
>                         id, proc->id, count - max_entries, count);
>         }
>
> -       return count;
> +       return (errs_found) ? -EINVAL : count;
>  }
>
>  int __init
> --

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

#1435649 — Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables

On 07/01/2016 03:32 PM, Rafael J. Wysocki wrote:
> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>> Without this patch, the acpi_parse_entries_array() function will return
>> the very first time there is any error found in either the array of
>> callback functions or if one of the callbacks returns an non-zero value.
>> However, the array of callbacks could still have valid entries further
>> on in the array, or the callbacks may be able to process subsequent
>> subtables without error.  The change here makes the function consistent
>> with its description so that it will properly return the sum of all
>> matching entries for all proc handlers, instead of stopping abruptly
>> as it does today.
> 
> I'm not sure I follow.
> 
> You seem to be saying that the function should process all of the
> subtables etc even though errors have been found for some of them, but
> it still will return an error in the end if there are any errors.  How
> exactly does it help to continue processing in case of an error, then?

The use case I have in mind is to simply count all of the subtables of
a certain type.  If for some reason, the callback -- or any other callback
-- fails, the traversal of all the subtables stops immediately.  So, I
could have two callbacks, and if the first one fails on the first subtable
of its type, traversal stops.  The count for the second callback will be
zero which may or may not be correct.

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

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

#1435653 — Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables

On Fri, Jul 1, 2016 at 11:41 PM, Al Stone <ahs3@redhat.com> wrote:
> On 07/01/2016 03:32 PM, Rafael J. Wysocki wrote:
>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>> Without this patch, the acpi_parse_entries_array() function will return
>>> the very first time there is any error found in either the array of
>>> callback functions or if one of the callbacks returns an non-zero value.
>>> However, the array of callbacks could still have valid entries further
>>> on in the array, or the callbacks may be able to process subsequent
>>> subtables without error.  The change here makes the function consistent
>>> with its description so that it will properly return the sum of all
>>> matching entries for all proc handlers, instead of stopping abruptly
>>> as it does today.
>>
>> I'm not sure I follow.
>>
>> You seem to be saying that the function should process all of the
>> subtables etc even though errors have been found for some of them, but
>> it still will return an error in the end if there are any errors.  How
>> exactly does it help to continue processing in case of an error, then?
>
> The use case I have in mind is to simply count all of the subtables of
> a certain type.  If for some reason, the callback -- or any other callback
> -- fails, the traversal of all the subtables stops immediately.  So, I
> could have two callbacks, and if the first one fails on the first subtable
> of its type, traversal stops.  The count for the second callback will be
> zero which may or may not be correct.

It will be zero, because the callback has not been invoked at all.
Why is this incorrect?

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

#1435658 — Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables

On 07/01/2016 03:46 PM, Rafael J. Wysocki wrote:
> On Fri, Jul 1, 2016 at 11:41 PM, Al Stone <ahs3@redhat.com> wrote:
>> On 07/01/2016 03:32 PM, Rafael J. Wysocki wrote:
>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>> Without this patch, the acpi_parse_entries_array() function will return
>>>> the very first time there is any error found in either the array of
>>>> callback functions or if one of the callbacks returns an non-zero value.
>>>> However, the array of callbacks could still have valid entries further
>>>> on in the array, or the callbacks may be able to process subsequent
>>>> subtables without error.  The change here makes the function consistent
>>>> with its description so that it will properly return the sum of all
>>>> matching entries for all proc handlers, instead of stopping abruptly
>>>> as it does today.
>>>
>>> I'm not sure I follow.
>>>
>>> You seem to be saying that the function should process all of the
>>> subtables etc even though errors have been found for some of them, but
>>> it still will return an error in the end if there are any errors.  How
>>> exactly does it help to continue processing in case of an error, then?
>>
>> The use case I have in mind is to simply count all of the subtables of
>> a certain type.  If for some reason, the callback -- or any other callback
>> -- fails, the traversal of all the subtables stops immediately.  So, I
>> could have two callbacks, and if the first one fails on the first subtable
>> of its type, traversal stops.  The count for the second callback will be
>> zero which may or may not be correct.
> 
> It will be zero, because the callback has not been invoked at all.
> Why is this incorrect?
> 

Because there could be additional subtables after the one causing a failure
that the second callback could have counted; e.g., if the failure is on the
first subtable of 20 in the MADT, the following 19 would be ignored, even if
they were all the right subtype for the second callback.

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

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

#1435666 — Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables

On Fri, Jul 1, 2016 at 11:55 PM, Al Stone <ahs3@redhat.com> wrote:
> On 07/01/2016 03:46 PM, Rafael J. Wysocki wrote:
>> On Fri, Jul 1, 2016 at 11:41 PM, Al Stone <ahs3@redhat.com> wrote:
>>> On 07/01/2016 03:32 PM, Rafael J. Wysocki wrote:
>>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>>> Without this patch, the acpi_parse_entries_array() function will return
>>>>> the very first time there is any error found in either the array of
>>>>> callback functions or if one of the callbacks returns an non-zero value.
>>>>> However, the array of callbacks could still have valid entries further
>>>>> on in the array, or the callbacks may be able to process subsequent
>>>>> subtables without error.  The change here makes the function consistent
>>>>> with its description so that it will properly return the sum of all
>>>>> matching entries for all proc handlers, instead of stopping abruptly
>>>>> as it does today.
>>>>
>>>> I'm not sure I follow.
>>>>
>>>> You seem to be saying that the function should process all of the
>>>> subtables etc even though errors have been found for some of them, but
>>>> it still will return an error in the end if there are any errors.  How
>>>> exactly does it help to continue processing in case of an error, then?
>>>
>>> The use case I have in mind is to simply count all of the subtables of
>>> a certain type.  If for some reason, the callback -- or any other callback
>>> -- fails, the traversal of all the subtables stops immediately.  So, I
>>> could have two callbacks, and if the first one fails on the first subtable
>>> of its type, traversal stops.  The count for the second callback will be
>>> zero which may or may not be correct.
>>
>> It will be zero, because the callback has not been invoked at all.
>> Why is this incorrect?
>>
>
> Because there could be additional subtables after the one causing a failure
> that the second callback could have counted; e.g., if the failure is on the
> first subtable of 20 in the MADT, the following 19 would be ignored, even if
> they were all the right subtype for the second callback.

Let me rephrase: Is there any practical value of invoking any more
callbacks if one of them has failed?  If so, what is it?

You are changing semantics from "abort on the first failure" to
"process everything and count errors".  That's quite a bit different
and I'm trying to understand why the latter is better.

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

#1435675 — Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables

On 07/01/2016 04:01 PM, Rafael J. Wysocki wrote:
> On Fri, Jul 1, 2016 at 11:55 PM, Al Stone <ahs3@redhat.com> wrote:
>> On 07/01/2016 03:46 PM, Rafael J. Wysocki wrote:
>>> On Fri, Jul 1, 2016 at 11:41 PM, Al Stone <ahs3@redhat.com> wrote:
>>>> On 07/01/2016 03:32 PM, Rafael J. Wysocki wrote:
>>>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>>>> Without this patch, the acpi_parse_entries_array() function will return
>>>>>> the very first time there is any error found in either the array of
>>>>>> callback functions or if one of the callbacks returns an non-zero value.
>>>>>> However, the array of callbacks could still have valid entries further
>>>>>> on in the array, or the callbacks may be able to process subsequent
>>>>>> subtables without error.  The change here makes the function consistent
>>>>>> with its description so that it will properly return the sum of all
>>>>>> matching entries for all proc handlers, instead of stopping abruptly
>>>>>> as it does today.
>>>>>
>>>>> I'm not sure I follow.
>>>>>
>>>>> You seem to be saying that the function should process all of the
>>>>> subtables etc even though errors have been found for some of them, but
>>>>> it still will return an error in the end if there are any errors.  How
>>>>> exactly does it help to continue processing in case of an error, then?
>>>>
>>>> The use case I have in mind is to simply count all of the subtables of
>>>> a certain type.  If for some reason, the callback -- or any other callback
>>>> -- fails, the traversal of all the subtables stops immediately.  So, I
>>>> could have two callbacks, and if the first one fails on the first subtable
>>>> of its type, traversal stops.  The count for the second callback will be
>>>> zero which may or may not be correct.
>>>
>>> It will be zero, because the callback has not been invoked at all.
>>> Why is this incorrect?
>>>
>>
>> Because there could be additional subtables after the one causing a failure
>> that the second callback could have counted; e.g., if the failure is on the
>> first subtable of 20 in the MADT, the following 19 would be ignored, even if
>> they were all the right subtype for the second callback.
> 
> Let me rephrase: Is there any practical value of invoking any more
> callbacks if one of them has failed?  If so, what is it?
> 
> You are changing semantics from "abort on the first failure" to
> "process everything and count errors".  That's quite a bit different
> and I'm trying to understand why the latter is better.
> 

Agreed, it is a shift in semantics.

The practical value to me is being able to use acpi_parse_entries_array() to
solve a broader range of problems.  The situation I have is that I need to
count three different subtable types in the MADT.  I could call
acpi_table_parse_madt() three different times, or I could call
acpi_parse_entries_array() once -- it seemed to me the second makes for cleaner
code and will be slightly more efficient (one map/unmap of the table, vs
three), but that only works if all of the subtables are traversed.

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

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

#1435683 — Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables

On Sat, Jul 2, 2016 at 1:07 AM, Al Stone <ahs3@redhat.com> wrote:
> On 07/01/2016 04:01 PM, Rafael J. Wysocki wrote:
>> On Fri, Jul 1, 2016 at 11:55 PM, Al Stone <ahs3@redhat.com> wrote:
>>> On 07/01/2016 03:46 PM, Rafael J. Wysocki wrote:
>>>> On Fri, Jul 1, 2016 at 11:41 PM, Al Stone <ahs3@redhat.com> wrote:
>>>>> On 07/01/2016 03:32 PM, Rafael J. Wysocki wrote:
>>>>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>>>>> Without this patch, the acpi_parse_entries_array() function will return
>>>>>>> the very first time there is any error found in either the array of
>>>>>>> callback functions or if one of the callbacks returns an non-zero value.
>>>>>>> However, the array of callbacks could still have valid entries further
>>>>>>> on in the array, or the callbacks may be able to process subsequent
>>>>>>> subtables without error.  The change here makes the function consistent
>>>>>>> with its description so that it will properly return the sum of all
>>>>>>> matching entries for all proc handlers, instead of stopping abruptly
>>>>>>> as it does today.
>>>>>>
>>>>>> I'm not sure I follow.
>>>>>>
>>>>>> You seem to be saying that the function should process all of the
>>>>>> subtables etc even though errors have been found for some of them, but
>>>>>> it still will return an error in the end if there are any errors.  How
>>>>>> exactly does it help to continue processing in case of an error, then?
>>>>>
>>>>> The use case I have in mind is to simply count all of the subtables of
>>>>> a certain type.  If for some reason, the callback -- or any other callback
>>>>> -- fails, the traversal of all the subtables stops immediately.  So, I
>>>>> could have two callbacks, and if the first one fails on the first subtable
>>>>> of its type, traversal stops.  The count for the second callback will be
>>>>> zero which may or may not be correct.
>>>>
>>>> It will be zero, because the callback has not been invoked at all.
>>>> Why is this incorrect?
>>>>
>>>
>>> Because there could be additional subtables after the one causing a failure
>>> that the second callback could have counted; e.g., if the failure is on the
>>> first subtable of 20 in the MADT, the following 19 would be ignored, even if
>>> they were all the right subtype for the second callback.
>>
>> Let me rephrase: Is there any practical value of invoking any more
>> callbacks if one of them has failed?  If so, what is it?
>>
>> You are changing semantics from "abort on the first failure" to
>> "process everything and count errors".  That's quite a bit different
>> and I'm trying to understand why the latter is better.
>>
>
> Agreed, it is a shift in semantics.
>
> The practical value to me is being able to use acpi_parse_entries_array() to
> solve a broader range of problems.  The situation I have is that I need to
> count three different subtable types in the MADT.  I could call
> acpi_table_parse_madt() three different times, or I could call
> acpi_parse_entries_array() once -- it seemed to me the second makes for cleaner
> code and will be slightly more efficient (one map/unmap of the table, vs
> three), but that only works if all of the subtables are traversed.

That makes sense, but then again please add a "motivation" part to the
changelog with that explanation.

Also, there is a slight danger here that some callbacks may assume
that they won't be invoked if one of the callbacks invoked earlier
returns an error.  Have you double checked that this is not the case?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web