[PATCH] ACPI: tables: simplify acpi_parse_entries

[PATCH] ACPI: tables: simplify acpi_parse_entries

[Sudeep Holla]

acpi_parse_entries passes the table end pointer to the sub-table entry
handler. acpi_parse_entries itself could validate the end of an entry
against the table end using the length in the sub-table entry.

This patch adds the validation of the sub-table entry end using the
length field.This will help to eliminate the need to pass the table end
to the handlers.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/tables.c | 34 +++++++++-------------------------
 1 file changed, 9 insertions(+), 25 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 17a6fa01a338..145d4f6a1c54 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -217,16 +217,13 @@ acpi_parse_entries(char *id, unsigned long table_size,
 		int entry_id, unsigned int max_entries)
 {
 	struct acpi_subtable_header *entry;
+	void *entry_end, *table_end;
 	int count = 0;
-	unsigned long table_end;
 
 	if (acpi_disabled)
 		return -ENODEV;
 
-	if (!id || !handler)
-		return -EINVAL;
-
-	if (!table_size)
+	if (!id || !handler || !table_size)
 		return -EINVAL;
 
 	if (!table_header) {
@@ -234,34 +231,21 @@ acpi_parse_entries(char *id, unsigned long table_size,
 		return -ENODEV;
 	}
 
-	table_end = (unsigned long)table_header + table_header->length;
+	table_end = (void *)table_header + table_header->length;
 
 	/* Parse all entries looking for a match. */
+	entry = (void *)table_header + table_size;
+	entry_end = (void *)entry + entry->length;
 
-	entry = (struct acpi_subtable_header *)
-	    ((unsigned long)table_header + table_size);
-
-	while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
-	       table_end) {
+	while (entry->length && entry_end <= table_end) {
 		if (entry->type == entry_id
 		    && (!max_entries || count < max_entries)) {
-			if (handler(entry, table_end))
+			if (handler(entry, (unsigned long)entry_end))
 				return -EINVAL;
-
 			count++;
 		}
-
-		/*
-		 * If entry->length is 0, break from this loop to avoid
-		 * infinite loop.
-		 */
-		if (entry->length == 0) {
-			pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id);
-			return -EINVAL;
-		}
-
-		entry = (struct acpi_subtable_header *)
-		    ((unsigned long)entry + entry->length);
+		entry = entry_end;
+		entry_end = (void *)entry + entry->length;
 	}
 
 	if (max_entries && count > max_entries) {
-- 
1.9.1

Re: [PATCH] ACPI: tables: simplify acpi_parse_entries

[Rafael J. Wysocki]

On Monday, September 14, 2015 04:14:46 PM Sudeep Holla wrote:
> acpi_parse_entries passes the table end pointer to the sub-table entry
> handler. acpi_parse_entries itself could validate the end of an entry
> against the table end using the length in the sub-table entry.
> 
> This patch adds the validation of the sub-table entry end using the
> length field.This will help to eliminate the need to pass the table end
> to the handlers.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Well, I'm not a big fan of (void *) arithmetics and the patch seems to be
doing too much to me.

> ---
>  drivers/acpi/tables.c | 34 +++++++++-------------------------
>  1 file changed, 9 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 17a6fa01a338..145d4f6a1c54 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -217,16 +217,13 @@ acpi_parse_entries(char *id, unsigned long table_size,
>  		int entry_id, unsigned int max_entries)
>  {
>  	struct acpi_subtable_header *entry;
> +	void *entry_end, *table_end;
>  	int count = 0;
> -	unsigned long table_end;

I'd keep that as unsigned long and I'd add unsigned long entry_end here.

>  
>  	if (acpi_disabled)
>  		return -ENODEV;
>  
> -	if (!id || !handler)
> -		return -EINVAL;
> -
> -	if (!table_size)
> +	if (!id || !handler || !table_size)
>  		return -EINVAL;

Please mention this cleanup bit in the changelog, as it is not related to
the other changes.

>  
>  	if (!table_header) {
> @@ -234,34 +231,21 @@ acpi_parse_entries(char *id, unsigned long table_size,
>  		return -ENODEV;
>  	}
>  
> -	table_end = (unsigned long)table_header + table_header->length;
> +	table_end = (void *)table_header + table_header->length;
>  
>  	/* Parse all entries looking for a match. */
> +	entry = (void *)table_header + table_size;
> +	entry_end = (void *)entry + entry->length;
>  
> -	entry = (struct acpi_subtable_header *)
> -	    ((unsigned long)table_header + table_size);

	entry_end = (unsigned long)table_header + table_size;
	entry = (struct acpi_subtable_header *)entry_end;
	entry_end += entry->length;

> -
> -	while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
> -	       table_end) {
> +	while (entry->length && entry_end <= table_end) {

We used to return -EINVAL for entry->length == 0 and now we don't.  Isn't that
a problem?

>  		if (entry->type == entry_id
>  		    && (!max_entries || count < max_entries)) {
> -			if (handler(entry, table_end))
> +			if (handler(entry, (unsigned long)entry_end))
>  				return -EINVAL;
> -
>  			count++;
>  		}
> -
> -		/*
> -		 * If entry->length is 0, break from this loop to avoid
> -		 * infinite loop.
> -		 */
> -		if (entry->length == 0) {
> -			pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id);
> -			return -EINVAL;
> -		}

Perhaps just reorder this with the previous if () and write the loop as

	while (entry_end <= table_end) {

> -
> -		entry = (struct acpi_subtable_header *)
> -		    ((unsigned long)entry + entry->length);
> +		entry = entry_end;
> +		entry_end = (void *)entry + entry->length;

And this can be

		entry = (struct acpi_subtable_header *)entry_end;
		entry_end += entry->length;

>  	}
>  
>  	if (max_entries && count > max_entries) {
> 

Thanks,
Rafael

Re: [PATCH] ACPI: tables: simplify acpi_parse_entries

[Sudeep Holla]

On 15/09/15 00:41, Rafael J. Wysocki wrote:
> On Monday, September 14, 2015 04:14:46 PM Sudeep Holla wrote:
>> acpi_parse_entries passes the table end pointer to the sub-table entry
>> handler. acpi_parse_entries itself could validate the end of an entry
>> against the table end using the length in the sub-table entry.
>>
>> This patch adds the validation of the sub-table entry end using the
>> length field.This will help to eliminate the need to pass the table end
>> to the handlers.
>>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> Well, I'm not a big fan of (void *) arithmetics and the patch seems to be
> doing too much to me.
>

Agreed, I will try to remove(if not most likely reduce) it.

>> ---
>>   drivers/acpi/tables.c | 34 +++++++++-------------------------
>>   1 file changed, 9 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
>> index 17a6fa01a338..145d4f6a1c54 100644
>> --- a/drivers/acpi/tables.c
>> +++ b/drivers/acpi/tables.c
>> @@ -217,16 +217,13 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>   		int entry_id, unsigned int max_entries)
>>   {
>>   	struct acpi_subtable_header *entry;
>> +	void *entry_end, *table_end;
>>   	int count = 0;
>> -	unsigned long table_end;
>
> I'd keep that as unsigned long and I'd add unsigned long entry_end here.
>

OK will change accordingly

>>
>>   	if (acpi_disabled)
>>   		return -ENODEV;
>>
>> -	if (!id || !handler)
>> -		return -EINVAL;
>> -
>> -	if (!table_size)
>> +	if (!id || !handler || !table_size)
>>   		return -EINVAL;
>
> Please mention this cleanup bit in the changelog, as it is not related to
> the other changes.
>

Ah right, sorry for unnecessary change. Better I will remove it.

>>
>>   	if (!table_header) {
>> @@ -234,34 +231,21 @@ acpi_parse_entries(char *id, unsigned long table_size,
>>   		return -ENODEV;
>>   	}
>>
>> -	table_end = (unsigned long)table_header + table_header->length;
>> +	table_end = (void *)table_header + table_header->length;
>>
>>   	/* Parse all entries looking for a match. */
>> +	entry = (void *)table_header + table_size;
>> +	entry_end = (void *)entry + entry->length;
>>
>> -	entry = (struct acpi_subtable_header *)
>> -	    ((unsigned long)table_header + table_size);
>
> 	entry_end = (unsigned long)table_header + table_size;
> 	entry = (struct acpi_subtable_header *)entry_end;
> 	entry_end += entry->length;
>
>> -
>> -	while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
>> -	       table_end) {
>> +	while (entry->length && entry_end <= table_end) {
>
> We used to return -EINVAL for entry->length == 0 and now we don't.  Isn't that
> a problem?
>

I thought the main reason was to break the infinite loop, will retain
that as you suggested.

>>   		if (entry->type == entry_id
>>   		    && (!max_entries || count < max_entries)) {
>> -			if (handler(entry, table_end))
>> +			if (handler(entry, (unsigned long)entry_end))
>>   				return -EINVAL;
>> -
>>   			count++;
>>   		}
>> -
>> -		/*
>> -		 * If entry->length is 0, break from this loop to avoid
>> -		 * infinite loop.
>> -		 */
>> -		if (entry->length == 0) {
>> -			pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id);
>> -			return -EINVAL;
>> -		}
>
> Perhaps just reorder this with the previous if () and write the loop as
>

Make sense.

> 	while (entry_end <= table_end) {
>
>> -
>> -		entry = (struct acpi_subtable_header *)
>> -		    ((unsigned long)entry + entry->length);
>> +		entry = entry_end;
>> +		entry_end = (void *)entry + entry->length;
>
> And this can be
>
> 		entry = (struct acpi_subtable_header *)entry_end;
> 		entry_end += entry->length;
>

OK

Anyways I realized that I tested this on top of Al's MADT cleanup
series. I think this might break BAD_MADT_ENTRY macro which expects to
get table_end rather than entry_end(though latter is correct)

I am not sure if that's the case with APIC, but for MADT and
BAD_MADT_GICC_ENTRY, entry + sizeof(*entry) > entry_end can be true(as
the specification is broken and Al is trying to solve)

I will respin the patch as per your suggestion but needs to go only
after Al's MADT patches.

Regards,
Sudeep

Re: [PATCH] ACPI: tables: simplify acpi_parse_entries

[Sudeep Holla]

On 15/09/15 10:31, Sudeep Holla wrote:
>
>
> On 15/09/15 00:41, Rafael J. Wysocki wrote:
>> On Monday, September 14, 2015 04:14:46 PM Sudeep Holla wrote:
>>> acpi_parse_entries passes the table end pointer to the sub-table entry
>>> handler. acpi_parse_entries itself could validate the end of an entry
>>> against the table end using the length in the sub-table entry.
>>>
>>> This patch adds the validation of the sub-table entry end using the
>>> length field.This will help to eliminate the need to pass the table end
>>> to the handlers.
>>>
>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>

[...]

> I will respin the patch as per your suggestion but needs to go only
> after Al's MADT patches.
>

Alternately we can retain passing table_end to the handlers for now.

Once Al's MADT series removes those BAD_GIC_ENTRY macros, there will be
no users of that second argument in subtable handlers and it can be
completely removed. Let me know your thoughts.

Regards,
Sudeep

Re: [PATCH] ACPI: tables: simplify acpi_parse_entries

[Rafael J. Wysocki]

On Tuesday, September 15, 2015 02:35:32 PM Sudeep Holla wrote:
> 
> On 15/09/15 10:31, Sudeep Holla wrote:
> >
> >
> > On 15/09/15 00:41, Rafael J. Wysocki wrote:
> >> On Monday, September 14, 2015 04:14:46 PM Sudeep Holla wrote:
> >>> acpi_parse_entries passes the table end pointer to the sub-table entry
> >>> handler. acpi_parse_entries itself could validate the end of an entry
> >>> against the table end using the length in the sub-table entry.
> >>>
> >>> This patch adds the validation of the sub-table entry end using the
> >>> length field.This will help to eliminate the need to pass the table end
> >>> to the handlers.
> >>>
> >>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >>
> 
> [...]
> 
> > I will respin the patch as per your suggestion but needs to go only
> > after Al's MADT patches.
> >
> 
> Alternately we can retain passing table_end to the handlers for now.
> 
> Once Al's MADT series removes those BAD_GIC_ENTRY macros, there will be
> no users of that second argument in subtable handlers and it can be
> completely removed. Let me know your thoughts.

I'm going to apply the Al's series, so that sounds like the way to go.

Thanks,
Rafael

[PATCH v2 1/2] ACPI / tables: simplify acpi_parse_entries

[Sudeep Holla]

acpi_parse_entries passes the table end pointer to the sub-table entry
handler. acpi_parse_entries itself could validate the end of an entry
against the table end using the length in the sub-table entry.

This patch adds the validation of the sub-table entry end using the
length field.This will help to eliminate the need to pass the table end
to the handlers.

It also moves the check for zero length entry early so that execution of
the handler can be avoided.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/tables.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

Hi Rafael,

As I mentioned earlier, this needs to be applied after Al's MADT changes
are merged. You might get simple conflicts in acpi_parse_entries.

Regards,
Sudeep

v1->v2:
	- Incorporated Rafael's review comments
	- Moved zero length entry check early
	- Added a patch to remove the unused table_end parameter

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index d0716bb6359d..69b9d05f5b96 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -459,7 +459,7 @@ acpi_parse_entries(char *id, unsigned long table_size,
 {
 	struct acpi_subtable_header *entry;
 	int count = 0;
-	unsigned long table_end;
+	unsigned long table_end, entry_end;

 	if (acpi_disabled)
 		return -ENODEV;
@@ -478,12 +478,20 @@ acpi_parse_entries(char *id, unsigned long table_size,
 	table_end = (unsigned long)table_header + table_header->length;

 	/* Parse all entries looking for a match. */
+	entry_end = (unsigned long)table_header + table_size;
+	entry = (struct acpi_subtable_header *)entry_end;
+	entry_end += entry->length;

-	entry = (struct acpi_subtable_header *)
-	    ((unsigned long)table_header + table_size);
+	while (entry_end <= table_end) {
+		/*
+		 * If entry->length is 0, break from this loop to avoid
+		 * infinite loop.
+		 */
+		if (entry->length == 0) {
+			pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id);
+			return -EINVAL;
+		}

-	while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
-	       table_end) {
 		if (entry->type == entry_id
 		    && (!max_entries || count < max_entries)) {
 			if (!strncmp(id, ACPI_SIG_MADT, 4) &&
@@ -495,17 +503,8 @@ acpi_parse_entries(char *id, unsigned long table_size,
 			count++;
 		}

-		/*
-		 * If entry->length is 0, break from this loop to avoid
-		 * infinite loop.
-		 */
-		if (entry->length == 0) {
-			pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id);
-			return -EINVAL;
-		}
-
-		entry = (struct acpi_subtable_header *)
-		    ((unsigned long)entry + entry->length);
+		entry = (struct acpi_subtable_header *)entry_end;
+		entry_end += entry->length;
 	}

 	if (max_entries && count > max_entries) {
--
1.9.1

[PATCH v2 2/2] ACPI / tables : remove unused table_end parameter to acpi_tbl_entry_handler

[Sudeep Holla]

acpi_tbl_entiry_handler expects `table_end` pointer to be passed so that
the individual subtable entry handler can use it to validate the entries.

However, acpi_parse_entries now validates the end of an entry against
the table end using the length in the sub-table entry. All the only user
of that argument namely MADT is also removed. So we can now eliminate
the need to pass the table end to the handlers.

This patch removes the unused second parameter in acpi_tbl_entry_handler
and updates all the handlers accordingly.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/kernel/smp.c     |  3 +--
 arch/ia64/kernel/acpi.c     | 22 +++++++---------------
 arch/x86/kernel/acpi/boot.c | 29 +++++++++--------------------
 drivers/acpi/numa.c         |  9 +++------
 drivers/acpi/tables.c       |  2 +-
 drivers/irqchip/irq-gic.c   |  7 ++-----
 drivers/mailbox/pcc.c       |  3 +--
 include/linux/acpi.h        |  3 +--
 8 files changed, 25 insertions(+), 53 deletions(-)

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 66cc8c4d170c..885299b1a24b 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -445,8 +445,7 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
 }

 static int __init
-acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
-			     const unsigned long end)
+acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_generic_interrupt *processor;

diff --git a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c
index efa3f0a299e2..0549504a8f6c 100644
--- a/arch/ia64/kernel/acpi.c
+++ b/arch/ia64/kernel/acpi.c
@@ -177,8 +177,7 @@ struct acpi_table_madt *acpi_madt __initdata;
 static u8 has_8259;

 static int __init
-acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
-			  const unsigned long end)
+acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header)
 {
 	struct acpi_madt_local_apic_override *lapic;

@@ -191,8 +190,7 @@ acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
 	return 0;
 }

-static int __init
-acpi_parse_lsapic(struct acpi_subtable_header * header, const unsigned long end)
+static int __init acpi_parse_lsapic(struct acpi_subtable_header * header)
 {
 	struct acpi_madt_local_sapic *lsapic;

@@ -210,8 +208,7 @@ acpi_parse_lsapic(struct acpi_subtable_header * header, const unsigned long end)
 	return 0;
 }

-static int __init
-acpi_parse_lapic_nmi(struct acpi_subtable_header * header, const unsigned long end)
+static int __init acpi_parse_lapic_nmi(struct acpi_subtable_header * header)
 {
 	struct acpi_madt_local_apic_nmi *lacpi_nmi;

@@ -221,8 +218,7 @@ acpi_parse_lapic_nmi(struct acpi_subtable_header * header, const unsigned long e
 	return 0;
 }

-static int __init
-acpi_parse_iosapic(struct acpi_subtable_header * header, const unsigned long end)
+static int __init acpi_parse_iosapic(struct acpi_subtable_header * header)
 {
 	struct acpi_madt_io_sapic *iosapic;

@@ -234,8 +230,7 @@ acpi_parse_iosapic(struct acpi_subtable_header * header, const unsigned long end
 static unsigned int __initdata acpi_madt_rev;

 static int __init
-acpi_parse_plat_int_src(struct acpi_subtable_header * header,
-			const unsigned long end)
+acpi_parse_plat_int_src(struct acpi_subtable_header * header)
 {
 	struct acpi_madt_interrupt_source *plintsrc;
 	int vector;
@@ -314,9 +309,7 @@ unsigned int get_cpei_target_cpu(void)
 	return acpi_cpei_phys_cpuid;
 }

-static int __init
-acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
-		       const unsigned long end)
+static int __init acpi_parse_int_src_ovr(struct acpi_subtable_header * header)
 {
 	struct acpi_madt_interrupt_override *p;

@@ -332,8 +325,7 @@ acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
 	return 0;
 }

-static int __init
-acpi_parse_nmi_src(struct acpi_subtable_header * header, const unsigned long end)
+static int __init acpi_parse_nmi_src(struct acpi_subtable_header * header)
 {
 	struct acpi_madt_nmi_source *nmi_src;

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index d37b84d4843b..7d7692df6fbb 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -185,8 +185,7 @@ static int acpi_register_lapic(int id, u8 enabled)
 	return generic_processor_info(id, ver);
 }

-static int __init
-acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
+static int __init acpi_parse_x2apic(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_local_x2apic *processor = NULL;
 	int apic_id;
@@ -217,8 +216,7 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
 	return 0;
 }

-static int __init
-acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end)
+static int __init acpi_parse_lapic(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_local_apic *processor = NULL;

@@ -239,8 +237,7 @@ acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end)
 	return 0;
 }

-static int __init
-acpi_parse_sapic(struct acpi_subtable_header *header, const unsigned long end)
+static int __init acpi_parse_sapic(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_local_sapic *processor = NULL;

@@ -255,8 +252,7 @@ acpi_parse_sapic(struct acpi_subtable_header *header, const unsigned long end)
 }

 static int __init
-acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
-			  const unsigned long end)
+acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header)
 {
 	struct acpi_madt_local_apic_override *lapic_addr_ovr = NULL;

@@ -267,9 +263,7 @@ acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
 	return 0;
 }

-static int __init
-acpi_parse_x2apic_nmi(struct acpi_subtable_header *header,
-		      const unsigned long end)
+static int __init acpi_parse_x2apic_nmi(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_local_x2apic_nmi *x2apic_nmi = NULL;

@@ -283,8 +277,7 @@ acpi_parse_x2apic_nmi(struct acpi_subtable_header *header,
 	return 0;
 }

-static int __init
-acpi_parse_lapic_nmi(struct acpi_subtable_header * header, const unsigned long end)
+static int __init acpi_parse_lapic_nmi(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_local_apic_nmi *lapic_nmi = NULL;

@@ -382,8 +375,7 @@ static int mp_config_acpi_gsi(struct device *dev, u32 gsi, int trigger,
 	return 0;
 }

-static int __init
-acpi_parse_ioapic(struct acpi_subtable_header * header, const unsigned long end)
+static int __init acpi_parse_ioapic(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_io_apic *ioapic = NULL;
 	struct ioapic_domain_cfg cfg = {
@@ -434,9 +426,7 @@ static void __init acpi_sci_ioapic_setup(u8 bus_irq, u16 polarity, u16 trigger,
 	return;
 }

-static int __init
-acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
-		       const unsigned long end)
+static int __init acpi_parse_int_src_ovr(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_interrupt_override *intsrc = NULL;

@@ -473,8 +463,7 @@ acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
 	return 0;
 }

-static int __init
-acpi_parse_nmi_src(struct acpi_subtable_header * header, const unsigned long end)
+static int __init acpi_parse_nmi_src(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_nmi_source *nmi_src = NULL;

diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c
index 72b6e9ef0ae9..ecaaa3087c86 100644
--- a/drivers/acpi/numa.c
+++ b/drivers/acpi/numa.c
@@ -240,8 +240,7 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)


 static int __init
-acpi_parse_x2apic_affinity(struct acpi_subtable_header *header,
-			   const unsigned long end)
+acpi_parse_x2apic_affinity(struct acpi_subtable_header *header)
 {
 	struct acpi_srat_x2apic_cpu_affinity *processor_affinity;

@@ -258,8 +257,7 @@ acpi_parse_x2apic_affinity(struct acpi_subtable_header *header,
 }

 static int __init
-acpi_parse_processor_affinity(struct acpi_subtable_header *header,
-			      const unsigned long end)
+acpi_parse_processor_affinity(struct acpi_subtable_header *header)
 {
 	struct acpi_srat_cpu_affinity *processor_affinity;

@@ -278,8 +276,7 @@ acpi_parse_processor_affinity(struct acpi_subtable_header *header,
 static int __initdata parsed_numa_memblks;

 static int __init
-acpi_parse_memory_affinity(struct acpi_subtable_header * header,
-			   const unsigned long end)
+acpi_parse_memory_affinity(struct acpi_subtable_header * header)
 {
 	struct acpi_srat_mem_affinity *memory_affinity;

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 69b9d05f5b96..d6f8d42b3cef 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -497,7 +497,7 @@ acpi_parse_entries(char *id, unsigned long table_size,
 			if (!strncmp(id, ACPI_SIG_MADT, 4) &&
 			    bad_madt_entry(table_header, entry))
 				return -EINVAL;
-			if (handler(entry, table_end))
+			if (handler(entry))
 				return -EINVAL;

 			count++;
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index b3530e37c036..4e1cb87bb91c 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1179,9 +1179,7 @@ IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init);
 #ifdef CONFIG_ACPI
 static phys_addr_t dist_phy_base, cpu_phy_base __initdata;

-static int __init
-gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
-			const unsigned long end)
+static int __init gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_generic_interrupt *processor;
 	phys_addr_t gic_cpu_base;
@@ -1203,8 +1201,7 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
 }

 static int __init
-gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header,
-				const unsigned long end)
+gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_generic_distributor *dist;

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 68885a82e704..3df1f792728b 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -212,8 +212,7 @@ static const struct mbox_chan_ops pcc_chan_ops = {
  *
  * This gets called for each entry in the PCC table.
  */
-static int parse_pcc_subspace(struct acpi_subtable_header *header,
-		const unsigned long end)
+static int parse_pcc_subspace(struct acpi_subtable_header *header)
 {
 	struct acpi_pcct_hw_reduced *pcct_ss;

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index a6d6326e5ced..a9e25c4a44d1 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -116,8 +116,7 @@ enum acpi_address_range_id {

 typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table);

-typedef int (*acpi_tbl_entry_handler)(struct acpi_subtable_header *header,
-				      const unsigned long end);
+typedef int (*acpi_tbl_entry_handler)(struct acpi_subtable_header *header);

 #ifdef CONFIG_ACPI_INITRD_TABLE_OVERRIDE
 void acpi_initrd_override(void *data, size_t size);
--
1.9.1

Re: [PATCH v2 1/2] ACPI / tables: simplify acpi_parse_entries

[Rafael J. Wysocki]

On Wednesday, September 16, 2015 01:58:06 PM Sudeep Holla wrote:
> acpi_parse_entries passes the table end pointer to the sub-table entry
> handler. acpi_parse_entries itself could validate the end of an entry
> against the table end using the length in the sub-table entry.
> 
> This patch adds the validation of the sub-table entry end using the
> length field.This will help to eliminate the need to pass the table end
> to the handlers.
> 
> It also moves the check for zero length entry early so that execution of
> the handler can be avoided.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> ---
>  drivers/acpi/tables.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
> 
> Hi Rafael,
> 
> As I mentioned earlier, this needs to be applied after Al's MADT changes
> are merged. You might get simple conflicts in acpi_parse_entries.

This needs to be rebased on top of some patches in my linux-next branch.

It probably is better to rebase it on top of my bleeding-edge branch that
contains the Al's patches already, though.

Thanks,
Rafael

Re: [PATCH v2 1/2] ACPI / tables: simplify acpi_parse_entries

[Sudeep Holla]

On 26/09/15 01:27, Rafael J. Wysocki wrote:
> On Wednesday, September 16, 2015 01:58:06 PM Sudeep Holla wrote:
>> acpi_parse_entries passes the table end pointer to the sub-table entry
>> handler. acpi_parse_entries itself could validate the end of an entry
>> against the table end using the length in the sub-table entry.
>>
>> This patch adds the validation of the sub-table entry end using the
>> length field.This will help to eliminate the need to pass the table end
>> to the handlers.
>>
>> It also moves the check for zero length entry early so that execution of
>> the handler can be avoided.
>>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>
>> ---
>>   drivers/acpi/tables.c | 31 +++++++++++++++----------------
>>   1 file changed, 15 insertions(+), 16 deletions(-)
>>
>> Hi Rafael,
>>
>> As I mentioned earlier, this needs to be applied after Al's MADT changes
>> are merged. You might get simple conflicts in acpi_parse_entries.
>
> This needs to be rebased on top of some patches in my linux-next branch.
>
> It probably is better to rebase it on top of my bleeding-edge branch that
> contains the Al's patches already, though.
>

I don't see Al's patches in your linux-next or bleeding-edge

Regards,
Sudeep

Re: [PATCH v2 1/2] ACPI / tables: simplify acpi_parse_entries

[Sudeep Holla]

On 28/09/15 14:50, Rafael J. Wysocki wrote:
> On Monday, September 28, 2015 11:11:11 AM Sudeep Holla wrote:
>>
>> On 26/09/15 01:27, Rafael J. Wysocki wrote:
>>> On Wednesday, September 16, 2015 01:58:06 PM Sudeep Holla wrote:
>>>> acpi_parse_entries passes the table end pointer to the sub-table entry
>>>> handler. acpi_parse_entries itself could validate the end of an entry
>>>> against the table end using the length in the sub-table entry.
>>>>
>>>> This patch adds the validation of the sub-table entry end using the
>>>> length field.This will help to eliminate the need to pass the table end
>>>> to the handlers.
>>>>
>>>> It also moves the check for zero length entry early so that execution of
>>>> the handler can be avoided.
>>>>
>>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>>>
>>>> ---
>>>>    drivers/acpi/tables.c | 31 +++++++++++++++----------------
>>>>    1 file changed, 15 insertions(+), 16 deletions(-)
>>>>
>>>> Hi Rafael,
>>>>
>>>> As I mentioned earlier, this needs to be applied after Al's MADT changes
>>>> are merged. You might get simple conflicts in acpi_parse_entries.
>>>
>>> This needs to be rebased on top of some patches in my linux-next branch.
>>>
>>> It probably is better to rebase it on top of my bleeding-edge branch that
>>> contains the Al's patches already, though.
>>>
>>
>> I don't see Al's patches in your linux-next or bleeding-edge
>
> They were there, but I've dropped them due to a 0-day testing failure.
>

Yes I guess we did see this last week, I had ask Al to fix it privately.
It was some discrepancy with ACPIv1.0 specification between different
sections that resulted in failures I saw.

> I think your patches depend on the Al's ones, is that correct?
>

Correct, I think it's easier if I wait for his patches.

Regards,
Sudeep

Re: [PATCH v2 1/2] ACPI / tables: simplify acpi_parse_entries

[Rafael J. Wysocki]

On Monday, September 28, 2015 11:11:11 AM Sudeep Holla wrote:
> 
> On 26/09/15 01:27, Rafael J. Wysocki wrote:
> > On Wednesday, September 16, 2015 01:58:06 PM Sudeep Holla wrote:
> >> acpi_parse_entries passes the table end pointer to the sub-table entry
> >> handler. acpi_parse_entries itself could validate the end of an entry
> >> against the table end using the length in the sub-table entry.
> >>
> >> This patch adds the validation of the sub-table entry end using the
> >> length field.This will help to eliminate the need to pass the table end
> >> to the handlers.
> >>
> >> It also moves the check for zero length entry early so that execution of
> >> the handler can be avoided.
> >>
> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >>
> >> ---
> >>   drivers/acpi/tables.c | 31 +++++++++++++++----------------
> >>   1 file changed, 15 insertions(+), 16 deletions(-)
> >>
> >> Hi Rafael,
> >>
> >> As I mentioned earlier, this needs to be applied after Al's MADT changes
> >> are merged. You might get simple conflicts in acpi_parse_entries.
> >
> > This needs to be rebased on top of some patches in my linux-next branch.
> >
> > It probably is better to rebase it on top of my bleeding-edge branch that
> > contains the Al's patches already, though.
> >
> 
> I don't see Al's patches in your linux-next or bleeding-edge

They were there, but I've dropped them due to a 0-day testing failure.

I think your patches depend on the Al's ones, is that correct?

Rafael

Re: [PATCH v2 1/2] ACPI / tables: simplify acpi_parse_entries

[Al Stone]

On 09/28/2015 07:37 AM, Sudeep Holla wrote:
> 
> 
> On 28/09/15 14:50, Rafael J. Wysocki wrote:
>> On Monday, September 28, 2015 11:11:11 AM Sudeep Holla wrote:
>>>
>>> On 26/09/15 01:27, Rafael J. Wysocki wrote:
>>>> On Wednesday, September 16, 2015 01:58:06 PM Sudeep Holla wrote:
>>>>> acpi_parse_entries passes the table end pointer to the sub-table entry
>>>>> handler. acpi_parse_entries itself could validate the end of an entry
>>>>> against the table end using the length in the sub-table entry.
>>>>>
>>>>> This patch adds the validation of the sub-table entry end using the
>>>>> length field.This will help to eliminate the need to pass the table end
>>>>> to the handlers.
>>>>>
>>>>> It also moves the check for zero length entry early so that execution of
>>>>> the handler can be avoided.
>>>>>
>>>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>>>>
>>>>> ---
>>>>>    drivers/acpi/tables.c | 31 +++++++++++++++----------------
>>>>>    1 file changed, 15 insertions(+), 16 deletions(-)
>>>>>
>>>>> Hi Rafael,
>>>>>
>>>>> As I mentioned earlier, this needs to be applied after Al's MADT changes
>>>>> are merged. You might get simple conflicts in acpi_parse_entries.
>>>>
>>>> This needs to be rebased on top of some patches in my linux-next branch.
>>>>
>>>> It probably is better to rebase it on top of my bleeding-edge branch that
>>>> contains the Al's patches already, though.
>>>>
>>>
>>> I don't see Al's patches in your linux-next or bleeding-edge
>>
>> They were there, but I've dropped them due to a 0-day testing failure.
>>
> 
> Yes I guess we did see this last week, I had ask Al to fix it privately.
> It was some discrepancy with ACPIv1.0 specification between different
> sections that resulted in failures I saw.
> 
>> I think your patches depend on the Al's ones, is that correct?
>>
> 
> Correct, I think it's easier if I wait for his patches.
> 
> Regards,
> Sudeep

My apologies.  Was participating in family stuff all weekend
and Linaro Connect all last week.

This appears to be an incorrect reading of the 1.0 spec, and not
being able to find the 1.0b version, on my part.  Unfortunately,
http://www.acpi.info/DOWNLOADS/*spec*.pdf is not public so one has
to guess at files names for older versions of the spec -- and I
assumed 1.0B, the current naming practice.

Sorry about that...the patch is pretty simple, I think.  Rafael,
which tree do you want me to base the respin on?  Your bleeding-edge
branch?

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------

Re: [PATCH v2 1/2] ACPI / tables: simplify acpi_parse_entries

[Rafael J. Wysocki]

Hi,

On Mon, Sep 28, 2015 at 9:39 PM, Al Stone <al.stone@linaro.org> wrote:
> On 09/28/2015 07:37 AM, Sudeep Holla wrote:
>>
>>
>> On 28/09/15 14:50, Rafael J. Wysocki wrote:
>>> On Monday, September 28, 2015 11:11:11 AM Sudeep Holla wrote:
>>>>
>>>> On 26/09/15 01:27, Rafael J. Wysocki wrote:
>>>>> On Wednesday, September 16, 2015 01:58:06 PM Sudeep Holla wrote:
>>>>>> acpi_parse_entries passes the table end pointer to the sub-table entry
>>>>>> handler. acpi_parse_entries itself could validate the end of an entry
>>>>>> against the table end using the length in the sub-table entry.
>>>>>>
>>>>>> This patch adds the validation of the sub-table entry end using the
>>>>>> length field.This will help to eliminate the need to pass the table end
>>>>>> to the handlers.
>>>>>>
>>>>>> It also moves the check for zero length entry early so that execution of
>>>>>> the handler can be avoided.
>>>>>>
>>>>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>>>>>
>>>>>> ---
>>>>>>    drivers/acpi/tables.c | 31 +++++++++++++++----------------
>>>>>>    1 file changed, 15 insertions(+), 16 deletions(-)
>>>>>>
>>>>>> Hi Rafael,
>>>>>>
>>>>>> As I mentioned earlier, this needs to be applied after Al's MADT changes
>>>>>> are merged. You might get simple conflicts in acpi_parse_entries.
>>>>>
>>>>> This needs to be rebased on top of some patches in my linux-next branch.
>>>>>
>>>>> It probably is better to rebase it on top of my bleeding-edge branch that
>>>>> contains the Al's patches already, though.
>>>>>
>>>>
>>>> I don't see Al's patches in your linux-next or bleeding-edge
>>>
>>> They were there, but I've dropped them due to a 0-day testing failure.
>>>
>>
>> Yes I guess we did see this last week, I had ask Al to fix it privately.
>> It was some discrepancy with ACPIv1.0 specification between different
>> sections that resulted in failures I saw.
>>
>>> I think your patches depend on the Al's ones, is that correct?
>>>
>>
>> Correct, I think it's easier if I wait for his patches.
>>
>> Regards,
>> Sudeep
>
> My apologies.  Was participating in family stuff all weekend
> and Linaro Connect all last week.
>
> This appears to be an incorrect reading of the 1.0 spec, and not
> being able to find the 1.0b version, on my part.  Unfortunately,
> http://www.acpi.info/DOWNLOADS/*spec*.pdf is not public so one has
> to guess at files names for older versions of the spec -- and I
> assumed 1.0B, the current naming practice.
>
> Sorry about that...the patch is pretty simple, I think.  Rafael,
> which tree do you want me to base the respin on?  Your bleeding-edge
> branch?

My linux-next branch should be OK for that.

I guess there will be a conflict between your patches and the Marc's
ones, but I can resolve that one I suppose.

Thanks,
Rafael

[PATCH v3 1/2] ACPI / tables: simplify acpi_parse_entries

[Sudeep Holla]

acpi_parse_entries passes the table end pointer to the sub-table entry
handler. acpi_parse_entries itself could validate the end of an entry
against the table end using the length in the sub-table entry.

This patch adds the validation of the sub-table entry end using the
length field.This will help to eliminate the need to pass the table end
to the handlers.

It also moves the check for zero length entry early so that execution of
the handler can be avoided.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/tables.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

v2->v3:
	- Rebased on Rafael's linux-pm/bleeding-edge branch to avoid
	  conflicts
v1->v2:
        - Incorporated Rafael's review comments
        - Moved zero length entry check early
        - Added a patch to remove the unused table_end parameter

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index a2ed38a20e7e..24b867e26191 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -476,7 +476,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
 		unsigned int max_entries)
 {
 	struct acpi_subtable_header *entry;
-	unsigned long table_end;
+	unsigned long table_end, entry_end;
 	int count = 0;
 	int i;
 
@@ -497,12 +497,20 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
 	table_end = (unsigned long)table_header + table_header->length;
 
 	/* Parse all entries looking for a match. */
+	entry_end = (unsigned long)table_header + table_size;
+	entry = (struct acpi_subtable_header *)entry_end;
+	entry_end += entry->length;
 
-	entry = (struct acpi_subtable_header *)
-	    ((unsigned long)table_header + table_size);
+	while (entry_end <= table_end) {
+		/*
+		 * If entry->length is 0, break from this loop to avoid
+		 * infinite loop.
+		 */
+		if (entry->length == 0) {
+			pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, proc->id);
+			return -EINVAL;
+		}
 
-	while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
-	       table_end) {
 		if (max_entries && count >= max_entries)
 			break;
 
@@ -523,17 +531,8 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
 		if (i != proc_num)
 			count++;
 
-		/*
-		 * If entry->length is 0, break from this loop to avoid
-		 * infinite loop.
-		 */
-		if (entry->length == 0) {
-			pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, proc->id);
-			return -EINVAL;
-		}
-
-		entry = (struct acpi_subtable_header *)
-		    ((unsigned long)entry + entry->length);
+		entry = (struct acpi_subtable_header *)entry_end;
+		entry_end += entry->length;
 	}
 
 	if (max_entries && count > max_entries) {
-- 
1.9.1

[PATCH v3 2/2] ACPI / tables : remove unused table_end parameter to acpi_tbl_entry_handler

[Sudeep Holla]

acpi_tbl_entiry_handler expects `table_end` pointer to be passed so that
the individual subtable entry handler can use it to validate the entries.

However, acpi_parse_entries now validates the end of an entry against
the table end using the length in the sub-table entry. The only user
of that argument namely MADT is also removed. So we can now eliminate
the need to pass the table end to the handlers.

This patch removes the unused second parameter in acpi_tbl_entry_handler
and updates all the handlers accordingly.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/kernel/smp.c     |  3 +--
 arch/ia64/kernel/acpi.c     | 22 +++++++---------------
 arch/x86/kernel/acpi/boot.c | 29 +++++++++--------------------
 drivers/acpi/numa.c         |  9 +++------
 drivers/acpi/scan.c         |  5 ++---
 drivers/acpi/tables.c       |  3 +--
 drivers/irqchip/irq-gic.c   | 10 +++-------
 drivers/mailbox/pcc.c       |  3 +--
 include/linux/acpi.h        |  3 +--
 9 files changed, 28 insertions(+), 59 deletions(-)

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 66cc8c4d170c..885299b1a24b 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -445,8 +445,7 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
 }
 
 static int __init
-acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
-			     const unsigned long end)
+acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_generic_interrupt *processor;
 
diff --git a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c
index efa3f0a299e2..0549504a8f6c 100644
--- a/arch/ia64/kernel/acpi.c
+++ b/arch/ia64/kernel/acpi.c
@@ -177,8 +177,7 @@ struct acpi_table_madt *acpi_madt __initdata;
 static u8 has_8259;
 
 static int __init
-acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
-			  const unsigned long end)
+acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header)
 {
 	struct acpi_madt_local_apic_override *lapic;
 
@@ -191,8 +190,7 @@ acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
 	return 0;
 }
 
-static int __init
-acpi_parse_lsapic(struct acpi_subtable_header * header, const unsigned long end)
+static int __init acpi_parse_lsapic(struct acpi_subtable_header * header)
 {
 	struct acpi_madt_local_sapic *lsapic;
 
@@ -210,8 +208,7 @@ acpi_parse_lsapic(struct acpi_subtable_header * header, const unsigned long end)
 	return 0;
 }
 
-static int __init
-acpi_parse_lapic_nmi(struct acpi_subtable_header * header, const unsigned long end)
+static int __init acpi_parse_lapic_nmi(struct acpi_subtable_header * header)
 {
 	struct acpi_madt_local_apic_nmi *lacpi_nmi;
 
@@ -221,8 +218,7 @@ acpi_parse_lapic_nmi(struct acpi_subtable_header * header, const unsigned long e
 	return 0;
 }
 
-static int __init
-acpi_parse_iosapic(struct acpi_subtable_header * header, const unsigned long end)
+static int __init acpi_parse_iosapic(struct acpi_subtable_header * header)
 {
 	struct acpi_madt_io_sapic *iosapic;
 
@@ -234,8 +230,7 @@ acpi_parse_iosapic(struct acpi_subtable_header * header, const unsigned long end
 static unsigned int __initdata acpi_madt_rev;
 
 static int __init
-acpi_parse_plat_int_src(struct acpi_subtable_header * header,
-			const unsigned long end)
+acpi_parse_plat_int_src(struct acpi_subtable_header * header)
 {
 	struct acpi_madt_interrupt_source *plintsrc;
 	int vector;
@@ -314,9 +309,7 @@ unsigned int get_cpei_target_cpu(void)
 	return acpi_cpei_phys_cpuid;
 }
 
-static int __init
-acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
-		       const unsigned long end)
+static int __init acpi_parse_int_src_ovr(struct acpi_subtable_header * header)
 {
 	struct acpi_madt_interrupt_override *p;
 
@@ -332,8 +325,7 @@ acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
 	return 0;
 }
 
-static int __init
-acpi_parse_nmi_src(struct acpi_subtable_header * header, const unsigned long end)
+static int __init acpi_parse_nmi_src(struct acpi_subtable_header * header)
 {
 	struct acpi_madt_nmi_source *nmi_src;
 
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index e65bcea63c35..08e0da4838cd 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -185,8 +185,7 @@ static int acpi_register_lapic(int id, u8 enabled)
 	return generic_processor_info(id, ver);
 }
 
-static int __init
-acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
+static int __init acpi_parse_x2apic(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_local_x2apic *processor = NULL;
 	int apic_id;
@@ -217,8 +216,7 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
 	return 0;
 }
 
-static int __init
-acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end)
+static int __init acpi_parse_lapic(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_local_apic *processor = NULL;
 
@@ -239,8 +237,7 @@ acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end)
 	return 0;
 }
 
-static int __init
-acpi_parse_sapic(struct acpi_subtable_header *header, const unsigned long end)
+static int __init acpi_parse_sapic(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_local_sapic *processor = NULL;
 
@@ -255,8 +252,7 @@ acpi_parse_sapic(struct acpi_subtable_header *header, const unsigned long end)
 }
 
 static int __init
-acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
-			  const unsigned long end)
+acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header)
 {
 	struct acpi_madt_local_apic_override *lapic_addr_ovr = NULL;
 
@@ -267,9 +263,7 @@ acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
 	return 0;
 }
 
-static int __init
-acpi_parse_x2apic_nmi(struct acpi_subtable_header *header,
-		      const unsigned long end)
+static int __init acpi_parse_x2apic_nmi(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_local_x2apic_nmi *x2apic_nmi = NULL;
 
@@ -283,8 +277,7 @@ acpi_parse_x2apic_nmi(struct acpi_subtable_header *header,
 	return 0;
 }
 
-static int __init
-acpi_parse_lapic_nmi(struct acpi_subtable_header * header, const unsigned long end)
+static int __init acpi_parse_lapic_nmi(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_local_apic_nmi *lapic_nmi = NULL;
 
@@ -382,8 +375,7 @@ static int mp_config_acpi_gsi(struct device *dev, u32 gsi, int trigger,
 	return 0;
 }
 
-static int __init
-acpi_parse_ioapic(struct acpi_subtable_header * header, const unsigned long end)
+static int __init acpi_parse_ioapic(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_io_apic *ioapic = NULL;
 	struct ioapic_domain_cfg cfg = {
@@ -434,9 +426,7 @@ static void __init acpi_sci_ioapic_setup(u8 bus_irq, u16 polarity, u16 trigger,
 	return;
 }
 
-static int __init
-acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
-		       const unsigned long end)
+static int __init acpi_parse_int_src_ovr(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_interrupt_override *intsrc = NULL;
 
@@ -473,8 +463,7 @@ acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
 	return 0;
 }
 
-static int __init
-acpi_parse_nmi_src(struct acpi_subtable_header * header, const unsigned long end)
+static int __init acpi_parse_nmi_src(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_nmi_source *nmi_src = NULL;
 
diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c
index 72b6e9ef0ae9..ecaaa3087c86 100644
--- a/drivers/acpi/numa.c
+++ b/drivers/acpi/numa.c
@@ -240,8 +240,7 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 
 
 static int __init
-acpi_parse_x2apic_affinity(struct acpi_subtable_header *header,
-			   const unsigned long end)
+acpi_parse_x2apic_affinity(struct acpi_subtable_header *header)
 {
 	struct acpi_srat_x2apic_cpu_affinity *processor_affinity;
 
@@ -258,8 +257,7 @@ acpi_parse_x2apic_affinity(struct acpi_subtable_header *header,
 }
 
 static int __init
-acpi_parse_processor_affinity(struct acpi_subtable_header *header,
-			      const unsigned long end)
+acpi_parse_processor_affinity(struct acpi_subtable_header *header)
 {
 	struct acpi_srat_cpu_affinity *processor_affinity;
 
@@ -278,8 +276,7 @@ acpi_parse_processor_affinity(struct acpi_subtable_header *header,
 static int __initdata parsed_numa_memblks;
 
 static int __init
-acpi_parse_memory_affinity(struct acpi_subtable_header * header,
-			   const unsigned long end)
+acpi_parse_memory_affinity(struct acpi_subtable_header * header)
 {
 	struct acpi_srat_mem_affinity *memory_affinity;
 
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index daf9fc8329e6..0054450dfbf7 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1918,11 +1918,10 @@ static struct acpi_probe_entry *ape;
 static int acpi_probe_count;
 static DEFINE_SPINLOCK(acpi_probe_lock);
 
-static int __init acpi_match_madt(struct acpi_subtable_header *header,
-				  const unsigned long end)
+static int __init acpi_match_madt(struct acpi_subtable_header *header)
 {
 	if (!ape->subtable_valid || ape->subtable_valid(header, ape))
-		if (!ape->probe_subtbl(header, end))
+		if (!ape->probe_subtbl(header))
 			acpi_probe_count++;
 
 	return 0;
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 24b867e26191..088b90707080 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -521,8 +521,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
 		for (i = 0; i < proc_num; i++) {
 			if (entry->type != proc[i].id)
 				continue;
-			if (!proc[i].handler ||
-			     proc[i].handler(entry, table_end))
+			if (!proc[i].handler || proc[i].handler(entry))
 				return -EINVAL;
 
 			proc->count++;
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 67b7b48d9715..1fe267b0f7c1 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1196,9 +1196,7 @@ IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init);
 #ifdef CONFIG_ACPI
 static phys_addr_t cpu_phy_base __initdata;
 
-static int __init
-gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
-			const unsigned long end)
+static int __init gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_generic_interrupt *processor;
 	phys_addr_t gic_cpu_base;
@@ -1220,8 +1218,7 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
 }
 
 /* The things you have to do to just *count* something... */
-static int __init acpi_dummy_func(struct acpi_subtable_header *header,
-				  const unsigned long end)
+static int __init acpi_dummy_func(struct acpi_subtable_header *header)
 {
 	return 0;
 }
@@ -1246,8 +1243,7 @@ static bool __init gic_validate_dist(struct acpi_subtable_header *header,
 #define ACPI_GICV2_DIST_MEM_SIZE	(SZ_4K)
 #define ACPI_GIC_CPU_IF_MEM_SIZE	(SZ_8K)
 
-static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
-				   const unsigned long end)
+static int __init gic_v2_acpi_init(struct acpi_subtable_header *header)
 {
 	struct acpi_madt_generic_distributor *dist;
 	void __iomem *cpu_base, *dist_base;
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 68885a82e704..3df1f792728b 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -212,8 +212,7 @@ static const struct mbox_chan_ops pcc_chan_ops = {
  *
  * This gets called for each entry in the PCC table.
  */
-static int parse_pcc_subspace(struct acpi_subtable_header *header,
-		const unsigned long end)
+static int parse_pcc_subspace(struct acpi_subtable_header *header)
 {
 	struct acpi_pcct_hw_reduced *pcct_ss;
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index dd5bd228e523..360c09b72314 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -116,8 +116,7 @@ enum acpi_address_range_id {
 
 typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table);
 
-typedef int (*acpi_tbl_entry_handler)(struct acpi_subtable_header *header,
-				      const unsigned long end);
+typedef int (*acpi_tbl_entry_handler)(struct acpi_subtable_header *header);
 
 #ifdef CONFIG_ACPI_INITRD_TABLE_OVERRIDE
 void acpi_initrd_override(void *data, size_t size);
-- 
1.9.1

Re: [PATCH v3 1/2] ACPI / tables: simplify acpi_parse_entries

[Sudeep Holla]

Hi Rafael,

On 01/10/15 16:11, Sudeep Holla wrote:
> acpi_parse_entries passes the table end pointer to the sub-table entry
> handler. acpi_parse_entries itself could validate the end of an entry
> against the table end using the length in the sub-table entry.
>
> This patch adds the validation of the sub-table entry end using the
> length field.This will help to eliminate the need to pass the table end
> to the handlers.
>
> It also moves the check for zero length entry early so that execution of
> the handler can be avoided.
>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>

Now that Al's MADT is queued, can you consider applying these ?

-- 
Regards,
Sudeep

Re: [PATCH v3 1/2] ACPI / tables: simplify acpi_parse_entries

[Al Stone]

On 10/15/2015 09:44 AM, Sudeep Holla wrote:
> Hi Rafael,
> 
> On 01/10/15 16:11, Sudeep Holla wrote:
>> acpi_parse_entries passes the table end pointer to the sub-table entry
>> handler. acpi_parse_entries itself could validate the end of an entry
>> against the table end using the length in the sub-table entry.
>>
>> This patch adds the validation of the sub-table entry end using the
>> length field.This will help to eliminate the need to pass the table end
>> to the handlers.
>>
>> It also moves the check for zero length entry early so that execution of
>> the handler can be avoided.
>>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> 
> Now that Al's MADT is queued, can you consider applying these ?
> 

I think we have to remove my patches from the queue until I can make
them arch-specific; doing these MADT checks breaks far too many existing
x86 systems where the firmware does things it should not; re-reading some
of the ia64 kernel code, there's a pathological case there where it could
break (but shouldn't if iasl is being used to compile tables).  I'll be
working on the new version today.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------

Re: [PATCH v3 1/2] ACPI / tables: simplify acpi_parse_entries

[Rafael J. Wysocki]

On Thursday, October 15, 2015 09:57:38 AM Al Stone wrote:
> On 10/15/2015 09:44 AM, Sudeep Holla wrote:
> > Hi Rafael,
> > 
> > On 01/10/15 16:11, Sudeep Holla wrote:
> >> acpi_parse_entries passes the table end pointer to the sub-table entry
> >> handler. acpi_parse_entries itself could validate the end of an entry
> >> against the table end using the length in the sub-table entry.
> >>
> >> This patch adds the validation of the sub-table entry end using the
> >> length field.This will help to eliminate the need to pass the table end
> >> to the handlers.
> >>
> >> It also moves the check for zero length entry early so that execution of
> >> the handler can be avoided.
> >>
> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > 
> > Now that Al's MADT is queued, can you consider applying these ?
> > 
> 
> I think we have to remove my patches from the queue until I can make
> them arch-specific; doing these MADT checks breaks far too many existing
> x86 systems where the firmware does things it should not;

Right.  I've dropped the series already.

> re-reading some of the ia64 kernel code, there's a pathological case there
> where it could break (but shouldn't if iasl is being used to compile tables).

I wouldn't count on iasl being used.

> I'll be working on the new version today.

Thanks!

Rafael