Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 04/15] ACPI: table: replace __attribute__((packed)) by
 __packed
To:     David Laight <David.Laight@ACULAB.COM>,
        "rjw@rjwysocki.net" <rjw@rjwysocki.net>,
        "lenb@kernel.org" <lenb@kernel.org>,
        "rui.zhang@intel.com" <rui.zhang@intel.com>,
        "bhelgaas@google.com" <bhelgaas@google.com>
References: <1616831193-17920-1-git-send-email-tanxiaofei@huawei.com>
 <1616831193-17920-5-git-send-email-tanxiaofei@huawei.com>
 <6df04be78e544e17b3b57f159312541f@AcuMS.aculab.com>
CC:     "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linuxarm@openeuler.org" <linuxarm@openeuler.org>
From:   Xiaofei Tan <tanxiaofei@huawei.com>
Message-ID: <34dd3de8-644d-6e44-965a-0991b7027cae@huawei.com>
Date:   Tue, 30 Mar 2021 10:23:16 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.7.1
MIME-Version: 1.0
In-Reply-To: <6df04be78e544e17b3b57f159312541f@AcuMS.aculab.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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)
>
>
> .
>