diff mbox

[RFC,05/19] ACPI: Provide union for address_space64 and ext_address_space64

Message ID 1420684386-5975-6-git-send-email-jiang.liu@linux.intel.com
State Changes Requested
Headers show

Commit Message

Jiang Liu Jan. 8, 2015, 2:32 a.m. UTC 
From: Thomas Gleixner <tglx@linutronix.de>

address_space64 and ext_address_space64 share substracts just at
different offsets. To unify the parsing functions implement the two
structs as unions of their substructs, so we can extract the shared
data.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 include/acpi/acrestyp.h |   49 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 13 deletions(-)

Comments

Rafael J. Wysocki Jan. 21, 2015, 12:32 a.m. UTC | #1 
On Thursday, January 08, 2015 10:32:52 AM Jiang Liu wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> address_space64 and ext_address_space64 share substracts just at
> different offsets. To unify the parsing functions implement the two
> structs as unions of their substructs, so we can extract the shared
> data.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>

This is ACPICA code, so if we change this, either we'll diverge from the
upstream (which is highly undesirable), or we'll have to change it for
the majority of OSes on the planet (all except Windows).

Lv, Bob, David, any comments here?

> ---
>  include/acpi/acrestyp.h |   49 ++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 36 insertions(+), 13 deletions(-)
> 
> diff --git a/include/acpi/acrestyp.h b/include/acpi/acrestyp.h
> index eb760ca0b2e0..307d5b2605c8 100644
> --- a/include/acpi/acrestyp.h
> +++ b/include/acpi/acrestyp.h
> @@ -326,23 +326,46 @@ struct acpi_resource_address32 {
>  	struct acpi_resource_source resource_source;
>  };
>  
> -struct acpi_resource_address64 {
> -	ACPI_RESOURCE_ADDRESS_COMMON u64 granularity;
> -	u64 minimum;
> -	u64 maximum;
> -	u64 translation_offset;
> +#define ACPI_RESOURCE_ADDRESS64_COMMON \
> +	u64 granularity;	       \
> +	u64 minimum;		       \
> +	u64 maximum;		       \
> +	u64 translation_offset;	       \
>  	u64 address_length;
> -	struct acpi_resource_source resource_source;
> +
> +struct acpi_resource_address64_common {
> +ACPI_RESOURCE_ADDRESS64_COMMON};
> +
> +struct acpi_resource_address64 {
> +	union {
> +		struct {
> +			ACPI_RESOURCE_ADDRESS_COMMON
> +			ACPI_RESOURCE_ADDRESS64_COMMON
> +			struct acpi_resource_source resource_source;
> +		};
> +		struct {
> +			struct acpi_resource_address base;
> +			struct acpi_resource_address64_common addr;
> +			struct acpi_resource_source resource_source;
> +		} common;
> +	};
>  };
>  
>  struct acpi_resource_extended_address64 {
> -	ACPI_RESOURCE_ADDRESS_COMMON u8 revision_ID;
> -	u64 granularity;
> -	u64 minimum;
> -	u64 maximum;
> -	u64 translation_offset;
> -	u64 address_length;
> -	u64 type_specific;
> +	union {
> +		struct {
> +			ACPI_RESOURCE_ADDRESS_COMMON
> +			u8 revision_ID;
> +			ACPI_RESOURCE_ADDRESS64_COMMON
> +			u64 type_specific;
> +		};
> +		struct {
> +			struct acpi_resource_address base;
> +			u8 revision_ID;
> +			struct acpi_resource_address64_common addr;
> +			u64 type_specific;
> +		} common;
> +	};
>  };
>  
>  struct acpi_resource_extended_irq {
>
Lv Zheng Jan. 22, 2015, 2:32 a.m. UTC | #2 
Hi, Thomas and Jiang

> From: Jiang Liu [mailto:jiang.liu@linux.intel.com]
> Sent: Thursday, January 08, 2015 10:33 AM
> 
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> address_space64 and ext_address_space64 share substracts just at
> different offsets. To unify the parsing functions implement the two
> structs as unions of their substructs, so we can extract the shared
> data.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
>  include/acpi/acrestyp.h |   49 ++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 36 insertions(+), 13 deletions(-)
> 
> diff --git a/include/acpi/acrestyp.h b/include/acpi/acrestyp.h
> index eb760ca0b2e0..307d5b2605c8 100644
> --- a/include/acpi/acrestyp.h
> +++ b/include/acpi/acrestyp.h
> @@ -326,23 +326,46 @@ struct acpi_resource_address32 {
>  	struct acpi_resource_source resource_source;
>  };
> 
> -struct acpi_resource_address64 {
> -	ACPI_RESOURCE_ADDRESS_COMMON u64 granularity;
> -	u64 minimum;
> -	u64 maximum;
> -	u64 translation_offset;
> +#define ACPI_RESOURCE_ADDRESS64_COMMON \
> +	u64 granularity;	       \
> +	u64 minimum;		       \
> +	u64 maximum;		       \
> +	u64 translation_offset;	       \
>  	u64 address_length;
> -	struct acpi_resource_source resource_source;
> +
> +struct acpi_resource_address64_common {
> +ACPI_RESOURCE_ADDRESS64_COMMON};
> +
> +struct acpi_resource_address64 {
> +	union {
> +		struct {
> +			ACPI_RESOURCE_ADDRESS_COMMON
> +			ACPI_RESOURCE_ADDRESS64_COMMON
> +			struct acpi_resource_source resource_source;
> +		};

This looks wrong to ACPICA upstream.

> +		struct {
> +			struct acpi_resource_address base;
> +			struct acpi_resource_address64_common addr;
> +			struct acpi_resource_source resource_source;
> +		} common;
> +	};
>  };

And this.
Though anonymous structs/unions are now C11 standard, I still didn't see it used in the ACPICA upstream.
It could be a problem if someone still compiles ACPICA using old compilers.

> 
>  struct acpi_resource_extended_address64 {
> -	ACPI_RESOURCE_ADDRESS_COMMON u8 revision_ID;
> -	u64 granularity;
> -	u64 minimum;
> -	u64 maximum;
> -	u64 translation_offset;
> -	u64 address_length;
> -	u64 type_specific;
> +	union {
> +		struct {
> +			ACPI_RESOURCE_ADDRESS_COMMON
> +			u8 revision_ID;
> +			ACPI_RESOURCE_ADDRESS64_COMMON
> +			u64 type_specific;
> +		};

Ditto.

> +		struct {
> +			struct acpi_resource_address base;
> +			u8 revision_ID;
> +			struct acpi_resource_address64_common addr;
> +			u64 type_specific;
> +		} common;
> +	};

Ditto.

I think what you want is the ability to access common.addr and common.base from different resource address64 types.
So we can achieve this directly in the ACPICA upstream without using the union.

I tried this in the ACPICA upstream and the result is:
https://github.com/zetalog/acpica/commit/0f4ed510
Let me send its linuxized version after this email.

Thanks and best regards
-Lv

>  };
> 
>  struct acpi_resource_extended_irq {
> --
> 1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiang Liu Jan. 22, 2015, 2:57 a.m. UTC | #3 
On 2015/1/22 10:32, Zheng, Lv wrote:
> Hi, Thomas and Jiang
> 
>> From: Jiang Liu [mailto:jiang.liu@linux.intel.com]
>> Sent: Thursday, January 08, 2015 10:33 AM
>>
>> From: Thomas Gleixner <tglx@linutronix.de>
>>
>> address_space64 and ext_address_space64 share substracts just at
>> different offsets. To unify the parsing functions implement the two
>> structs as unions of their substructs, so we can extract the shared
>> data.
>>
>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>> ---
>>  include/acpi/acrestyp.h |   49 ++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 36 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/acpi/acrestyp.h b/include/acpi/acrestyp.h
>> index eb760ca0b2e0..307d5b2605c8 100644
>> --- a/include/acpi/acrestyp.h
>> +++ b/include/acpi/acrestyp.h
>> @@ -326,23 +326,46 @@ struct acpi_resource_address32 {
>>  	struct acpi_resource_source resource_source;
>>  };
>>
>> -struct acpi_resource_address64 {
>> -	ACPI_RESOURCE_ADDRESS_COMMON u64 granularity;
>> -	u64 minimum;
>> -	u64 maximum;
>> -	u64 translation_offset;
>> +#define ACPI_RESOURCE_ADDRESS64_COMMON \
>> +	u64 granularity;	       \
>> +	u64 minimum;		       \
>> +	u64 maximum;		       \
>> +	u64 translation_offset;	       \
>>  	u64 address_length;
>> -	struct acpi_resource_source resource_source;
>> +
>> +struct acpi_resource_address64_common {
>> +ACPI_RESOURCE_ADDRESS64_COMMON};
>> +
>> +struct acpi_resource_address64 {
>> +	union {
>> +		struct {
>> +			ACPI_RESOURCE_ADDRESS_COMMON
>> +			ACPI_RESOURCE_ADDRESS64_COMMON
>> +			struct acpi_resource_source resource_source;
>> +		};
> 
> This looks wrong to ACPICA upstream.
> 
>> +		struct {
>> +			struct acpi_resource_address base;
>> +			struct acpi_resource_address64_common addr;
>> +			struct acpi_resource_source resource_source;
>> +		} common;
>> +	};
>>  };
> 
> And this.
> Though anonymous structs/unions are now C11 standard, I still didn't see it used in the ACPICA upstream.
> It could be a problem if someone still compiles ACPICA using old compilers.
> 
>>
>>  struct acpi_resource_extended_address64 {
>> -	ACPI_RESOURCE_ADDRESS_COMMON u8 revision_ID;
>> -	u64 granularity;
>> -	u64 minimum;
>> -	u64 maximum;
>> -	u64 translation_offset;
>> -	u64 address_length;
>> -	u64 type_specific;
>> +	union {
>> +		struct {
>> +			ACPI_RESOURCE_ADDRESS_COMMON
>> +			u8 revision_ID;
>> +			ACPI_RESOURCE_ADDRESS64_COMMON
>> +			u64 type_specific;
>> +		};
> 
> Ditto.
> 
>> +		struct {
>> +			struct acpi_resource_address base;
>> +			u8 revision_ID;
>> +			struct acpi_resource_address64_common addr;
>> +			u64 type_specific;
>> +		} common;
>> +	};
> 
> Ditto.
> 
> I think what you want is the ability to access common.addr and common.base from different resource address64 types.
> So we can achieve this directly in the ACPICA upstream without using the union.
> 
> I tried this in the ACPICA upstream and the result is:
> https://github.com/zetalog/acpica/commit/0f4ed510
> Let me send its linuxized version after this email.
Hi Lv,
	Thanks for your great support:)
	What's the normal process to propagate this patch into
linux kernel? Or how long will it take? We have another hotplug
patch set which depends on this patch set, then depends on this
ACPICA core change.
Regards!
Gerry
> 
> Thanks and best regards
> -Lv
> 
>>  };
>>
>>  struct acpi_resource_extended_irq {
>> --
>> 1.7.10.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lv Zheng Jan. 22, 2015, 3:24 a.m. UTC | #4 
Hi, Gerry

> From: Jiang Liu [mailto:jiang.liu@linux.intel.com]
> Sent: Thursday, January 22, 2015 10:58 AM
> 
> On 2015/1/22 10:32, Zheng, Lv wrote:
> > Hi, Thomas and Jiang
> >
> >> From: Jiang Liu [mailto:jiang.liu@linux.intel.com]
> >> Sent: Thursday, January 08, 2015 10:33 AM
> >>
> >> From: Thomas Gleixner <tglx@linutronix.de>
> >>
> >> address_space64 and ext_address_space64 share substracts just at
> >> different offsets. To unify the parsing functions implement the two
> >> structs as unions of their substructs, so we can extract the shared
> >> data.
> >>
> >> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> >> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> >> ---
> >>  include/acpi/acrestyp.h |   49 ++++++++++++++++++++++++++++++++++-------------
> >>  1 file changed, 36 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/include/acpi/acrestyp.h b/include/acpi/acrestyp.h
> >> index eb760ca0b2e0..307d5b2605c8 100644
> >> --- a/include/acpi/acrestyp.h
> >> +++ b/include/acpi/acrestyp.h
> >> @@ -326,23 +326,46 @@ struct acpi_resource_address32 {
> >>  	struct acpi_resource_source resource_source;
> >>  };
> >>
> >> -struct acpi_resource_address64 {
> >> -	ACPI_RESOURCE_ADDRESS_COMMON u64 granularity;
> >> -	u64 minimum;
> >> -	u64 maximum;
> >> -	u64 translation_offset;
> >> +#define ACPI_RESOURCE_ADDRESS64_COMMON \
> >> +	u64 granularity;	       \
> >> +	u64 minimum;		       \
> >> +	u64 maximum;		       \
> >> +	u64 translation_offset;	       \
> >>  	u64 address_length;
> >> -	struct acpi_resource_source resource_source;
> >> +
> >> +struct acpi_resource_address64_common {
> >> +ACPI_RESOURCE_ADDRESS64_COMMON};
> >> +
> >> +struct acpi_resource_address64 {
> >> +	union {
> >> +		struct {
> >> +			ACPI_RESOURCE_ADDRESS_COMMON
> >> +			ACPI_RESOURCE_ADDRESS64_COMMON
> >> +			struct acpi_resource_source resource_source;
> >> +		};
> >
> > This looks wrong to ACPICA upstream.
> >
> >> +		struct {
> >> +			struct acpi_resource_address base;
> >> +			struct acpi_resource_address64_common addr;
> >> +			struct acpi_resource_source resource_source;
> >> +		} common;
> >> +	};
> >>  };
> >
> > And this.
> > Though anonymous structs/unions are now C11 standard, I still didn't see it used in the ACPICA upstream.
> > It could be a problem if someone still compiles ACPICA using old compilers.
> >
> >>
> >>  struct acpi_resource_extended_address64 {
> >> -	ACPI_RESOURCE_ADDRESS_COMMON u8 revision_ID;
> >> -	u64 granularity;
> >> -	u64 minimum;
> >> -	u64 maximum;
> >> -	u64 translation_offset;
> >> -	u64 address_length;
> >> -	u64 type_specific;
> >> +	union {
> >> +		struct {
> >> +			ACPI_RESOURCE_ADDRESS_COMMON
> >> +			u8 revision_ID;
> >> +			ACPI_RESOURCE_ADDRESS64_COMMON
> >> +			u64 type_specific;
> >> +		};
> >
> > Ditto.
> >
> >> +		struct {
> >> +			struct acpi_resource_address base;
> >> +			u8 revision_ID;
> >> +			struct acpi_resource_address64_common addr;
> >> +			u64 type_specific;
> >> +		} common;
> >> +	};
> >
> > Ditto.
> >
> > I think what you want is the ability to access common.addr and common.base from different resource address64 types.
> > So we can achieve this directly in the ACPICA upstream without using the union.
> >
> > I tried this in the ACPICA upstream and the result is:
> > https://github.com/zetalog/acpica/commit/0f4ed510
> > Let me send its linuxized version after this email.
> Hi Lv,
> 	Thanks for your great support:)
> 	What's the normal process to propagate this patch into
> linux kernel? Or how long will it take? We have another hotplug
> patch set which depends on this patch set, then depends on this
> ACPICA core change.

There won't be divergences if things are proceeded in this way:
If we can reach an agreement on the linuxized version, then it can be merged directly by Rafael.
And I'll zap it from the ACPICA release series.

Thanks and best regards
-Lv

> Regards!
> Gerry
> >
> > Thanks and best regards
> > -Lv
> >
> >>  };
> >>
> >>  struct acpi_resource_extended_irq {
> >> --
> >> 1.7.10.4
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Gleixner Jan. 22, 2015, 10:38 a.m. UTC | #5 
On Thu, 22 Jan 2015, Zheng, Lv wrote:
> If we can reach an agreement on the linuxized version, then it can
> be merged directly by Rafael.  And I'll zap it from the ACPICA
> release series.

Fine with me.

Thanks,

	tglx

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" 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/include/acpi/acrestyp.h b/include/acpi/acrestyp.h
index eb760ca0b2e0..307d5b2605c8 100644
--- a/include/acpi/acrestyp.h
+++ b/include/acpi/acrestyp.h
@@ -326,23 +326,46 @@  struct acpi_resource_address32 {
 	struct acpi_resource_source resource_source;
 };
 
-struct acpi_resource_address64 {
-	ACPI_RESOURCE_ADDRESS_COMMON u64 granularity;
-	u64 minimum;
-	u64 maximum;
-	u64 translation_offset;
+#define ACPI_RESOURCE_ADDRESS64_COMMON \
+	u64 granularity;	       \
+	u64 minimum;		       \
+	u64 maximum;		       \
+	u64 translation_offset;	       \
 	u64 address_length;
-	struct acpi_resource_source resource_source;
+
+struct acpi_resource_address64_common {
+ACPI_RESOURCE_ADDRESS64_COMMON};
+
+struct acpi_resource_address64 {
+	union {
+		struct {
+			ACPI_RESOURCE_ADDRESS_COMMON
+			ACPI_RESOURCE_ADDRESS64_COMMON
+			struct acpi_resource_source resource_source;
+		};
+		struct {
+			struct acpi_resource_address base;
+			struct acpi_resource_address64_common addr;
+			struct acpi_resource_source resource_source;
+		} common;
+	};
 };
 
 struct acpi_resource_extended_address64 {
-	ACPI_RESOURCE_ADDRESS_COMMON u8 revision_ID;
-	u64 granularity;
-	u64 minimum;
-	u64 maximum;
-	u64 translation_offset;
-	u64 address_length;
-	u64 type_specific;
+	union {
+		struct {
+			ACPI_RESOURCE_ADDRESS_COMMON
+			u8 revision_ID;
+			ACPI_RESOURCE_ADDRESS64_COMMON
+			u64 type_specific;
+		};
+		struct {
+			struct acpi_resource_address base;
+			u8 revision_ID;
+			struct acpi_resource_address64_common addr;
+			u64 type_specific;
+		} common;
+	};
 };
 
 struct acpi_resource_extended_irq {