| Message ID | 1616831193-17920-5-git-send-email-tanxiaofei@huawei.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | acpi: fix some coding style issues | expand |
From: Xiaofei Tan > Sent: 27 March 2021 07:46 > > Replace __attribute__((packed)) by __packed following the > advice of checkpatch.pl. > > Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> > --- > drivers/acpi/acpi_fpdt.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/acpi_fpdt.c b/drivers/acpi/acpi_fpdt.c > index a89a806..690a88a 100644 > --- a/drivers/acpi/acpi_fpdt.c > +++ b/drivers/acpi/acpi_fpdt.c > @@ -53,7 +53,7 @@ struct resume_performance_record { > u32 resume_count; > u64 resume_prev; > u64 resume_avg; > -} __attribute__((packed)); > +} __packed; > > struct boot_performance_record { > struct fpdt_record_header header; > @@ -63,13 +63,13 @@ struct boot_performance_record { > u64 bootloader_launch; > u64 exitbootservice_start; > u64 exitbootservice_end; > -} __attribute__((packed)); > +} __packed; > > struct suspend_performance_record { > struct fpdt_record_header header; > u64 suspend_start; > u64 suspend_end; > -} __attribute__((packed)); > +} __packed; My standard question about 'packed' is whether it is actually needed. It should only be used if the structures might be misaligned in memory. If the only problem is that a 64bit item needs to be 32bit aligned then a suitable type should be used for those specific fields. Those all look very dubious - the standard header isn't packed so everything must eb assumed to be at least 32bit aligned. There are also other sub-structures that contain 64bit values. These don't contain padding - but that requires 64bit alignement. The only problematic structure is the last one - which would have a 32bit pad after the header. Is this even right given than there are explicit alignment pads in some of the other structures. If 64bit alignment isn't guaranteed then a '64bit aligned to 32bit' type should be used for the u64 fields. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi David, On 2021/3/29 18:09, David Laight wrote: > From: Xiaofei Tan >> Sent: 27 March 2021 07:46 >> >> Replace __attribute__((packed)) by __packed following the >> advice of checkpatch.pl. >> >> Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> >> --- >> drivers/acpi/acpi_fpdt.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/acpi/acpi_fpdt.c b/drivers/acpi/acpi_fpdt.c >> index a89a806..690a88a 100644 >> --- a/drivers/acpi/acpi_fpdt.c >> +++ b/drivers/acpi/acpi_fpdt.c >> @@ -53,7 +53,7 @@ struct resume_performance_record { >> u32 resume_count; >> u64 resume_prev; >> u64 resume_avg; >> -} __attribute__((packed)); >> +} __packed; >> >> struct boot_performance_record { >> struct fpdt_record_header header; >> @@ -63,13 +63,13 @@ struct boot_performance_record { >> u64 bootloader_launch; >> u64 exitbootservice_start; >> u64 exitbootservice_end; >> -} __attribute__((packed)); >> +} __packed; >> >> struct suspend_performance_record { >> struct fpdt_record_header header; >> u64 suspend_start; >> u64 suspend_end; >> -} __attribute__((packed)); >> +} __packed; > > My standard question about 'packed' is whether it is actually needed. > It should only be used if the structures might be misaligned in memory. > If the only problem is that a 64bit item needs to be 32bit aligned > then a suitable type should be used for those specific fields. > > Those all look very dubious - the standard header isn't packed > so everything must eb assumed to be at least 32bit aligned. > > There are also other sub-structures that contain 64bit values. > These don't contain padding - but that requires 64bit alignement. > > The only problematic structure is the last one - which would have > a 32bit pad after the header. > Is this even right given than there are explicit alignment pads > in some of the other structures. > > If 64bit alignment isn't guaranteed then a '64bit aligned to 32bit' > type should be used for the u64 fields. > Yes, some of them has been aligned already, then nothing changed when add this "packed ". Maybe the purpose of the original author is for extension, and can tell others that this struct need be packed. > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) > > > . >
On Tue, 2021-03-30 at 10:23 +0800, Xiaofei Tan wrote: > Hi David, > > On 2021/3/29 18:09, David Laight wrote: > > From: Xiaofei Tan > > > Sent: 27 March 2021 07:46 > > > > > > Replace __attribute__((packed)) by __packed following the > > > advice of checkpatch.pl. > > > > > > Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> > > > --- > > > drivers/acpi/acpi_fpdt.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/acpi/acpi_fpdt.c b/drivers/acpi/acpi_fpdt.c > > > index a89a806..690a88a 100644 > > > --- a/drivers/acpi/acpi_fpdt.c > > > +++ b/drivers/acpi/acpi_fpdt.c > > > @@ -53,7 +53,7 @@ struct resume_performance_record { > > > u32 resume_count; > > > u64 resume_prev; > > > u64 resume_avg; > > > -} __attribute__((packed)); > > > +} __packed; > > > > > > struct boot_performance_record { > > > struct fpdt_record_header header; > > > @@ -63,13 +63,13 @@ struct boot_performance_record { > > > u64 bootloader_launch; > > > u64 exitbootservice_start; > > > u64 exitbootservice_end; > > > -} __attribute__((packed)); > > > +} __packed; > > > > > > struct suspend_performance_record { > > > struct fpdt_record_header header; > > > u64 suspend_start; > > > u64 suspend_end; > > > -} __attribute__((packed)); > > > +} __packed; > > > > My standard question about 'packed' is whether it is actually > > needed. > > It should only be used if the structures might be misaligned in > > memory. > > If the only problem is that a 64bit item needs to be 32bit aligned > > then a suitable type should be used for those specific fields. > > > > Those all look very dubious - the standard header isn't packed > > so everything must eb assumed to be at least 32bit aligned. > > > > There are also other sub-structures that contain 64bit values. > > These don't contain padding - but that requires 64bit alignement. > > > > The only problematic structure is the last one - which would have > > a 32bit pad after the header. > > Is this even right given than there are explicit alignment pads > > in some of the other structures. > > > > If 64bit alignment isn't guaranteed then a '64bit aligned to 32bit' > > type should be used for the u64 fields. > > > > Yes, some of them has been aligned already, then nothing changed > when > add this "packed ". Maybe the purpose of the original author is for > extension, and can tell others that this struct need be packed. > The patch is upstreamed recently but it was made long time ago. I think the original problem is that one of the address, probably the suspend_performance record, is not 64bit aligned, thus we can not read the proper content of suspend_start and suspend_end, mapped from physical memory. I will try to find a machine to reproduce the problem with all __attribute__((packed)) removed to double confirm this. thanks, rui > > David > > > > - > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton > > Keynes, MK1 1PT, UK > > Registration No: 1397386 (Wales) > > > > > > . > > > >
On Tue, 2021-03-30 at 15:31 +0800, Zhang Rui wrote: > On Tue, 2021-03-30 at 10:23 +0800, Xiaofei Tan wrote: > > Hi David, > > > > On 2021/3/29 18:09, David Laight wrote: > > > From: Xiaofei Tan > > > > Sent: 27 March 2021 07:46 > > > > > > > > Replace __attribute__((packed)) by __packed following the > > > > advice of checkpatch.pl. > > > > > > > > Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> > > > > --- > > > > drivers/acpi/acpi_fpdt.c | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/acpi/acpi_fpdt.c > > > > b/drivers/acpi/acpi_fpdt.c > > > > index a89a806..690a88a 100644 > > > > --- a/drivers/acpi/acpi_fpdt.c > > > > +++ b/drivers/acpi/acpi_fpdt.c > > > > @@ -53,7 +53,7 @@ struct resume_performance_record { > > > > u32 resume_count; > > > > u64 resume_prev; > > > > u64 resume_avg; > > > > -} __attribute__((packed)); > > > > +} __packed; > > > > > > > > struct boot_performance_record { > > > > struct fpdt_record_header header; > > > > @@ -63,13 +63,13 @@ struct boot_performance_record { > > > > u64 bootloader_launch; > > > > u64 exitbootservice_start; > > > > u64 exitbootservice_end; > > > > -} __attribute__((packed)); > > > > +} __packed; > > > > > > > > struct suspend_performance_record { > > > > struct fpdt_record_header header; > > > > u64 suspend_start; > > > > u64 suspend_end; > > > > -} __attribute__((packed)); > > > > +} __packed; > > > > > > My standard question about 'packed' is whether it is actually > > > needed. > > > It should only be used if the structures might be misaligned in > > > memory. > > > If the only problem is that a 64bit item needs to be 32bit > > > aligned > > > then a suitable type should be used for those specific fields. > > > > > > Those all look very dubious - the standard header isn't packed > > > so everything must eb assumed to be at least 32bit aligned. > > > > > > There are also other sub-structures that contain 64bit values. > > > These don't contain padding - but that requires 64bit alignement. > > > > > > The only problematic structure is the last one - which would have > > > a 32bit pad after the header. > > > Is this even right given than there are explicit alignment pads > > > in some of the other structures. > > > > > > If 64bit alignment isn't guaranteed then a '64bit aligned to > > > 32bit' > > > type should be used for the u64 fields. > > > > > > > Yes, some of them has been aligned already, then nothing changed > > when > > add this "packed ". Maybe the purpose of the original author is > > for > > extension, and can tell others that this struct need be packed. > > > > The patch is upstreamed recently but it was made long time ago. > I think the original problem is that one of the address, probably the > suspend_performance record, is not 64bit aligned, thus we can not > read > the proper content of suspend_start and suspend_end, mapped from > physical memory. > > I will try to find a machine to reproduce the problem with all > __attribute__((packed)) removed to double confirm this. > So here is the problem, without __attribute__((packed)) [ 0.858442] suspend_record: 0xffffaad500175020 /sys/firmware/acpi/fpdt/suspend/suspend_end_ns:addr: 0xffffaad500175030, 15998179292659843072 /sys/firmware/acpi/fpdt/suspend/suspend_start_ns:addr: 0xffffaad500175028, 0 suspend_record is mapped to 0xffffaad500175020, and it is combined with one 32bit header and two 64bit fields (suspend_start and suspend_end), this is how it is located in physical memory. So the addresses of the two 64bit fields are actually not 64bit aligned. David, Is this the "a 64bit item needs to be 32bit aligned" problem you referred? If yes, what is the proper fix? should I used two 32bits for each of the field instead? thanks, rui > thanks, > rui > > > David > > > > > > - > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton > > > Keynes, MK1 1PT, UK > > > Registration No: 1397386 (Wales) > > > > > > > > > . > > > > > > > > >
From: Zhang Rui > Sent: 30 March 2021 09:00 > To: Xiaofei Tan <tanxiaofei@huawei.com>; David Laight <David.Laight@ACULAB.COM>; rjw@rjwysocki.net; > lenb@kernel.org; bhelgaas@google.com > Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; > linuxarm@openeuler.org > Subject: Re: [PATCH v2 04/15] ACPI: table: replace __attribute__((packed)) by __packed > > On Tue, 2021-03-30 at 15:31 +0800, Zhang Rui wrote: > > On Tue, 2021-03-30 at 10:23 +0800, Xiaofei Tan wrote: > > > Hi David, > > > > > > On 2021/3/29 18:09, David Laight wrote: > > > > From: Xiaofei Tan > > > > > Sent: 27 March 2021 07:46 > > > > > > > > > > Replace __attribute__((packed)) by __packed following the > > > > > advice of checkpatch.pl. > > > > > > > > > > Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> > > > > > --- > > > > > drivers/acpi/acpi_fpdt.c | 6 +++--- > > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/acpi/acpi_fpdt.c > > > > > b/drivers/acpi/acpi_fpdt.c > > > > > index a89a806..690a88a 100644 > > > > > --- a/drivers/acpi/acpi_fpdt.c > > > > > +++ b/drivers/acpi/acpi_fpdt.c > > > > > @@ -53,7 +53,7 @@ struct resume_performance_record { > > > > > u32 resume_count; > > > > > u64 resume_prev; > > > > > u64 resume_avg; > > > > > -} __attribute__((packed)); > > > > > +} __packed; > > > > > > > > > > struct boot_performance_record { > > > > > struct fpdt_record_header header; > > > > > @@ -63,13 +63,13 @@ struct boot_performance_record { > > > > > u64 bootloader_launch; > > > > > u64 exitbootservice_start; > > > > > u64 exitbootservice_end; > > > > > -} __attribute__((packed)); > > > > > +} __packed; > > > > > > > > > > struct suspend_performance_record { > > > > > struct fpdt_record_header header; > > > > > u64 suspend_start; > > > > > u64 suspend_end; > > > > > -} __attribute__((packed)); > > > > > +} __packed; > > > > > > > > My standard question about 'packed' is whether it is actually > > > > needed. > > > > It should only be used if the structures might be misaligned in > > > > memory. > > > > If the only problem is that a 64bit item needs to be 32bit > > > > aligned > > > > then a suitable type should be used for those specific fields. > > > > > > > > Those all look very dubious - the standard header isn't packed > > > > so everything must eb assumed to be at least 32bit aligned. > > > > > > > > There are also other sub-structures that contain 64bit values. > > > > These don't contain padding - but that requires 64bit alignement. > > > > > > > > The only problematic structure is the last one - which would have > > > > a 32bit pad after the header. > > > > Is this even right given than there are explicit alignment pads > > > > in some of the other structures. > > > > > > > > If 64bit alignment isn't guaranteed then a '64bit aligned to > > > > 32bit' > > > > type should be used for the u64 fields. > > > > > > > > > > Yes, some of them has been aligned already, then nothing changed > > > when > > > add this "packed ". Maybe the purpose of the original author is > > > for > > > extension, and can tell others that this struct need be packed. > > > > > > > The patch is upstreamed recently but it was made long time ago. > > I think the original problem is that one of the address, probably the > > suspend_performance record, is not 64bit aligned, thus we can not > > read > > the proper content of suspend_start and suspend_end, mapped from > > physical memory. > > > > I will try to find a machine to reproduce the problem with all > > __attribute__((packed)) removed to double confirm this. > > > > So here is the problem, without __attribute__((packed)) > > [ 0.858442] suspend_record: 0xffffaad500175020 > /sys/firmware/acpi/fpdt/suspend/suspend_end_ns:addr: > 0xffffaad500175030, 15998179292659843072 > /sys/firmware/acpi/fpdt/suspend/suspend_start_ns:addr: > 0xffffaad500175028, 0 > > suspend_record is mapped to 0xffffaad500175020, and it is combined with > one 32bit header and two 64bit fields (suspend_start and suspend_end), > this is how it is located in physical memory. > So the addresses of the two 64bit fields are actually not 64bit > aligned. > > David, > Is this the "a 64bit item needs to be 32bit aligned" problem you > referred? > If yes, what is the proper fix? should I used two 32bits for each of > the field instead? Define something like: typedef u64 __attribute__((aligned(4))) u64_align32; and then use it for the 64bit structure members. There doesn't seem to be a standard type name for it - although it is used in several places. I'm not entirely sure but is ACPI always LE? (is it even x86 only??) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Mar 30, 2021 at 10:15 AM David Laight <David.Laight@aculab.com> wrote: > > From: Zhang Rui > > Sent: 30 March 2021 09:00 > > To: Xiaofei Tan <tanxiaofei@huawei.com>; David Laight <David.Laight@ACULAB.COM>; rjw@rjwysocki.net; > > lenb@kernel.org; bhelgaas@google.com > > Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; > > linuxarm@openeuler.org > > Subject: Re: [PATCH v2 04/15] ACPI: table: replace __attribute__((packed)) by __packed > > > > On Tue, 2021-03-30 at 15:31 +0800, Zhang Rui wrote: > > > On Tue, 2021-03-30 at 10:23 +0800, Xiaofei Tan wrote: > > > > Hi David, > > > > > > > > On 2021/3/29 18:09, David Laight wrote: > > > > > From: Xiaofei Tan > > > > > > Sent: 27 March 2021 07:46 > > > > > > > > > > > > Replace __attribute__((packed)) by __packed following the > > > > > > advice of checkpatch.pl. > > > > > > > > > > > > Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> > > > > > > --- > > > > > > drivers/acpi/acpi_fpdt.c | 6 +++--- > > > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/drivers/acpi/acpi_fpdt.c > > > > > > b/drivers/acpi/acpi_fpdt.c > > > > > > index a89a806..690a88a 100644 > > > > > > --- a/drivers/acpi/acpi_fpdt.c > > > > > > +++ b/drivers/acpi/acpi_fpdt.c > > > > > > @@ -53,7 +53,7 @@ struct resume_performance_record { > > > > > > u32 resume_count; > > > > > > u64 resume_prev; > > > > > > u64 resume_avg; > > > > > > -} __attribute__((packed)); > > > > > > +} __packed; > > > > > > > > > > > > struct boot_performance_record { > > > > > > struct fpdt_record_header header; > > > > > > @@ -63,13 +63,13 @@ struct boot_performance_record { > > > > > > u64 bootloader_launch; > > > > > > u64 exitbootservice_start; > > > > > > u64 exitbootservice_end; > > > > > > -} __attribute__((packed)); > > > > > > +} __packed; > > > > > > > > > > > > struct suspend_performance_record { > > > > > > struct fpdt_record_header header; > > > > > > u64 suspend_start; > > > > > > u64 suspend_end; > > > > > > -} __attribute__((packed)); > > > > > > +} __packed; > > > > > > > > > > My standard question about 'packed' is whether it is actually > > > > > needed. > > > > > It should only be used if the structures might be misaligned in > > > > > memory. > > > > > If the only problem is that a 64bit item needs to be 32bit > > > > > aligned > > > > > then a suitable type should be used for those specific fields. > > > > > > > > > > Those all look very dubious - the standard header isn't packed > > > > > so everything must eb assumed to be at least 32bit aligned. > > > > > > > > > > There are also other sub-structures that contain 64bit values. > > > > > These don't contain padding - but that requires 64bit alignement. > > > > > > > > > > The only problematic structure is the last one - which would have > > > > > a 32bit pad after the header. > > > > > Is this even right given than there are explicit alignment pads > > > > > in some of the other structures. > > > > > > > > > > If 64bit alignment isn't guaranteed then a '64bit aligned to > > > > > 32bit' > > > > > type should be used for the u64 fields. > > > > > > > > > > > > > Yes, some of them has been aligned already, then nothing changed > > > > when > > > > add this "packed ". Maybe the purpose of the original author is > > > > for > > > > extension, and can tell others that this struct need be packed. > > > > > > > > > > The patch is upstreamed recently but it was made long time ago. > > > I think the original problem is that one of the address, probably the > > > suspend_performance record, is not 64bit aligned, thus we can not > > > read > > > the proper content of suspend_start and suspend_end, mapped from > > > physical memory. > > > > > > I will try to find a machine to reproduce the problem with all > > > __attribute__((packed)) removed to double confirm this. > > > > > > > So here is the problem, without __attribute__((packed)) > > > > [ 0.858442] suspend_record: 0xffffaad500175020 > > /sys/firmware/acpi/fpdt/suspend/suspend_end_ns:addr: > > 0xffffaad500175030, 15998179292659843072 > > /sys/firmware/acpi/fpdt/suspend/suspend_start_ns:addr: > > 0xffffaad500175028, 0 > > > > suspend_record is mapped to 0xffffaad500175020, and it is combined with > > one 32bit header and two 64bit fields (suspend_start and suspend_end), > > this is how it is located in physical memory. > > So the addresses of the two 64bit fields are actually not 64bit > > aligned. > > > > David, > > Is this the "a 64bit item needs to be 32bit aligned" problem you > > referred? > > If yes, what is the proper fix? should I used two 32bits for each of > > the field instead? > > Define something like: > typedef u64 __attribute__((aligned(4))) u64_align32; > and then use it for the 64bit structure members. > > There doesn't seem to be a standard type name for it - although > it is used in several places. > > I'm not entirely sure but is ACPI always LE? Yes. > (is it even x86 only??) No.
On Tue, 2021-03-30 at 08:14 +0000, David Laight wrote: > From: Zhang Rui > > Sent: 30 March 2021 09:00 > > To: Xiaofei Tan <tanxiaofei@huawei.com>; David Laight < > > David.Laight@ACULAB.COM>; rjw@rjwysocki.net; > > lenb@kernel.org; bhelgaas@google.com > > Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; > > linux-pci@vger.kernel.org; > > linuxarm@openeuler.org > > Subject: Re: [PATCH v2 04/15] ACPI: table: replace > > __attribute__((packed)) by __packed > > > > On Tue, 2021-03-30 at 15:31 +0800, Zhang Rui wrote: > > > On Tue, 2021-03-30 at 10:23 +0800, Xiaofei Tan wrote: > > > > Hi David, > > > > > > > > On 2021/3/29 18:09, David Laight wrote: > > > > > From: Xiaofei Tan > > > > > > Sent: 27 March 2021 07:46 > > > > > > > > > > > > Replace __attribute__((packed)) by __packed following the > > > > > > advice of checkpatch.pl. > > > > > > > > > > > > Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> > > > > > > --- > > > > > > drivers/acpi/acpi_fpdt.c | 6 +++--- > > > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/drivers/acpi/acpi_fpdt.c > > > > > > b/drivers/acpi/acpi_fpdt.c > > > > > > index a89a806..690a88a 100644 > > > > > > --- a/drivers/acpi/acpi_fpdt.c > > > > > > +++ b/drivers/acpi/acpi_fpdt.c > > > > > > @@ -53,7 +53,7 @@ struct resume_performance_record { > > > > > > u32 resume_count; > > > > > > u64 resume_prev; > > > > > > u64 resume_avg; > > > > > > -} __attribute__((packed)); > > > > > > +} __packed; > > > > > > > > > > > > struct boot_performance_record { > > > > > > struct fpdt_record_header header; > > > > > > @@ -63,13 +63,13 @@ struct boot_performance_record { > > > > > > u64 bootloader_launch; > > > > > > u64 exitbootservice_start; > > > > > > u64 exitbootservice_end; > > > > > > -} __attribute__((packed)); > > > > > > +} __packed; > > > > > > > > > > > > struct suspend_performance_record { > > > > > > struct fpdt_record_header header; > > > > > > u64 suspend_start; > > > > > > u64 suspend_end; > > > > > > -} __attribute__((packed)); > > > > > > +} __packed; > > > > > > > > > > My standard question about 'packed' is whether it is actually > > > > > needed. > > > > > It should only be used if the structures might be misaligned > > > > > in > > > > > memory. > > > > > If the only problem is that a 64bit item needs to be 32bit > > > > > aligned > > > > > then a suitable type should be used for those specific > > > > > fields. > > > > > > > > > > Those all look very dubious - the standard header isn't > > > > > packed > > > > > so everything must eb assumed to be at least 32bit aligned. > > > > > > > > > > There are also other sub-structures that contain 64bit > > > > > values. > > > > > These don't contain padding - but that requires 64bit > > > > > alignement. > > > > > > > > > > The only problematic structure is the last one - which would > > > > > have > > > > > a 32bit pad after the header. > > > > > Is this even right given than there are explicit alignment > > > > > pads > > > > > in some of the other structures. > > > > > > > > > > If 64bit alignment isn't guaranteed then a '64bit aligned to > > > > > 32bit' > > > > > type should be used for the u64 fields. > > > > > > > > > > > > > Yes, some of them has been aligned already, then nothing > > > > changed > > > > when > > > > add this "packed ". Maybe the purpose of the original author is > > > > for > > > > extension, and can tell others that this struct need be packed. > > > > > > > > > > The patch is upstreamed recently but it was made long time ago. > > > I think the original problem is that one of the address, probably > > > the > > > suspend_performance record, is not 64bit aligned, thus we can not > > > read > > > the proper content of suspend_start and suspend_end, mapped from > > > physical memory. > > > > > > I will try to find a machine to reproduce the problem with all > > > __attribute__((packed)) removed to double confirm this. > > > > > > > So here is the problem, without __attribute__((packed)) > > > > [ 0.858442] suspend_record: 0xffffaad500175020 > > /sys/firmware/acpi/fpdt/suspend/suspend_end_ns:addr: > > 0xffffaad500175030, 15998179292659843072 > > /sys/firmware/acpi/fpdt/suspend/suspend_start_ns:addr: > > 0xffffaad500175028, 0 > > > > suspend_record is mapped to 0xffffaad500175020, and it is combined > > with > > one 32bit header and two 64bit fields (suspend_start and > > suspend_end), > > this is how it is located in physical memory. > > So the addresses of the two 64bit fields are actually not 64bit > > aligned. > > > > David, > > Is this the "a 64bit item needs to be 32bit aligned" problem you > > referred? > > If yes, what is the proper fix? should I used two 32bits for each > > of > > the field instead? > > Define something like: > typedef u64 __attribute__((aligned(4))) u64_align32; > and then use it for the 64bit structure members. > Hi, David, Please kindly help check if the following patch is the right fix or not. I've verified it to work on my test box. The reason I use this typedef for all the u64 items because there is no guarantee that the suspend_performance record is in the end of the memory, thus it may pollute the others. From e18c942855e2f51e814d057fff4dd951cd0d0907 Mon Sep 17 00:00:00 2001 From: Zhang Rui <rui.zhang@intel.com> Date: Wed, 31 Mar 2021 20:34:13 +0800 Subject: [PATCH] ACPI: tables: FPDT: Fix 64bit alignment issue Some of the 64bit items in FPDT table may be 32bit aligned. Using __attribute__((packed)) is not needed in this case, fixing it by allowing 32bit alignment for these 64bit items. Signed-off-by: Zhang Rui <rui.zhang@intel.com> --- drivers/acpi/acpi_fpdt.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/drivers/acpi/acpi_fpdt.c b/drivers/acpi/acpi_fpdt.c index a89a806a7a2a..94e107b9a114 100644 --- a/drivers/acpi/acpi_fpdt.c +++ b/drivers/acpi/acpi_fpdt.c @@ -23,12 +23,14 @@ enum fpdt_subtable_type { SUBTABLE_S3PT, }; +typedef u64 __attribute__((aligned(4))) u64_align32; + struct fpdt_subtable_entry { u16 type; /* refer to enum fpdt_subtable_type */ u8 length; u8 revision; u32 reserved; - u64 address; /* physical address of the S3PT/FBPT table */ + u64_align32 address; /* physical address of the S3PT/FBPT table */ }; struct fpdt_subtable_header { @@ -51,25 +53,25 @@ struct fpdt_record_header { struct resume_performance_record { struct fpdt_record_header header; u32 resume_count; - u64 resume_prev; - u64 resume_avg; -} __attribute__((packed)); + u64_align32 resume_prev; + u64_align32 resume_avg; +}; struct boot_performance_record { struct fpdt_record_header header; u32 reserved; - u64 firmware_start; - u64 bootloader_load; - u64 bootloader_launch; - u64 exitbootservice_start; - u64 exitbootservice_end; -} __attribute__((packed)); + u64_align32 firmware_start; + u64_align32 bootloader_load; + u64_align32 bootloader_launch; + u64_align32 exitbootservice_start; + u64_align32 exitbootservice_end; +}; struct suspend_performance_record { struct fpdt_record_header header; - u64 suspend_start; - u64 suspend_end; -} __attribute__((packed)); + u64_align32 suspend_start; + u64_align32 suspend_end; +}; static struct resume_performance_record *record_resume;
From: Zhang Rui > Sent: 31 March 2021 16:55 > On Tue, 2021-03-30 at 08:14 +0000, David Laight wrote: > > From: Zhang Rui > > > Sent: 30 March 2021 09:00 > > > > On Tue, 2021-03-30 at 10:23 +0800, Xiaofei Tan wrote: > > > > > Hi David, > > > > > > > > > > On 2021/3/29 18:09, David Laight wrote: > > > > > > From: Xiaofei Tan > > > > > > > Sent: 27 March 2021 07:46 > > > > > > > > > > > > > > Replace __attribute__((packed)) by __packed following the > > > > > > > advice of checkpatch.pl. > > > > > > > > > > > > > > Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> > > > > > > > --- > > > > > > > drivers/acpi/acpi_fpdt.c | 6 +++--- > > > > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/acpi/acpi_fpdt.c > > > > > > > b/drivers/acpi/acpi_fpdt.c > > > > > > > index a89a806..690a88a 100644 > > > > > > > --- a/drivers/acpi/acpi_fpdt.c > > > > > > > +++ b/drivers/acpi/acpi_fpdt.c > > > > > > > @@ -53,7 +53,7 @@ struct resume_performance_record { > > > > > > > u32 resume_count; > > > > > > > u64 resume_prev; > > > > > > > u64 resume_avg; > > > > > > > -} __attribute__((packed)); > > > > > > > +} __packed; > > > > > > > > > > > > > > struct boot_performance_record { > > > > > > > struct fpdt_record_header header; > > > > > > > @@ -63,13 +63,13 @@ struct boot_performance_record { > > > > > > > u64 bootloader_launch; > > > > > > > u64 exitbootservice_start; > > > > > > > u64 exitbootservice_end; > > > > > > > -} __attribute__((packed)); > > > > > > > +} __packed; > > > > > > > > > > > > > > struct suspend_performance_record { > > > > > > > struct fpdt_record_header header; > > > > > > > u64 suspend_start; > > > > > > > u64 suspend_end; > > > > > > > -} __attribute__((packed)); > > > > > > > +} __packed; > > > > > > > > > > > > My standard question about 'packed' is whether it is actually > > > > > > needed. > > > > > > It should only be used if the structures might be misaligned > > > > > > in > > > > > > memory. > > > > > > If the only problem is that a 64bit item needs to be 32bit > > > > > > aligned > > > > > > then a suitable type should be used for those specific > > > > > > fields. > > > > > > > > > > > > Those all look very dubious - the standard header isn't > > > > > > packed > > > > > > so everything must eb assumed to be at least 32bit aligned. > > > > > > > > > > > > There are also other sub-structures that contain 64bit > > > > > > values. > > > > > > These don't contain padding - but that requires 64bit > > > > > > alignement. > > > > > > > > > > > > The only problematic structure is the last one - which would > > > > > > have > > > > > > a 32bit pad after the header. > > > > > > Is this even right given than there are explicit alignment > > > > > > pads > > > > > > in some of the other structures. > > > > > > > > > > > > If 64bit alignment isn't guaranteed then a '64bit aligned to > > > > > > 32bit' > > > > > > type should be used for the u64 fields. > > > > > > > > > > > > > > > > Yes, some of them has been aligned already, then nothing > > > > > changed > > > > > when > > > > > add this "packed ". Maybe the purpose of the original author is > > > > > for > > > > > extension, and can tell others that this struct need be packed. > > > > > > > > > > > > > The patch is upstreamed recently but it was made long time ago. > > > > I think the original problem is that one of the address, probably > > > > the > > > > suspend_performance record, is not 64bit aligned, thus we can not > > > > read > > > > the proper content of suspend_start and suspend_end, mapped from > > > > physical memory. > > > > > > > > I will try to find a machine to reproduce the problem with all > > > > __attribute__((packed)) removed to double confirm this. > > > > > > > > > > So here is the problem, without __attribute__((packed)) > > > > > > [ 0.858442] suspend_record: 0xffffaad500175020 > > > /sys/firmware/acpi/fpdt/suspend/suspend_end_ns:addr: > > > 0xffffaad500175030, 15998179292659843072 > > > /sys/firmware/acpi/fpdt/suspend/suspend_start_ns:addr: > > > 0xffffaad500175028, 0 > > > > > > suspend_record is mapped to 0xffffaad500175020, and it is combined > > > with > > > one 32bit header and two 64bit fields (suspend_start and > > > suspend_end), > > > this is how it is located in physical memory. > > > So the addresses of the two 64bit fields are actually not 64bit > > > aligned. > > > > > > David, > > > Is this the "a 64bit item needs to be 32bit aligned" problem you > > > referred? > > > If yes, what is the proper fix? should I used two 32bits for each > > > of > > > the field instead? > > > > Define something like: > > typedef u64 __attribute__((aligned(4))) u64_align32; > > and then use it for the 64bit structure members. > > > Hi, David, > > Please kindly help check if the following patch is the right fix or > not. I've verified it to work on my test box. > > The reason I use this typedef for all the u64 items because there is no > guarantee that the suspend_performance record is in the end of the > memory, thus it may pollute the others. Looks about right. David > > From e18c942855e2f51e814d057fff4dd951cd0d0907 Mon Sep 17 00:00:00 2001 > From: Zhang Rui <rui.zhang@intel.com> > Date: Wed, 31 Mar 2021 20:34:13 +0800 > Subject: [PATCH] ACPI: tables: FPDT: Fix 64bit alignment issue > > Some of the 64bit items in FPDT table may be 32bit aligned. > Using __attribute__((packed)) is not needed in this case, fixing it by > allowing 32bit alignment for these 64bit items. > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > drivers/acpi/acpi_fpdt.c | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) > > diff --git a/drivers/acpi/acpi_fpdt.c b/drivers/acpi/acpi_fpdt.c > index a89a806a7a2a..94e107b9a114 100644 > --- a/drivers/acpi/acpi_fpdt.c > +++ b/drivers/acpi/acpi_fpdt.c > @@ -23,12 +23,14 @@ enum fpdt_subtable_type { > SUBTABLE_S3PT, > }; > > +typedef u64 __attribute__((aligned(4))) u64_align32; > + > struct fpdt_subtable_entry { > u16 type; /* refer to enum fpdt_subtable_type */ > u8 length; > u8 revision; > u32 reserved; > - u64 address; /* physical address of the S3PT/FBPT table */ > + u64_align32 address; /* physical address of the S3PT/FBPT table */ > }; > > struct fpdt_subtable_header { > @@ -51,25 +53,25 @@ struct fpdt_record_header { > struct resume_performance_record { > struct fpdt_record_header header; > u32 resume_count; > - u64 resume_prev; > - u64 resume_avg; > -} __attribute__((packed)); > + u64_align32 resume_prev; > + u64_align32 resume_avg; > +}; > > struct boot_performance_record { > struct fpdt_record_header header; > u32 reserved; > - u64 firmware_start; > - u64 bootloader_load; > - u64 bootloader_launch; > - u64 exitbootservice_start; > - u64 exitbootservice_end; > -} __attribute__((packed)); > + u64_align32 firmware_start; > + u64_align32 bootloader_load; > + u64_align32 bootloader_launch; > + u64_align32 exitbootservice_start; > + u64_align32 exitbootservice_end; > +}; > > struct suspend_performance_record { > struct fpdt_record_header header; > - u64 suspend_start; > - u64 suspend_end; > -} __attribute__((packed)); > + u64_align32 suspend_start; > + u64_align32 suspend_end; > +}; > > > static struct resume_performance_record *record_resume; > -- > 2.17.1 > - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Mar 31, 2021 at 11:55:08PM +0800, Zhang Rui wrote: > ... > From e18c942855e2f51e814d057fff4dd951cd0d0907 Mon Sep 17 00:00:00 2001 > From: Zhang Rui <rui.zhang@intel.com> > Date: Wed, 31 Mar 2021 20:34:13 +0800 > Subject: [PATCH] ACPI: tables: FPDT: Fix 64bit alignment issue > > Some of the 64bit items in FPDT table may be 32bit aligned. > Using __attribute__((packed)) is not needed in this case, fixing it by > allowing 32bit alignment for these 64bit items. 1) Can you please add a spec reference for this? I think it's ACPI v6.3, sec 5.2.23.5, or something close to that. 2) The exact layout in memory is prescribed by the spec. I think that's basically what "packed" accomplishes. I don't understand why using "aligned" would be preferable. Using "aligned" means things can be at different offsets depending on the starting address of the structure. We always want the identical layout, no matter what the starting address is. > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > drivers/acpi/acpi_fpdt.c | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) > > diff --git a/drivers/acpi/acpi_fpdt.c b/drivers/acpi/acpi_fpdt.c > index a89a806a7a2a..94e107b9a114 100644 > --- a/drivers/acpi/acpi_fpdt.c > +++ b/drivers/acpi/acpi_fpdt.c > @@ -23,12 +23,14 @@ enum fpdt_subtable_type { > SUBTABLE_S3PT, > }; > > +typedef u64 __attribute__((aligned(4))) u64_align32; > + > struct fpdt_subtable_entry { > u16 type; /* refer to enum fpdt_subtable_type */ > u8 length; > u8 revision; > u32 reserved; > - u64 address; /* physical address of the S3PT/FBPT table */ > + u64_align32 address; /* physical address of the S3PT/FBPT table */ > }; > > struct fpdt_subtable_header { > @@ -51,25 +53,25 @@ struct fpdt_record_header { > struct resume_performance_record { > struct fpdt_record_header header; > u32 resume_count; > - u64 resume_prev; > - u64 resume_avg; > -} __attribute__((packed)); > + u64_align32 resume_prev; > + u64_align32 resume_avg; > +}; > > struct boot_performance_record { > struct fpdt_record_header header; > u32 reserved; > - u64 firmware_start; > - u64 bootloader_load; > - u64 bootloader_launch; > - u64 exitbootservice_start; > - u64 exitbootservice_end; > -} __attribute__((packed)); > + u64_align32 firmware_start; > + u64_align32 bootloader_load; > + u64_align32 bootloader_launch; > + u64_align32 exitbootservice_start; > + u64_align32 exitbootservice_end; > +}; > > struct suspend_performance_record { > struct fpdt_record_header header; > - u64 suspend_start; > - u64 suspend_end; > -} __attribute__((packed)); > + u64_align32 suspend_start; > + u64_align32 suspend_end; > +}; > > > static struct resume_performance_record *record_resume; > -- > 2.17.1 > >
From: Bjorn Helgaas > Sent: 31 March 2021 18:22 > > On Wed, Mar 31, 2021 at 11:55:08PM +0800, Zhang Rui wrote: > > ... > > > From e18c942855e2f51e814d057fff4dd951cd0d0907 Mon Sep 17 00:00:00 2001 > > From: Zhang Rui <rui.zhang@intel.com> > > Date: Wed, 31 Mar 2021 20:34:13 +0800 > > Subject: [PATCH] ACPI: tables: FPDT: Fix 64bit alignment issue > > > > Some of the 64bit items in FPDT table may be 32bit aligned. > > Using __attribute__((packed)) is not needed in this case, fixing it by > > allowing 32bit alignment for these 64bit items. > > 1) Can you please add a spec reference for this? I think it's ACPI > v6.3, sec 5.2.23.5, or something close to that. > > 2) The exact layout in memory is prescribed by the spec. I think > that's basically what "packed" accomplishes. I don't understand > why using "aligned" would be preferable. Using "aligned" means > things can be at different offsets depending on the starting > address of the structure. We always want the identical layout, no > matter what the starting address is. Both 'packed' and 'aligned(4)' remove any structure alignment padding before 64bit items that aren't on an 8 byte boundary. (Because everything else in the structures is naturally aligned.) The difference is significant on cpu that don't support misaligned addresses. Assuming that the structure is always on a 4n byte boundary (which the ACPI spec probably requires) accesses to the 32-bit fields are always ok. It is only 64-bit fields that must be accessed as two 32-bit memory cycles, not all the fields using multiple single byte cycles. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu, Apr 1, 2021 at 11:00 AM David Laight <David.Laight@aculab.com> wrote: > > From: Bjorn Helgaas > > Sent: 31 March 2021 18:22 > > > > On Wed, Mar 31, 2021 at 11:55:08PM +0800, Zhang Rui wrote: > > > ... > > > > > From e18c942855e2f51e814d057fff4dd951cd0d0907 Mon Sep 17 00:00:00 2001 > > > From: Zhang Rui <rui.zhang@intel.com> > > > Date: Wed, 31 Mar 2021 20:34:13 +0800 > > > Subject: [PATCH] ACPI: tables: FPDT: Fix 64bit alignment issue > > > > > > Some of the 64bit items in FPDT table may be 32bit aligned. > > > Using __attribute__((packed)) is not needed in this case, fixing it by > > > allowing 32bit alignment for these 64bit items. > > > > 1) Can you please add a spec reference for this? I think it's ACPI > > v6.3, sec 5.2.23.5, or something close to that. > > > > 2) The exact layout in memory is prescribed by the spec. I think > > that's basically what "packed" accomplishes. I don't understand > > why using "aligned" would be preferable. Using "aligned" means > > things can be at different offsets depending on the starting > > address of the structure. We always want the identical layout, no > > matter what the starting address is. > > Both 'packed' and 'aligned(4)' remove any structure alignment > padding before 64bit items that aren't on an 8 byte boundary. > (Because everything else in the structures is naturally aligned.) > > The difference is significant on cpu that don't support misaligned > addresses. > Assuming that the structure is always on a 4n byte boundary > (which the ACPI spec probably requires) accesses to the 32-bit > fields are always ok. > It is only 64-bit fields that must be accessed as two 32-bit > memory cycles, not all the fields using multiple single byte > cycles. So what exactly is wrong with using "packed"? It is way easier to understand for a casual reader of the code.
From: Rafael J. Wysocki > Sent: 01 April 2021 14:50 ... > So what exactly is wrong with using "packed"? It is way easier to > understand for a casual reader of the code. Because it is usually wrong! If I have: struct foo { u64 val; } __packed; And then have: u64 bar(struct foo *foo) { return foo->val; } The on some cpu the compiler has to generate the equivalent of: u8 *x = (void *)&foo->val; return x[0] | x[1] << 8 | x[2] << 16 | x[3] << 24 | x[4] << 32 | x[5] << 40 | x[6] << 48 | x[7] << 56; If you can guarantee that the structure is 32bit aligned then it can generate the simpler: u32 *x = (void *)&foo->val; return x[0] | x[1] << 32; (Yes I've missed out the 64-bit casts) This is why you should almost never use __packed. There are historic structures with 64 bit items on 4 byte boundaries (and 32 bit values on 2 byte boundaries). Typically most of the fields are shorter so can be read directly (although they might need a byte-swapping load). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu, Apr 1, 2021 at 4:23 PM David Laight <David.Laight@aculab.com> wrote: > > From: Rafael J. Wysocki > > Sent: 01 April 2021 14:50 > ... > > So what exactly is wrong with using "packed"? It is way easier to > > understand for a casual reader of the code. > > Because it is usually wrong! > > If I have: > struct foo { > u64 val; > } __packed; > > And then have: > u64 bar(struct foo *foo) > { > return foo->val; > } > > The on some cpu the compiler has to generate the equivalent of: > u8 *x = (void *)&foo->val; > return x[0] | x[1] << 8 | x[2] << 16 | x[3] << 24 | x[4] << 32 | x[5] << 40 | x[6] << 48 | x[7] << 56; > > If you can guarantee that the structure is 32bit aligned > then it can generate the simpler: > u32 *x = (void *)&foo->val; > return x[0] | x[1] << 32; > > (Yes I've missed out the 64-bit casts) > > This is why you should almost never use __packed. > > There are historic structures with 64 bit items on 4 byte boundaries > (and 32 bit values on 2 byte boundaries). > Typically most of the fields are shorter so can be read directly > (although they might need a byte-swapping load). The possible overhead impact is clear to me, but I really don't like the "local" typedef idea. It at least would need to be accompanied by a comment explaining why it is there and why using it is better than using __packed and why this needs to be defined locally and not in some generic header file. Also, the FPDT code is just one function that parses the entire table and there is no object passing between functions in it etc, so is __packed still problematic in there?
diff --git a/drivers/acpi/acpi_fpdt.c b/drivers/acpi/acpi_fpdt.c index a89a806..690a88a 100644 --- a/drivers/acpi/acpi_fpdt.c +++ b/drivers/acpi/acpi_fpdt.c @@ -53,7 +53,7 @@ struct resume_performance_record { u32 resume_count; u64 resume_prev; u64 resume_avg; -} __attribute__((packed)); +} __packed; struct boot_performance_record { struct fpdt_record_header header; @@ -63,13 +63,13 @@ struct boot_performance_record { u64 bootloader_launch; u64 exitbootservice_start; u64 exitbootservice_end; -} __attribute__((packed)); +} __packed; struct suspend_performance_record { struct fpdt_record_header header; u64 suspend_start; u64 suspend_end; -} __attribute__((packed)); +} __packed; static struct resume_performance_record *record_resume;
Replace __attribute__((packed)) by __packed following the advice of checkpatch.pl. Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> --- drivers/acpi/acpi_fpdt.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)