diff mbox

[v5,03/18] ACPI / table: Count matched and successfully parsed entries without specifying max entries

Message ID 1413553034-20956-4-git-send-email-hanjun.guo@linaro.org
State New
Headers show

Commit Message

Hanjun Guo Oct. 17, 2014, 1:36 p.m. UTC
From: Tomasz Nowicki <tomasz.nowicki@linaro.org>

It is very useful to traverse all available table entries without max
number of expected entries type. Current acpi_parse_entries()
implementation gives that feature but it does not count those entries,
it returns 0 instead, so fix it to count matched and successfully
entries and return it.

NOTE: This change has no impact to x86 and ia64 archs since existing code
checks for error occurrence only (acpi_parse_entries(...,0) < 0).

Acked-by: Grant Likely <grant.likely@linaro.org>
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 drivers/acpi/tables.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Hanjun Guo Nov. 18, 2014, 1:51 p.m. UTC | #1
Hi Rafael,

On 2014年10月17日 21:36, Hanjun Guo wrote:
> From: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>
> It is very useful to traverse all available table entries without max
> number of expected entries type. Current acpi_parse_entries()
> implementation gives that feature but it does not count those entries,
> it returns 0 instead, so fix it to count matched and successfully
> entries and return it.
>
> NOTE: This change has no impact to x86 and ia64 archs since existing code
> checks for error occurrence only (acpi_parse_entries(...,0) < 0).
>
> Acked-by: Grant Likely <grant.likely@linaro.org>
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>

Patch 2 and patch 3 are only for ACPI cores, and have no harm to x86
and IA64, could you merge first in 3.19?

Thanks
Hanjun

> ---
>   drivers/acpi/tables.c |    5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 21ae521..b18e45e 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -225,10 +225,13 @@ acpi_parse_entries(unsigned long table_size,
>   	while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
>   	       table_end) {
>   		if (entry->type == entry_id
> -		    && (!max_entries || count++ < max_entries))
> +		    && (!max_entries || count < max_entries)) {
>   			if (handler(entry, table_end))
>   				return -EINVAL;
>
> +			count++;
> +		}
> +
>   		/*
>   		 * If entry->length is 0, break from this loop to avoid
>   		 * infinite loop.
>
--
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
Rafael J. Wysocki Nov. 18, 2014, 8:15 p.m. UTC | #2
On Tuesday, November 18, 2014 09:51:25 PM Hanjun Guo wrote:
> Hi Rafael,
> 
> On 2014年10月17日 21:36, Hanjun Guo wrote:
> > From: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> >
> > It is very useful to traverse all available table entries without max
> > number of expected entries type. Current acpi_parse_entries()
> > implementation gives that feature but it does not count those entries,
> > it returns 0 instead, so fix it to count matched and successfully
> > entries and return it.
> >
> > NOTE: This change has no impact to x86 and ia64 archs since existing code
> > checks for error occurrence only (acpi_parse_entries(...,0) < 0).
> >
> > Acked-by: Grant Likely <grant.likely@linaro.org>
> > Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> 
> Patch 2 and patch 3 are only for ACPI cores, and have no harm to x86
> and IA64, could you merge first in 3.19?

I can do that if that helps.
Hanjun Guo Nov. 19, 2014, 12:34 a.m. UTC | #3
On 2014-11-19 4:15, Rafael J. Wysocki wrote:
> On Tuesday, November 18, 2014 09:51:25 PM Hanjun Guo wrote:
>> Hi Rafael,
>>
>> On 2014年10月17日 21:36, Hanjun Guo wrote:
>>> From: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>>>
>>> It is very useful to traverse all available table entries without max
>>> number of expected entries type. Current acpi_parse_entries()
>>> implementation gives that feature but it does not count those entries,
>>> it returns 0 instead, so fix it to count matched and successfully
>>> entries and return it.
>>>
>>> NOTE: This change has no impact to x86 and ia64 archs since existing code
>>> checks for error occurrence only (acpi_parse_entries(...,0) < 0).
>>>
>>> Acked-by: Grant Likely <grant.likely@linaro.org>
>>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>
>> Patch 2 and patch 3 are only for ACPI cores, and have no harm to x86
>> and IA64, could you merge first in 3.19?
> 
> I can do that if that helps.

Yes, it helps to keep the next version of ARM64 ACPI patch set smaller
and easy for review. If any rebase work needed, please let me know.

Thanks
Hanjun

--
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
Tomasz Nowicki Nov. 24, 2014, 8:34 a.m. UTC | #4
On 24.11.2014 02:45, Rafael J. Wysocki wrote:
> On Friday, October 17, 2014 09:36:59 PM Hanjun Guo wrote:
>> From: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>>
>> It is very useful to traverse all available table entries without max
>> number of expected entries type. Current acpi_parse_entries()
>> implementation gives that feature but it does not count those entries,
>> it returns 0 instead, so fix it to count matched and successfully
>> entries and return it.
>
> Hmm.  I guess that the goal is for count to only be incremented when the
> condition is satisfied entirely, while without the patch it may be incremented
> even if that isn't the case.

That would be our goal if patch would look like:
-		    && (!max_entries || count++ < max_entries))
+		    && (!max_entries && count++ < max_entries)) {
but then we can not walk through all available entries (with max_entries==0)

>
> I'm not sure how that is related to the above paragraph, however.
>

Previous changelog is not clear, let me rewrite it:

acpi_parse_entries() allows to traverse all available table entries (aka 
subtables) by passing max_entries parameter equal to 0. But for that use 
case acpi_parse_entries() does not inform caller how many entries were 
matched and for how many entries handler was run against. That patch is 
going to fix it.

Regards,
Tomasz

>> NOTE: This change has no impact to x86 and ia64 archs since existing code
>> checks for error occurrence only (acpi_parse_entries(...,0) < 0).
>>
>> Acked-by: Grant Likely <grant.likely@linaro.org>
>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> ---
>>   drivers/acpi/tables.c |    5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
>> index 21ae521..b18e45e 100644
>> --- a/drivers/acpi/tables.c
>> +++ b/drivers/acpi/tables.c
>> @@ -225,10 +225,13 @@ acpi_parse_entries(unsigned long table_size,
>>   	while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
>>   	       table_end) {
>>   		if (entry->type == entry_id
>> -		    && (!max_entries || count++ < max_entries))
>> +		    && (!max_entries || count < max_entries)) {
>>   			if (handler(entry, table_end))
>>   				return -EINVAL;
>>
>> +			count++;
>> +		}
>> +
>>   		/*
>>   		 * If entry->length is 0, break from this loop to avoid
>>   		 * infinite loop.
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Tomasz Nowicki Nov. 24, 2014, 3:01 p.m. UTC | #5
On 24.11.2014 16:16, Rafael J. Wysocki wrote:
> On Monday, November 24, 2014 09:34:24 AM Tomasz Nowicki wrote:
>> On 24.11.2014 02:45, Rafael J. Wysocki wrote:
>>> On Friday, October 17, 2014 09:36:59 PM Hanjun Guo wrote:
>>>> From: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>>>>
>>>> It is very useful to traverse all available table entries without max
>>>> number of expected entries type. Current acpi_parse_entries()
>>>> implementation gives that feature but it does not count those entries,
>>>> it returns 0 instead, so fix it to count matched and successfully
>>>> entries and return it.
>>>
>>> Hmm.  I guess that the goal is for count to only be incremented when the
>>> condition is satisfied entirely, while without the patch it may be incremented
>>> even if that isn't the case.
>>
>> That would be our goal if patch would look like:
>> -		    && (!max_entries || count++ < max_entries))
>> +		    && (!max_entries && count++ < max_entries)) {
>> but then we can not walk through all available entries (with max_entries==0)
>
> No, that's not what I was trying to say. :-)
>
>>>
>>> I'm not sure how that is related to the above paragraph, however.
>>>
>>
>> Previous changelog is not clear, let me rewrite it:
>>
>> acpi_parse_entries() allows to traverse all available table entries (aka
>> subtables) by passing max_entries parameter equal to 0. But for that use
>> case acpi_parse_entries() does not inform caller how many entries were
>> matched and for how many entries handler was run against. That patch is
>> going to fix it.
>
> Do I understand correctly that count is only ever incremented by current code
> if max_entries is different from 0?

Right. Currently "count" is incremented only if max_entries > 0.

Tomasz

>
>
>>>> NOTE: This change has no impact to x86 and ia64 archs since existing code
>>>> checks for error occurrence only (acpi_parse_entries(...,0) < 0).
>>>>
>>>> Acked-by: Grant Likely <grant.likely@linaro.org>
>>>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> ---
>>>>    drivers/acpi/tables.c |    5 ++++-
>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
>>>> index 21ae521..b18e45e 100644
>>>> --- a/drivers/acpi/tables.c
>>>> +++ b/drivers/acpi/tables.c
>>>> @@ -225,10 +225,13 @@ acpi_parse_entries(unsigned long table_size,
>>>>    	while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
>>>>    	       table_end) {
>>>>    		if (entry->type == entry_id
>>>> -		    && (!max_entries || count++ < max_entries))
>>>> +		    && (!max_entries || count < max_entries)) {
>>>>    			if (handler(entry, table_end))
>>>>    				return -EINVAL;
>>>>
>>>> +			count++;
>>>> +		}
>>>> +
>>>>    		/*
>>>>    		 * If entry->length is 0, break from this loop to avoid
>>>>    		 * infinite loop.
>>>>
>>>
>> --
>> 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
Tomasz Nowicki Nov. 24, 2014, 3:18 p.m. UTC | #6
On 24.11.2014 16:37, Rafael J. Wysocki wrote:
> On Monday, November 24, 2014 04:01:24 PM Tomasz Nowicki wrote:
>> On 24.11.2014 16:16, Rafael J. Wysocki wrote:
>>> On Monday, November 24, 2014 09:34:24 AM Tomasz Nowicki wrote:
>>>> On 24.11.2014 02:45, Rafael J. Wysocki wrote:
>>>>> On Friday, October 17, 2014 09:36:59 PM Hanjun Guo wrote:
>>>>>> From: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>>>>>>
>>>>>> It is very useful to traverse all available table entries without max
>>>>>> number of expected entries type. Current acpi_parse_entries()
>>>>>> implementation gives that feature but it does not count those entries,
>>>>>> it returns 0 instead, so fix it to count matched and successfully
>>>>>> entries and return it.
>>>>>
>>>>> Hmm.  I guess that the goal is for count to only be incremented when the
>>>>> condition is satisfied entirely, while without the patch it may be incremented
>>>>> even if that isn't the case.
>>>>
>>>> That would be our goal if patch would look like:
>>>> -		    && (!max_entries || count++ < max_entries))
>>>> +		    && (!max_entries && count++ < max_entries)) {
>>>> but then we can not walk through all available entries (with max_entries==0)
>>>
>>> No, that's not what I was trying to say. :-)
>>>
>>>>>
>>>>> I'm not sure how that is related to the above paragraph, however.
>>>>>
>>>>
>>>> Previous changelog is not clear, let me rewrite it:
>>>>
>>>> acpi_parse_entries() allows to traverse all available table entries (aka
>>>> subtables) by passing max_entries parameter equal to 0. But for that use
>>>> case acpi_parse_entries() does not inform caller how many entries were
>>>> matched and for how many entries handler was run against. That patch is
>>>> going to fix it.
>>>
>>> Do I understand correctly that count is only ever incremented by current code
>>> if max_entries is different from 0?
>>
>> Right. Currently "count" is incremented only if max_entries > 0.
>
> So can you say that in the changelog, please?
>
> Something like:
>
> "acpi_parse_entries() allows to traverse all available table entries (aka
>   subtables) by passing max_entries parameter equal to 0, but since its count
>   variable is only incremented if max_entries is not 0, the function always
>   returns 0 for max_entries equal to 0.  It would be more useful if it returned
>   the number of entries matched instead, so make it increment count in that
>   case too".
>

Sure, thanks.

Tomasz
--
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
diff mbox

Patch

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 21ae521..b18e45e 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -225,10 +225,13 @@  acpi_parse_entries(unsigned long table_size,
 	while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
 	       table_end) {
 		if (entry->type == entry_id
-		    && (!max_entries || count++ < max_entries))
+		    && (!max_entries || count < max_entries)) {
 			if (handler(entry, table_end))
 				return -EINVAL;
 
+			count++;
+		}
+
 		/*
 		 * If entry->length is 0, break from this loop to avoid
 		 * infinite loop.