[PATCH v2 00/15] acpi: fix some coding style issues
Fix some coding style issues reported by checkpatch.pl. Differences from v1 to v2: - Add subsystem and module name in the name of patch 05/15. - Change to use more proper module name for some patch names. Xiaofei Tan (15): ACPI: APD: fix a block comment align issue ACPI: processor: fix some coding style issues ACPI: debug: fix some coding style issues ACPI: table: replace __attribute__((packed)) by __packed ACPI: ipmi: remove useless return statement for void function ACPI: LPSS: fix some coding style issues ACPI: memhotplug: fix a coding style issue ACPI: acpi_pad: fix a coding style issue ACPI: battery: fix some coding style issues ACPI: button: fix some coding style issues ACPI: CPPC: fix some coding style issues ACPI: custom_method: fix a coding style issue ACPI: PM: fix some coding style issues ACPI: sysfs: fix some coding style issues ACPI: dock: fix some coding style issues drivers/acpi/acpi_apd.c | 8 ++--- drivers/acpi/acpi_dbg.c | 40 +++++++++++------------- drivers/acpi/acpi_fpdt.c | 6 ++-- drivers/acpi/acpi_ipmi.c | 1 - drivers/acpi/acpi_lpss.c | 4 ++- drivers/acpi/acpi_memhotplug.c | 2 +- drivers/acpi/acpi_pad.c | 4 +++ drivers/acpi/acpi_processor.c | 18 +++-------- drivers/acpi/battery.c | 64 +++++++++++++++++++++---------------- drivers/acpi/button.c | 10 +++--- drivers/acpi/cppc_acpi.c | 71 +++++++++++++++++++++--------------------- drivers/acpi/custom_method.c | 2 +- drivers/acpi/device_pm.c | 3 ++ drivers/acpi/device_sysfs.c | 15 ++++++--- drivers/acpi/dock.c | 7 +++-- 15 files changed, 138 insertions(+), 117 deletions(-) -- 2.8.1
Fix the following coding style issue reported by checkpatch.pl. WARNING: Block comments should align the * on each line +/** +* Create platform device during acpi scan attach handle. Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> --- drivers/acpi/acpi_apd.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c index 39359ce..0ec5b3f 100644 --- a/drivers/acpi/acpi_apd.c +++ b/drivers/acpi/acpi_apd.c @@ -176,10 +176,10 @@ static const struct apd_device_desc hip08_spi_desc = { #endif -/** -* Create platform device during acpi scan attach handle. -* Return value > 0 on success of creating device. -*/ +/* + * Create platform device during acpi scan attach handle. + * Return value > 0 on success of creating device. + */ static int acpi_apd_create_device(struct acpi_device *adev, const struct acpi_device_id *id) { -- 2.8.1
Fix some coding style issues reported by checkpatch.pl, including following types: ERROR: code indent should use tabs where possible WARNING: Block comments use a trailing */ on a separate line WARNING: Missing a blank line after declarations WARNING: labels should not be indented Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> --- drivers/acpi/acpi_processor.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c index fc89f3a..2d5bd2a 100644 --- a/drivers/acpi/acpi_processor.c +++ b/drivers/acpi/acpi_processor.c @@ -25,10 +25,7 @@ DEFINE_PER_CPU(struct acpi_processor *, processors); EXPORT_PER_CPU_SYMBOL(processors); -/* -------------------------------------------------------------------------- - Errata Handling - -------------------------------------------------------------------------- */ - +/* Errata Handling */ struct acpi_processor_errata errata __read_mostly; EXPORT_SYMBOL_GPL(errata); @@ -151,10 +148,7 @@ static int acpi_processor_errata(void) return result; } -/* -------------------------------------------------------------------------- - Initialization - -------------------------------------------------------------------------- */ - +/* Initialization */ #ifdef CONFIG_ACPI_HOTPLUG_CPU int __weak acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, u32 acpi_id, int *pcpu) @@ -306,6 +300,7 @@ static int acpi_processor_get_info(struct acpi_device *device) */ if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) { int ret = acpi_processor_hotadd_init(pr); + if (ret) return ret; } @@ -431,10 +426,7 @@ static int acpi_processor_add(struct acpi_device *device, } #ifdef CONFIG_ACPI_HOTPLUG_CPU -/* -------------------------------------------------------------------------- - Removal - -------------------------------------------------------------------------- */ - +/* Removal */ static void acpi_processor_remove(struct acpi_device *device) { struct acpi_processor *pr; @@ -892,7 +884,7 @@ int acpi_processor_evaluate_cst(acpi_handle handle, u32 cpu, info->count = last_index; - end: +end: kfree(buffer.pointer); return ret; -- 2.8.1
Fix some coding style issues reported by checkpatch.pl, including following types: WARNING: space prohibited between function name and open parenthesis WARNING: else is not generally useful after a break or return Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> --- drivers/acpi/acpi_dbg.c | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/drivers/acpi/acpi_dbg.c b/drivers/acpi/acpi_dbg.c index d50261d..e641bc1 100644 --- a/drivers/acpi/acpi_dbg.c +++ b/drivers/acpi/acpi_dbg.c @@ -21,7 +21,7 @@ #include <linux/acpi.h> #include "internal.h" -#define ACPI_AML_BUF_ALIGN (sizeof (acpi_size)) +#define ACPI_AML_BUF_ALIGN (sizeof(acpi_size)) #define ACPI_AML_BUF_SIZE PAGE_SIZE #define circ_count(circ) \ @@ -613,16 +613,15 @@ static ssize_t acpi_aml_read(struct file *file, char __user *buf, if (ret == -EAGAIN) { if (file->f_flags & O_NONBLOCK) break; - else { - ret = wait_event_interruptible(acpi_aml_io.wait, - acpi_aml_user_readable()); - /* - * We need to retry when the condition - * becomes true. - */ - if (ret == 0) - goto again; - } + + ret = wait_event_interruptible(acpi_aml_io.wait, + acpi_aml_user_readable()); + /* + * We need to retry when the condition + * becomes true. + */ + if (ret == 0) + goto again; } if (ret < 0) { if (!acpi_aml_running()) @@ -683,16 +682,15 @@ static ssize_t acpi_aml_write(struct file *file, const char __user *buf, if (ret == -EAGAIN) { if (file->f_flags & O_NONBLOCK) break; - else { - ret = wait_event_interruptible(acpi_aml_io.wait, - acpi_aml_user_writable()); - /* - * We need to retry when the condition - * becomes true. - */ - if (ret == 0) - goto again; - } + + ret = wait_event_interruptible(acpi_aml_io.wait, + acpi_aml_user_writable()); + /* + * We need to retry when the condition + * becomes true. + */ + if (ret == 0) + goto again; } if (ret < 0) { if (!acpi_aml_running()) -- 2.8.1
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; static struct resume_performance_record *record_resume; -- 2.8.1
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; -- 2.17.1
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?
Remove useless return statement for void function, reported by checkpatch.pl. WARNING: void function return statements are not generally useful FILE: drivers/acpi/acpi_ipmi.c:482: + return; +} Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> --- drivers/acpi/acpi_ipmi.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index 9d6c0fc..bbd00d9 100644 --- a/drivers/acpi/acpi_ipmi.c +++ b/drivers/acpi/acpi_ipmi.c @@ -478,7 +478,6 @@ static void ipmi_register_bmc(int iface, struct device *dev) ipmi_dev_release(ipmi_device); err_ref: put_device(smi_data.dev); - return; } static void ipmi_bmc_gone(int iface) -- 2.8.1
Fix some coding style issues reported by checkpatch.pl, including following types: WARNING: simple_strtol is obsolete, use kstrtol instead WARNING: Missing a blank line after declarations Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> --- drivers/acpi/acpi_lpss.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index be73974..2df231e 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -187,7 +187,7 @@ static void byt_i2c_setup(struct lpss_private_data *pdata) /* Expected to always be true, but better safe then sorry */ if (uid_str) - uid = simple_strtol(uid_str, NULL, 10); + uid = kstrtol(uid_str, NULL, 10); /* Detect I2C bus shared with PUNIT and ignore its d3 status */ status = acpi_evaluate_integer(handle, "_SEM", NULL, &shared_host); @@ -377,6 +377,7 @@ static const struct acpi_device_id acpi_lpss_device_ids[] = { static int is_memory(struct acpi_resource *res, void *not_used) { struct resource r; + return !acpi_dev_resource_memory(res, &r); } @@ -1200,6 +1201,7 @@ static int acpi_lpss_poweroff_noirq(struct device *dev) if (pdata->dev_desc->resume_from_noirq) { /* This is analogous to the acpi_lpss_suspend_noirq() case. */ int ret = acpi_lpss_do_poweroff_late(dev); + if (ret) return ret; } -- 2.8.1
On Saturday, March 27, 2021, Xiaofei Tan <tanxiaofei@huawei.com> wrote:
Fix some coding style issues reported by checkpatch.pl, including following types:
WARNING: simple_strtol is obsolete, use kstrtol instead WARNING: Missing a blank line after declarations
First of all, two changes ==> two patches.
Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> --- drivers/acpi/acpi_lpss.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index be73974..2df231e 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -187,7 +187,7 @@ static void byt_i2c_setup(struct lpss_private_data *pdata)
/* Expected to always be true, but better safe then sorry */ if (uid_str) - uid = simple_strtol(uid_str, NULL, 10); + uid = kstrtol(uid_str, NULL, 10);
How did you test this? Is there any guarantee that input is null-terminated number? Where is the check of returned value from `kstrtol()`? Yes, you haven’t tested that at all. Don’t submit patches you are not able to test and haven’t tested. NAK.
/* Detect I2C bus shared with PUNIT and ignore its d3 status */ status = acpi_evaluate_integer(handle, "_SEM", NULL, &shared_host); @@ -377,6 +377,7 @@ static const struct acpi_device_id acpi_lpss_device_ids[] = { static int is_memory(struct acpi_resource *res, void *not_used) { struct resource r; + return !acpi_dev_resource_memory(res, &r); }
@@ -1200,6 +1201,7 @@ static int acpi_lpss_poweroff_noirq(struct device *dev) if (pdata->dev_desc->resume_from_noirq) { /* This is analogous to the acpi_lpss_suspend_noirq() case. */ int ret = acpi_lpss_do_poweroff_late(dev); + if (ret) return ret; } -- 2.8.1
-- With Best Regards, Andy Shevchenko
Hi Andy, On 2021/3/27 16:17, Andy Shevchenko wrote:
On Saturday, March 27, 2021, Xiaofei Tan <tanxiaofei@huawei.com <mailto:tanxiaofei@huawei.com>> wrote:
Fix some coding style issues reported by checkpatch.pl <http://checkpatch.pl>, including following types:
WARNING: simple_strtol is obsolete, use kstrtol instead WARNING: Missing a blank line after declarations
First of all, two changes ==> two patches.
I thought it would be better to include more simple cleanup in one patch.
Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com <mailto:tanxiaofei@huawei.com>> --- drivers/acpi/acpi_lpss.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index be73974..2df231e 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -187,7 +187,7 @@ static void byt_i2c_setup(struct lpss_private_data *pdata)
/* Expected to always be true, but better safe then sorry */ if (uid_str) - uid = simple_strtol(uid_str, NULL, 10); + uid = kstrtol(uid_str, NULL, 10);
How did you test this? Is there any guarantee that input is null-terminated number?
Where is the check of returned value from `kstrtol()`?
Yes, you haven’t tested that at all. Don’t submit patches you are not able to test and haven’t tested.
It's my fault. Sorry for that, i will fix it.
NAK.
/* Detect I2C bus shared with PUNIT and ignore its d3 status */ status = acpi_evaluate_integer(handle, "_SEM", NULL, &shared_host); @@ -377,6 +377,7 @@ static const struct acpi_device_id acpi_lpss_device_ids[] = { static int is_memory(struct acpi_resource *res, void *not_used) { struct resource r; + return !acpi_dev_resource_memory(res, &r); }
@@ -1200,6 +1201,7 @@ static int acpi_lpss_poweroff_noirq(struct device *dev) if (pdata->dev_desc->resume_from_noirq) { /* This is analogous to the acpi_lpss_suspend_noirq() case. */ int ret = acpi_lpss_do_poweroff_late(dev); + if (ret) return ret; } -- 2.8.1
-- With Best Regards, Andy Shevchenko
On Saturday, March 27, 2021, Xiaofei Tan <tanxiaofei@huawei.com> wrote:
Fix some coding style issues reported by checkpatch.pl, including following types:
WARNING: simple_strtol is obsolete, use kstrtol instead
And one more thing, the above message is bogus. Read what the comments in the code says about use cases for simple_*() vs. kstrto*() ones. Joe?
WARNING: Missing a blank line after declarations
Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> --- drivers/acpi/acpi_lpss.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index be73974..2df231e 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -187,7 +187,7 @@ static void byt_i2c_setup(struct lpss_private_data *pdata)
/* Expected to always be true, but better safe then sorry */ if (uid_str) - uid = simple_strtol(uid_str, NULL, 10); + uid = kstrtol(uid_str, NULL, 10);
/* Detect I2C bus shared with PUNIT and ignore its d3 status */ status = acpi_evaluate_integer(handle, "_SEM", NULL, &shared_host); @@ -377,6 +377,7 @@ static const struct acpi_device_id acpi_lpss_device_ids[] = { static int is_memory(struct acpi_resource *res, void *not_used) { struct resource r; + return !acpi_dev_resource_memory(res, &r); }
@@ -1200,6 +1201,7 @@ static int acpi_lpss_poweroff_noirq(struct device *dev) if (pdata->dev_desc->resume_from_noirq) { /* This is analogous to the acpi_lpss_suspend_noirq() case. */ int ret = acpi_lpss_do_poweroff_late(dev); + if (ret) return ret; } -- 2.8.1
-- With Best Regards, Andy Shevchenko
Hi Andy, On 2021/3/27 16:19, Andy Shevchenko wrote:
On Saturday, March 27, 2021, Xiaofei Tan <tanxiaofei@huawei.com <mailto:tanxiaofei@huawei.com>> wrote:
Fix some coding style issues reported by checkpatch.pl <http://checkpatch.pl>, including following types:
WARNING: simple_strtol is obsolete, use kstrtol instead
And one more thing, the above message is bogus. Read what the comments in the code says about use cases for simple_*() vs. kstrto*() ones.
OK. I would remove this modification from the patch.
Joe?
WARNING: Missing a blank line after declarations
Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com <mailto:tanxiaofei@huawei.com>> --- drivers/acpi/acpi_lpss.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index be73974..2df231e 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -187,7 +187,7 @@ static void byt_i2c_setup(struct lpss_private_data *pdata)
/* Expected to always be true, but better safe then sorry */ if (uid_str) - uid = simple_strtol(uid_str, NULL, 10); + uid = kstrtol(uid_str, NULL, 10);
/* Detect I2C bus shared with PUNIT and ignore its d3 status */ status = acpi_evaluate_integer(handle, "_SEM", NULL, &shared_host); @@ -377,6 +377,7 @@ static const struct acpi_device_id acpi_lpss_device_ids[] = { static int is_memory(struct acpi_resource *res, void *not_used) { struct resource r; + return !acpi_dev_resource_memory(res, &r); }
@@ -1200,6 +1201,7 @@ static int acpi_lpss_poweroff_noirq(struct device *dev) if (pdata->dev_desc->resume_from_noirq) { /* This is analogous to the acpi_lpss_suspend_noirq() case. */ int ret = acpi_lpss_do_poweroff_late(dev); + if (ret) return ret; } -- 2.8.1
-- With Best Regards, Andy Shevchenko
On Sat, 2021-03-27 at 10:19 +0200, Andy Shevchenko wrote:
On Saturday, March 27, 2021, Xiaofei Tan <tanxiaofei@huawei.com> wrote:
Fix some coding style issues reported by checkpatch.pl, including following types:
WARNING: simple_strtol is obsolete, use kstrtol instead
And one more thing, the above message is bogus. Read what the comments in the code says about use cases for simple_*() vs. kstrto*() ones.
Joe?
This check and message is nearly 10 years old and was appropriate for when it was implemented. kernel.h currently has: * Use these functions if and only if you cannot use kstrto<foo>, because So the message could be changed to something like: WARNING: simple_strtol should be used only when kstrtol can not be used.
On Sat, Mar 27, 2021 at 3:39 PM Joe Perches <joe@perches.com> wrote:
On Sat, 2021-03-27 at 10:19 +0200, Andy Shevchenko wrote:
On Saturday, March 27, 2021, Xiaofei Tan <tanxiaofei@huawei.com> wrote:
Fix some coding style issues reported by checkpatch.pl, including following types:
WARNING: simple_strtol is obsolete, use kstrtol instead
And one more thing, the above message is bogus. Read what the comments in the code says about use cases for simple_*() vs. kstrto*() ones.
Joe?
This check and message is nearly 10 years old and was appropriate for when it was implemented.
kernel.h currently has: * Use these functions if and only if you cannot use kstrto<foo>, because
So the message could be changed to something like:
WARNING: simple_strtol should be used only when kstrtol can not be used.
Fine with me, thanks! -- With Best Regards, Andy Shevchenko
Fix the following coding style issue reported by checkpatch.pl WARNING: __initdata should be placed after acpi_no_memhotplug FILE: drivers/acpi/acpi_memhotplug.c:326: +static bool __initdata acpi_no_memhotplug; Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> --- drivers/acpi/acpi_memhotplug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c index 8cc195c..5c5ed22 100644 --- a/drivers/acpi/acpi_memhotplug.c +++ b/drivers/acpi/acpi_memhotplug.c @@ -323,7 +323,7 @@ static void acpi_memory_device_remove(struct acpi_device *device) acpi_memory_device_free(mem_device); } -static bool __initdata acpi_no_memhotplug; +static bool acpi_no_memhotplug __initdata; void __init acpi_memory_hotplug_init(void) { -- 2.8.1
Fix the following coding style issue reported by checkpatch.pl WARNING: Missing a blank line after declarations Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> --- drivers/acpi/acpi_pad.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/acpi/acpi_pad.c b/drivers/acpi/acpi_pad.c index b84ab72..df4adeb 100644 --- a/drivers/acpi/acpi_pad.c +++ b/drivers/acpi/acpi_pad.c @@ -128,6 +128,7 @@ static void round_robin_cpu(unsigned int tsk_index) static void exit_round_robin(unsigned int tsk_index) { struct cpumask *pad_busy_cpus = to_cpumask(pad_busy_cpus_bits); + cpumask_clear_cpu(tsk_in_cpu[tsk_index], pad_busy_cpus); tsk_in_cpu[tsk_index] = -1; } @@ -265,6 +266,7 @@ static ssize_t rrtime_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { unsigned long num; + if (kstrtoul(buf, 0, &num)) return -EINVAL; if (num < 1 || num >= 100) @@ -286,6 +288,7 @@ static ssize_t idlepct_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { unsigned long num; + if (kstrtoul(buf, 0, &num)) return -EINVAL; if (num < 1 || num >= 100) @@ -307,6 +310,7 @@ static ssize_t idlecpus_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { unsigned long num; + if (kstrtoul(buf, 0, &num)) return -EINVAL; mutex_lock(&isolated_cpus_lock); -- 2.8.1
Fix some coding style issues reported by checkpatch.pl, including following types: WARNING: Block comments use * on subsequent lines WARNING: Block comments use a trailing */ on a separate line ERROR: code indent should use tabs where possible WARNING: Missing a blank line after declarations ERROR: spaces required around that '?' (ctx:WxV) WARNING: Block comments should align the * on each line Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> --- drivers/acpi/battery.c | 64 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 38 insertions(+), 26 deletions(-) diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c index b822f77..a0d8ead 100644 --- a/drivers/acpi/battery.c +++ b/drivers/acpi/battery.c @@ -74,16 +74,17 @@ enum { ACPI_BATTERY_XINFO_PRESENT, ACPI_BATTERY_QUIRK_PERCENTAGE_CAPACITY, /* On Lenovo Thinkpad models from 2010 and 2011, the power unit - switches between mWh and mAh depending on whether the system - is running on battery or not. When mAh is the unit, most - reported values are incorrect and need to be adjusted by - 10000/design_voltage. Verified on x201, t410, t410s, and x220. - Pre-2010 and 2012 models appear to always report in mWh and - are thus unaffected (tested with t42, t61, t500, x200, x300, - and x230). Also, in mid-2012 Lenovo issued a BIOS update for - the 2011 models that fixes the issue (tested on x220 with a - post-1.29 BIOS), but as of Nov. 2012, no such update is - available for the 2010 models. */ + * switches between mWh and mAh depending on whether the system + * is running on battery or not. When mAh is the unit, most + * reported values are incorrect and need to be adjusted by + * 10000/design_voltage. Verified on x201, t410, t410s, and x220. + * Pre-2010 and 2012 models appear to always report in mWh and + * are thus unaffected (tested with t42, t61, t500, x200, x300, + * and x230). Also, in mid-2012 Lenovo issued a BIOS update for + * the 2011 models that fixes the issue (tested on x220 with a + * post-1.29 BIOS), but as of Nov. 2012, no such update is + * available for the 2010 models. + */ ACPI_BATTERY_QUIRK_THINKPAD_MAH, /* for batteries reporting current capacity with design capacity * on a full charge, but showing degradation in full charge cap. @@ -372,8 +373,9 @@ static enum power_supply_property energy_battery_full_cap_broken_props[] = { }; /* -------------------------------------------------------------------------- - Battery Management - -------------------------------------------------------------------------- */ + * Battery Management + * -------------------------------------------------------------------------- + */ struct acpi_offsets { size_t offset; /* offset inside struct acpi_sbs_battery */ u8 mode; /* int or string? */ @@ -431,6 +433,7 @@ static int extract_package(struct acpi_battery *battery, { int i; union acpi_object *element; + if (package->type != ACPI_TYPE_PACKAGE) return -EFAULT; for (i = 0; i < num; ++i) { @@ -439,6 +442,7 @@ static int extract_package(struct acpi_battery *battery, element = &package->package.elements[i]; if (offsets[i].mode) { u8 *ptr = (u8 *)battery + offsets[i].offset; + if (element->type == ACPI_TYPE_STRING || element->type == ACPI_TYPE_BUFFER) strncpy(ptr, element->string.pointer, 32); @@ -497,10 +501,12 @@ static int extract_battery_info(const int use_bix, battery->design_capacity_warning * 10000 / battery->design_voltage; /* Curiously, design_capacity_low, unlike the rest of them, - is correct. */ + * is correct. + */ /* capacity_granularity_* equal 1 on the systems tested, so - it's impossible to tell if they would need an adjustment - or not if their values were higher. */ + * it's impossible to tell if they would need an adjustment + * or not if their values were higher. + */ } if (test_bit(ACPI_BATTERY_QUIRK_DEGRADED_FULL_CHARGE, &battery->flags) && battery->capacity_now > battery->full_charge_capacity) @@ -532,8 +538,8 @@ static int acpi_battery_get_info(struct acpi_battery *battery) if (ACPI_FAILURE(status)) { acpi_handle_info(battery->device->handle, "%s evaluation failed: %s\n", - use_bix ?"_BIX":"_BIF", - acpi_format_exception(status)); + use_bix ? "_BIX":"_BIF", + acpi_format_exception(status)); } else { result = extract_battery_info(use_bix, battery, @@ -648,6 +654,7 @@ static ssize_t acpi_battery_alarm_show(struct device *dev, char *buf) { struct acpi_battery *battery = to_acpi_battery(dev_get_drvdata(dev)); + return sprintf(buf, "%d\n", battery->alarm * 1000); } @@ -657,6 +664,7 @@ static ssize_t acpi_battery_alarm_store(struct device *dev, { unsigned long x; struct acpi_battery *battery = to_acpi_battery(dev_get_drvdata(dev)); + if (sscanf(buf, "%lu\n", &x) == 1) battery->alarm = x/1000; if (acpi_battery_present(battery)) @@ -743,7 +751,7 @@ EXPORT_SYMBOL_GPL(battery_hook_register); * This function gets called right after the battery sysfs * attributes have been added, so that the drivers that * define custom sysfs attributes can add their own. -*/ + */ static void battery_hook_add_battery(struct acpi_battery *battery) { struct acpi_battery_hook *hook_node, *tmp; @@ -872,10 +880,12 @@ static void find_battery(const struct dmi_header *dm, void *private) { struct acpi_battery *battery = (struct acpi_battery *)private; /* Note: the hardcoded offsets below have been extracted from - the source code of dmidecode. */ + * the source code of dmidecode. + */ if (dm->type == DMI_ENTRY_PORTABLE_BATTERY && dm->length >= 8) { const u8 *dmi_data = (const u8 *)(dm + 1); int dmi_capacity = get_unaligned((const u16 *)(dmi_data + 6)); + if (dm->length >= 18) dmi_capacity *= dmi_data[17]; if (battery->design_capacity * battery->design_voltage / 1000 @@ -917,6 +927,7 @@ static void acpi_battery_quirks(struct acpi_battery *battery) if (battery->power_unit && dmi_name_in_vendors("LENOVO")) { const char *s; + s = dmi_get_system_info(DMI_PRODUCT_VERSION); if (s && !strncasecmp(s, "ThinkPad", 8)) { dmi_walk(find_battery, battery); @@ -1014,8 +1025,9 @@ static void acpi_battery_refresh(struct acpi_battery *battery) } /* -------------------------------------------------------------------------- - Driver Interface - -------------------------------------------------------------------------- */ + * Driver Interface + * -------------------------------------------------------------------------- + */ static void acpi_battery_notify(struct acpi_device *device, u32 event) { @@ -1026,11 +1038,11 @@ static void acpi_battery_notify(struct acpi_device *device, u32 event) return; old = battery->bat; /* - * On Acer Aspire V5-573G notifications are sometimes triggered too - * early. For example, when AC is unplugged and notification is - * triggered, battery state is still reported as "Full", and changes to - * "Discharging" only after short delay, without any notification. - */ + * On Acer Aspire V5-573G notifications are sometimes triggered too + * early. For example, when AC is unplugged and notification is + * triggered, battery state is still reported as "Full", and changes to + * "Discharging" only after short delay, without any notification. + */ if (battery_notification_delay_ms > 0) msleep(battery_notification_delay_ms); if (event == ACPI_BATTERY_NOTIFY_INFO) -- 2.8.1
Fix some coding style issues reported by checkpatch.pl, including following types: WARNING: Block comments use * on subsequent lines WARNING: Block comments use a trailing */ on a separate line ERROR: code indent should use tabs where possible Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> --- drivers/acpi/button.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c index 85e5e03..c66db72 100644 --- a/drivers/acpi/button.c +++ b/drivers/acpi/button.c @@ -157,8 +157,9 @@ module_param(lid_report_interval, ulong, 0644); MODULE_PARM_DESC(lid_report_interval, "Interval (ms) between lid key events"); /* -------------------------------------------------------------------------- - FS Interface (/proc) - -------------------------------------------------------------------------- */ + * FS Interface (/proc) + * -------------------------------------------------------------------------- + */ static struct proc_dir_entry *acpi_button_dir; static struct proc_dir_entry *acpi_lid_dir; @@ -349,8 +350,9 @@ static int acpi_button_remove_fs(struct acpi_device *device) } /* -------------------------------------------------------------------------- - Driver Interface - -------------------------------------------------------------------------- */ + * Driver Interface + * -------------------------------------------------------------------------- + */ int acpi_lid_open(void) { if (!lid_device) -- 2.8.1
Fix some coding style issues reported by checkpatch.pl, including following types: WARNING: Missing a blank line after declarations WARNING: unnecessary whitespace before a quoted newline ERROR: spaces required around that '>=' ERROR: switch and case should be at the same indent Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> --- drivers/acpi/cppc_acpi.c | 71 ++++++++++++++++++++++++------------------------ 1 file changed, 36 insertions(+), 35 deletions(-) diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index ae53740..3dbaf47 100644 --- a/drivers/acpi/cppc_acpi.c +++ b/drivers/acpi/cppc_acpi.c @@ -326,6 +326,7 @@ static int send_pcc_cmd(int pcc_ss_id, u16 cmd) if (unlikely(ret)) { for_each_possible_cpu(i) { struct cpc_desc *desc = per_cpu(cpc_desc_ptr, i); + if (!desc) continue; @@ -777,7 +778,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr) cpc_ptr->cpc_regs[i-2].type = ACPI_TYPE_BUFFER; memcpy(&cpc_ptr->cpc_regs[i-2].cpc_entry.reg, gas_t, sizeof(*gas_t)); } else { - pr_debug("Err in entry:%d in CPC table of CPU:%d \n", i, pr->id); + pr_debug("Err in entry:%d in CPC table of CPU:%d\n", i, pr->id); goto out_free; } } @@ -867,7 +868,7 @@ void acpi_cppc_processor_exit(struct acpi_processor *pr) void __iomem *addr; int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, pr->id); - if (pcc_ss_id >=0 && pcc_data[pcc_ss_id]) { + if (pcc_ss_id >= 0 && pcc_data[pcc_ss_id]) { if (pcc_data[pcc_ss_id]->pcc_channel_acquired) { pcc_data[pcc_ss_id]->refcount--; if (!pcc_data[pcc_ss_id]->refcount) { @@ -954,22 +955,22 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val) val, reg->bit_width); switch (reg->bit_width) { - case 8: - *val = readb_relaxed(vaddr); - break; - case 16: - *val = readw_relaxed(vaddr); - break; - case 32: - *val = readl_relaxed(vaddr); - break; - case 64: - *val = readq_relaxed(vaddr); - break; - default: - pr_debug("Error: Cannot read %u bit width from PCC for ss: %d\n", - reg->bit_width, pcc_ss_id); - ret_val = -EFAULT; + case 8: + *val = readb_relaxed(vaddr); + break; + case 16: + *val = readw_relaxed(vaddr); + break; + case 32: + *val = readl_relaxed(vaddr); + break; + case 64: + *val = readq_relaxed(vaddr); + break; + default: + pr_debug("Error: Cannot read %u bit width from PCC for ss: %d\n", + reg->bit_width, pcc_ss_id); + ret_val = -EFAULT; } return ret_val; @@ -993,23 +994,23 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) val, reg->bit_width); switch (reg->bit_width) { - case 8: - writeb_relaxed(val, vaddr); - break; - case 16: - writew_relaxed(val, vaddr); - break; - case 32: - writel_relaxed(val, vaddr); - break; - case 64: - writeq_relaxed(val, vaddr); - break; - default: - pr_debug("Error: Cannot write %u bit width to PCC for ss: %d\n", - reg->bit_width, pcc_ss_id); - ret_val = -EFAULT; - break; + case 8: + writeb_relaxed(val, vaddr); + break; + case 16: + writew_relaxed(val, vaddr); + break; + case 32: + writel_relaxed(val, vaddr); + break; + case 64: + writeq_relaxed(val, vaddr); + break; + default: + pr_debug("Error: Cannot write %u bit width to PCC for ss: %d\n", + reg->bit_width, pcc_ss_id); + ret_val = -EFAULT; + break; } return ret_val; -- 2.8.1
Fix the following coding style issue reported by checkpatch.pl ERROR: "foo * bar" should be "foo *bar" FILE: drivers/acpi/custom_method.c:22: +static ssize_t cm_write(struct file *file, const char __user * user_buf, Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> --- drivers/acpi/custom_method.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c index 7b54dc9..443fdf62 100644 --- a/drivers/acpi/custom_method.c +++ b/drivers/acpi/custom_method.c @@ -19,7 +19,7 @@ static struct dentry *cm_dentry; /* /sys/kernel/debug/acpi/custom_method */ -static ssize_t cm_write(struct file *file, const char __user * user_buf, +static ssize_t cm_write(struct file *file, const char __user *user_buf, size_t count, loff_t *ppos) { static char *buf; -- 2.8.1
Fix the following coding style issue reported by checkpatch.pl WARNING: Missing a blank line after declarations Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> --- drivers/acpi/device_pm.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c index 0961537..16c0fe8 100644 --- a/drivers/acpi/device_pm.c +++ b/drivers/acpi/device_pm.c @@ -966,6 +966,7 @@ EXPORT_SYMBOL_GPL(acpi_dev_resume); int acpi_subsys_runtime_suspend(struct device *dev) { int ret = pm_generic_runtime_suspend(dev); + return ret ? ret : acpi_dev_suspend(dev, true); } EXPORT_SYMBOL_GPL(acpi_subsys_runtime_suspend); @@ -980,6 +981,7 @@ EXPORT_SYMBOL_GPL(acpi_subsys_runtime_suspend); int acpi_subsys_runtime_resume(struct device *dev) { int ret = acpi_dev_resume(dev); + return ret ? ret : pm_generic_runtime_resume(dev); } EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume); @@ -1171,6 +1173,7 @@ EXPORT_SYMBOL_GPL(acpi_subsys_freeze); int acpi_subsys_restore_early(struct device *dev) { int ret = acpi_dev_resume(dev); + return ret ? ret : pm_generic_restore_early(dev); } EXPORT_SYMBOL_GPL(acpi_subsys_restore_early); -- 2.8.1
Fix some coding style issues reported by checkpatch.pl, including following types: WARNING: Missing a blank line after declarations WARNING: Block comments should align the * on each line ERROR: open brace '{' following function definitions go on the next line Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> --- drivers/acpi/device_sysfs.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c index da4ff2a..a07d4ad 100644 --- a/drivers/acpi/device_sysfs.c +++ b/drivers/acpi/device_sysfs.c @@ -73,6 +73,7 @@ static const struct sysfs_ops acpi_data_node_sysfs_ops = { static void acpi_data_node_release(struct kobject *kobj) { struct acpi_data_node *dn = to_data_node(kobj); + complete(&dn->kobj_done); } @@ -130,7 +131,7 @@ static void acpi_hide_nondev_subnodes(struct acpi_device_data *data) * Return: 0: no _HID and no _CID * -EINVAL: output error * -ENOMEM: output is truncated -*/ + */ static int create_pnp_modalias(struct acpi_device *acpi_dev, char *modalias, int size) { @@ -431,7 +432,8 @@ static DEVICE_ATTR_RO(path); /* sysfs file that shows description text from the ACPI _STR method */ static ssize_t description_show(struct device *dev, struct device_attribute *attr, - char *buf) { + char *buf) +{ struct acpi_device *acpi_dev = to_acpi_device(dev); int result; @@ -456,7 +458,8 @@ static DEVICE_ATTR_RO(description); static ssize_t sun_show(struct device *dev, struct device_attribute *attr, - char *buf) { + char *buf) +{ struct acpi_device *acpi_dev = to_acpi_device(dev); acpi_status status; unsigned long long sun; @@ -471,7 +474,8 @@ static DEVICE_ATTR_RO(sun); static ssize_t hrv_show(struct device *dev, struct device_attribute *attr, - char *buf) { + char *buf) +{ struct acpi_device *acpi_dev = to_acpi_device(dev); acpi_status status; unsigned long long hrv; @@ -485,7 +489,8 @@ hrv_show(struct device *dev, struct device_attribute *attr, static DEVICE_ATTR_RO(hrv); static ssize_t status_show(struct device *dev, struct device_attribute *attr, - char *buf) { + char *buf) +{ struct acpi_device *acpi_dev = to_acpi_device(dev); acpi_status status; unsigned long long sta; -- 2.8.1
Fix some coding style issues reported by checkpatch.pl, including following types: WARNING: Missing a blank line after declarations ERROR: spaces required around that ':' WARNING: Statements should start on a tabstop Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com> --- drivers/acpi/dock.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c index 0937cea..7cf9215 100644 --- a/drivers/acpi/dock.c +++ b/drivers/acpi/dock.c @@ -271,6 +271,7 @@ static void hotplug_dock_devices(struct dock_station *ds, u32 event) if (!acpi_device_enumerated(adev)) { int ret = acpi_bus_scan(adev->handle); + if (ret) dev_dbg(&adev->dev, "scan error %d\n", -ret); } @@ -502,6 +503,7 @@ static ssize_t flags_show(struct device *dev, struct device_attribute *attr, char *buf) { struct dock_station *dock_station = dev->platform_data; + return snprintf(buf, PAGE_SIZE, "%d\n", dock_station->flags); } @@ -523,7 +525,7 @@ static ssize_t undock_store(struct device *dev, struct device_attribute *attr, begin_undock(dock_station); ret = handle_eject_request(dock_station, ACPI_NOTIFY_EJECT_REQUEST); acpi_scan_lock_release(); - return ret ? ret: count; + return ret ? ret : count; } static DEVICE_ATTR_WO(undock); @@ -535,10 +537,11 @@ static ssize_t uid_show(struct device *dev, { unsigned long long lbuf; struct dock_station *dock_station = dev->platform_data; + acpi_status status = acpi_evaluate_integer(dock_station->handle, "_UID", NULL, &lbuf); if (ACPI_FAILURE(status)) - return 0; + return 0; return snprintf(buf, PAGE_SIZE, "%llx\n", lbuf); } -- 2.8.1
On Saturday, March 27, 2021, Xiaofei Tan <tanxiaofei@huawei.com> wrote:
Fix some coding style issues reported by checkpatch.pl.
NAK until it’s proven that you have tested each change on the real system (virtual more or less okay).
Differences from v1 to v2: - Add subsystem and module name in the name of patch 05/15. - Change to use more proper module name for some patch names.
Xiaofei Tan (15): ACPI: APD: fix a block comment align issue ACPI: processor: fix some coding style issues ACPI: debug: fix some coding style issues ACPI: table: replace __attribute__((packed)) by __packed ACPI: ipmi: remove useless return statement for void function ACPI: LPSS: fix some coding style issues ACPI: memhotplug: fix a coding style issue ACPI: acpi_pad: fix a coding style issue ACPI: battery: fix some coding style issues ACPI: button: fix some coding style issues ACPI: CPPC: fix some coding style issues ACPI: custom_method: fix a coding style issue ACPI: PM: fix some coding style issues ACPI: sysfs: fix some coding style issues ACPI: dock: fix some coding style issues
drivers/acpi/acpi_apd.c | 8 ++--- drivers/acpi/acpi_dbg.c | 40 +++++++++++------------- drivers/acpi/acpi_fpdt.c | 6 ++-- drivers/acpi/acpi_ipmi.c | 1 - drivers/acpi/acpi_lpss.c | 4 ++- drivers/acpi/acpi_memhotplug.c | 2 +- drivers/acpi/acpi_pad.c | 4 +++ drivers/acpi/acpi_processor.c | 18 +++-------- drivers/acpi/battery.c | 64 +++++++++++++++++++++---------------- drivers/acpi/button.c | 10 +++--- drivers/acpi/cppc_acpi.c | 71 +++++++++++++++++++++--------- ------------ drivers/acpi/custom_method.c | 2 +- drivers/acpi/device_pm.c | 3 ++ drivers/acpi/device_sysfs.c | 15 ++++++--- drivers/acpi/dock.c | 7 +++-- 15 files changed, 138 insertions(+), 117 deletions(-)
-- 2.8.1
-- With Best Regards, Andy Shevchenko
OK. thanks for reviewing this patch set. On 2021/3/27 16:21, Andy Shevchenko wrote:
On Saturday, March 27, 2021, Xiaofei Tan <tanxiaofei@huawei.com <mailto:tanxiaofei@huawei.com>> wrote:
Fix some coding style issues reported by checkpatch.pl <http://checkpatch.pl>.
NAK until it’s proven that you have tested each change on the real system (virtual more or less okay).
Differences from v1 to v2: - Add subsystem and module name in the name of patch 05/15. - Change to use more proper module name for some patch names.
Xiaofei Tan (15): ACPI: APD: fix a block comment align issue ACPI: processor: fix some coding style issues ACPI: debug: fix some coding style issues ACPI: table: replace __attribute__((packed)) by __packed ACPI: ipmi: remove useless return statement for void function ACPI: LPSS: fix some coding style issues ACPI: memhotplug: fix a coding style issue ACPI: acpi_pad: fix a coding style issue ACPI: battery: fix some coding style issues ACPI: button: fix some coding style issues ACPI: CPPC: fix some coding style issues ACPI: custom_method: fix a coding style issue ACPI: PM: fix some coding style issues ACPI: sysfs: fix some coding style issues ACPI: dock: fix some coding style issues
drivers/acpi/acpi_apd.c | 8 ++--- drivers/acpi/acpi_dbg.c | 40 +++++++++++------------- drivers/acpi/acpi_fpdt.c | 6 ++-- drivers/acpi/acpi_ipmi.c | 1 - drivers/acpi/acpi_lpss.c | 4 ++- drivers/acpi/acpi_memhotplug.c | 2 +- drivers/acpi/acpi_pad.c | 4 +++ drivers/acpi/acpi_processor.c | 18 +++-------- drivers/acpi/battery.c | 64 +++++++++++++++++++++---------------- drivers/acpi/button.c | 10 +++--- drivers/acpi/cppc_acpi.c | 71 +++++++++++++++++++++--------------------- drivers/acpi/custom_method.c | 2 +- drivers/acpi/device_pm.c | 3 ++ drivers/acpi/device_sysfs.c | 15 ++++++--- drivers/acpi/dock.c | 7 +++-- 15 files changed, 138 insertions(+), 117 deletions(-)
-- 2.8.1
-- With Best Regards, Andy Shevchenko
participants (7)
-
Andy Shevchenko -
Bjorn Helgaas -
David Laight -
Joe Perches -
Rafael J. Wysocki -
Xiaofei Tan -
Zhang Rui