ACPI & Power Management Patches for Linux 3.5-rc4

ACPI & Power Management Patches for Linux 3.5-rc4

[Len Brown]

Here are some bug fixes queued for 3.5-rc4
Please let me know if you see troubles with any of them.

thanks,
Len Brown, Intel Open Source Technology Center

[PATCH 1/8] ACPI sysfs.c strlen fix

[Len Brown]

From: Pavel Vasilyev <pavel@pavlinux.ru>

Current code is ignoring the last character of "enable" and "disable"
in comparisons.

https://bugzilla.kernel.org/show_bug.cgi?id=33732

Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/acpi/sysfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 9f66181..240a244 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -173,7 +173,7 @@ static int param_set_trace_state(const char *val, struct kernel_param *kp)
 {
 	int result = 0;
 
-	if (!strncmp(val, "enable", strlen("enable") - 1)) {
+	if (!strncmp(val, "enable", strlen("enable"))) {
 		result = acpi_debug_trace(trace_method_name, trace_debug_level,
 					  trace_debug_layer, 0);
 		if (result)
@@ -181,7 +181,7 @@ static int param_set_trace_state(const char *val, struct kernel_param *kp)
 		goto exit;
 	}
 
-	if (!strncmp(val, "disable", strlen("disable") - 1)) {
+	if (!strncmp(val, "disable", strlen("disable"))) {
 		int name = 0;
 		result = acpi_debug_trace((char *)&name, trace_debug_level,
 					  trace_debug_layer, 0);
-- 
1.7.11.1.104.ge7b44f1

[PATCH 2/8] ACPI, x86: fix Dell M6600 ACPI reboot regression via DMI

[Len Brown]

From: Zhang Rui <rui.zhang@intel.com>

Dell Precision M6600 is known to require PCI reboot, so add it to
the reboot blacklist in pci_reboot_dmi_table[].

https://bugzilla.kernel.org/show_bug.cgi?id=42749

cc: x86@kernel.org
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/x86/kernel/reboot.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 79c45af..412db57 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -451,6 +451,14 @@ static struct dmi_system_id __initdata reboot_dmi_table[] = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 990"),
 		},
 	},
+	{	/* Handle problems with rebooting on the Precision M6600. */
+		.callback = set_pci_reboot,
+		.ident = "Dell OptiPlex 990",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Precision M6600"),
+		},
+	},
 	{ }
 };
 
-- 
1.7.11.1.104.ge7b44f1

[PATCH 2/8] ACPI, x86: fix Dell M6600 ACPI reboot regression via DMI

[Len Brown]

From: Zhang Rui <rui.zhang@intel.com>

Dell Precision M6600 is known to require PCI reboot, so add it to
the reboot blacklist in pci_reboot_dmi_table[].

https://bugzilla.kernel.org/show_bug.cgi?id=42749

cc: x86@kernel.org
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/x86/kernel/reboot.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 79c45af..412db57 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -451,6 +451,14 @@ static struct dmi_system_id __initdata reboot_dmi_table[] = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 990"),
 		},
 	},
+	{	/* Handle problems with rebooting on the Precision M6600. */
+		.callback = set_pci_reboot,
+		.ident = "Dell OptiPlex 990",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Precision M6600"),
+		},
+	},
 	{ }
 };
 
-- 
1.7.11.1.104.ge7b44f1

[PATCH 3/8] ACPI: Make acpi_skip_timer_override cover all source_irq==0 cases

From: Feng Tang <feng.tang@intel.com>

Currently when acpi_skip_timer_override is set, it only cover the
(source_irq == 0 && global_irq == 2) cases. While there is also
platform which need use this option and its global_irq is not 2.
This patch will extend acpi_skip_timer_override to cover all
timer overriding cases as long as the source irq is 0.

This is the first part of a fix to kernel bug bugzilla 40002:
	"IRQ 0 assigned to VGA"
https://bugzilla.kernel.org/show_bug.cgi?id=40002

Reported-and-tested-by: Szymon Kowalczyk <fazerxlo@o2.pl>
Signed-off-by: Feng Tang <feng.tang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/x86/kernel/acpi/boot.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 8afb693..e7c698e 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -422,12 +422,14 @@ acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
 		return 0;
 	}
 
-	if (intsrc->source_irq == 0 && intsrc->global_irq == 2) {
+	if (intsrc->source_irq == 0) {
 		if (acpi_skip_timer_override) {
-			printk(PREFIX "BIOS IRQ0 pin2 override ignored.\n");
+			printk(PREFIX "BIOS IRQ0 override ignored.\n");
 			return 0;
 		}
-		if (acpi_fix_pin2_polarity && (intsrc->inti_flags & ACPI_MADT_POLARITY_MASK)) {
+
+		if ((intsrc->global_irq == 2) && acpi_fix_pin2_polarity
+			&& (intsrc->inti_flags & ACPI_MADT_POLARITY_MASK)) {
 			intsrc->inti_flags &= ~ACPI_MADT_POLARITY_MASK;
 			printk(PREFIX "BIOS IRQ0 pin2 override: forcing polarity to high active.\n");
 		}
@@ -1334,7 +1336,7 @@ static int __init dmi_disable_acpi(const struct dmi_system_id *d)
 }
 
 /*
- * Force ignoring BIOS IRQ0 pin2 override
+ * Force ignoring BIOS IRQ0 override
  */
 static int __init dmi_ignore_irq0_timer_override(const struct dmi_system_id *d)
 {
@@ -1344,7 +1346,7 @@ static int __init dmi_ignore_irq0_timer_override(const struct dmi_system_id *d)
 	 */
 	if (!acpi_skip_timer_override) {
 		WARN(1, KERN_ERR "ati_ixp4x0 quirk not complete.\n");
-		pr_notice("%s detected: Ignoring BIOS IRQ0 pin2 override\n",
+		pr_notice("%s detected: Ignoring BIOS IRQ0 override\n",
 			d->ident);
 		acpi_skip_timer_override = 1;
 	}
@@ -1438,7 +1440,7 @@ static struct dmi_system_id __initdata acpi_dmi_table_late[] = {
 	 * is enabled.  This input is incorrectly designated the
 	 * ISA IRQ 0 via an interrupt source override even though
 	 * it is wired to the output of the master 8259A and INTIN0
-	 * is not connected at all.  Force ignoring BIOS IRQ0 pin2
+	 * is not connected at all.  Force ignoring BIOS IRQ0
 	 * override in that cases.
 	 */
 	{
-- 
1.7.11.1.104.ge7b44f1

[PATCH 3/8] ACPI: Make acpi_skip_timer_override cover all source_irq==0 cases

[Len Brown]

From: Feng Tang <feng.tang@intel.com>

Currently when acpi_skip_timer_override is set, it only cover the
(source_irq == 0 && global_irq == 2) cases. While there is also
platform which need use this option and its global_irq is not 2.
This patch will extend acpi_skip_timer_override to cover all
timer overriding cases as long as the source irq is 0.

This is the first part of a fix to kernel bug bugzilla 40002:
	"IRQ 0 assigned to VGA"
https://bugzilla.kernel.org/show_bug.cgi?id=40002

Reported-and-tested-by: Szymon Kowalczyk <fazerxlo@o2.pl>
Signed-off-by: Feng Tang <feng.tang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/x86/kernel/acpi/boot.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 8afb693..e7c698e 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -422,12 +422,14 @@ acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
 		return 0;
 	}
 
-	if (intsrc->source_irq == 0 && intsrc->global_irq == 2) {
+	if (intsrc->source_irq == 0) {
 		if (acpi_skip_timer_override) {
-			printk(PREFIX "BIOS IRQ0 pin2 override ignored.\n");
+			printk(PREFIX "BIOS IRQ0 override ignored.\n");
 			return 0;
 		}
-		if (acpi_fix_pin2_polarity && (intsrc->inti_flags & ACPI_MADT_POLARITY_MASK)) {
+
+		if ((intsrc->global_irq == 2) && acpi_fix_pin2_polarity
+			&& (intsrc->inti_flags & ACPI_MADT_POLARITY_MASK)) {
 			intsrc->inti_flags &= ~ACPI_MADT_POLARITY_MASK;
 			printk(PREFIX "BIOS IRQ0 pin2 override: forcing polarity to high active.\n");
 		}
@@ -1334,7 +1336,7 @@ static int __init dmi_disable_acpi(const struct dmi_system_id *d)
 }
 
 /*
- * Force ignoring BIOS IRQ0 pin2 override
+ * Force ignoring BIOS IRQ0 override
  */
 static int __init dmi_ignore_irq0_timer_override(const struct dmi_system_id *d)
 {
@@ -1344,7 +1346,7 @@ static int __init dmi_ignore_irq0_timer_override(const struct dmi_system_id *d)
 	 */
 	if (!acpi_skip_timer_override) {
 		WARN(1, KERN_ERR "ati_ixp4x0 quirk not complete.\n");
-		pr_notice("%s detected: Ignoring BIOS IRQ0 pin2 override\n",
+		pr_notice("%s detected: Ignoring BIOS IRQ0 override\n",
 			d->ident);
 		acpi_skip_timer_override = 1;
 	}
@@ -1438,7 +1440,7 @@ static struct dmi_system_id __initdata acpi_dmi_table_late[] = {
 	 * is enabled.  This input is incorrectly designated the
 	 * ISA IRQ 0 via an interrupt source override even though
 	 * it is wired to the output of the master 8259A and INTIN0
-	 * is not connected at all.  Force ignoring BIOS IRQ0 pin2
+	 * is not connected at all.  Force ignoring BIOS IRQ0
 	 * override in that cases.
 	 */
 	{
-- 
1.7.11.1.104.ge7b44f1

[PATCH 4/8] ACPI: Remove one board specific WARN when ignoring timer overriding

[Len Brown]

From: Feng Tang <feng.tang@intel.com>

Current WARN msg is only for the ati_ixp4x0 board, while this function
is used by mulitple platforms. So this one board specific warning
is not appropriate any more.

Signed-off-by: Feng Tang <feng.tang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/x86/kernel/acpi/boot.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index e7c698e..3a6afba 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -1340,12 +1340,7 @@ static int __init dmi_disable_acpi(const struct dmi_system_id *d)
  */
 static int __init dmi_ignore_irq0_timer_override(const struct dmi_system_id *d)
 {
-	/*
-	 * The ati_ixp4x0_rev() early PCI quirk should have set
-	 * the acpi_skip_timer_override flag already:
-	 */
 	if (!acpi_skip_timer_override) {
-		WARN(1, KERN_ERR "ati_ixp4x0 quirk not complete.\n");
 		pr_notice("%s detected: Ignoring BIOS IRQ0 override\n",
 			d->ident);
 		acpi_skip_timer_override = 1;
-- 
1.7.11.1.104.ge7b44f1

[PATCH 4/8] ACPI: Remove one board specific WARN when ignoring timer overriding

[Len Brown]

From: Feng Tang <feng.tang@intel.com>

Current WARN msg is only for the ati_ixp4x0 board, while this function
is used by mulitple platforms. So this one board specific warning
is not appropriate any more.

Signed-off-by: Feng Tang <feng.tang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/x86/kernel/acpi/boot.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index e7c698e..3a6afba 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -1340,12 +1340,7 @@ static int __init dmi_disable_acpi(const struct dmi_system_id *d)
  */
 static int __init dmi_ignore_irq0_timer_override(const struct dmi_system_id *d)
 {
-	/*
-	 * The ati_ixp4x0_rev() early PCI quirk should have set
-	 * the acpi_skip_timer_override flag already:
-	 */
 	if (!acpi_skip_timer_override) {
-		WARN(1, KERN_ERR "ati_ixp4x0 quirk not complete.\n");
 		pr_notice("%s detected: Ignoring BIOS IRQ0 override\n",
 			d->ident);
 		acpi_skip_timer_override = 1;
-- 
1.7.11.1.104.ge7b44f1

[PATCH 5/8] ACPI: Add a quirk for "AMILO PRO V2030" to ignore the timer overriding

[Len Brown]

From: Feng Tang <feng.tang@intel.com>

This is the 2nd part of fix for kernel bugzilla 40002:
    "IRQ 0 assigned to VGA"
https://bugzilla.kernel.org/show_bug.cgi?id=40002

The root cause is the buggy FW, whose ACPI tables assign the GSI 16
to 2 irqs 0 and 16(VGA), and the VGA is the right owner of GSI 16.
So add a quirk to ignore the irq0 overriding GSI 16 for the
FUJITSU SIEMENS AMILO PRO V2030 platform will solve this issue.

Reported-and-tested-by: Szymon Kowalczyk <fazerxlo@o2.pl>
Signed-off-by: Feng Tang <feng.tang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/x86/kernel/acpi/boot.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 3a6afba..b2297e5 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -1470,6 +1470,14 @@ static struct dmi_system_id __initdata acpi_dmi_table_late[] = {
 		     DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq 6715b"),
 		     },
 	 },
+	{
+	 .callback = dmi_ignore_irq0_timer_override,
+	 .ident = "FUJITSU SIEMENS",
+	 .matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU SIEMENS"),
+		     DMI_MATCH(DMI_PRODUCT_NAME, "AMILO PRO V2030"),
+		     },
+	 },
 	{}
 };
 
-- 
1.7.11.1.104.ge7b44f1

[PATCH 5/8] ACPI: Add a quirk for "AMILO PRO V2030" to ignore the timer overriding

[Len Brown]

From: Feng Tang <feng.tang@intel.com>

This is the 2nd part of fix for kernel bugzilla 40002:
    "IRQ 0 assigned to VGA"
https://bugzilla.kernel.org/show_bug.cgi?id=40002

The root cause is the buggy FW, whose ACPI tables assign the GSI 16
to 2 irqs 0 and 16(VGA), and the VGA is the right owner of GSI 16.
So add a quirk to ignore the irq0 overriding GSI 16 for the
FUJITSU SIEMENS AMILO PRO V2030 platform will solve this issue.

Reported-and-tested-by: Szymon Kowalczyk <fazerxlo@o2.pl>
Signed-off-by: Feng Tang <feng.tang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/x86/kernel/acpi/boot.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 3a6afba..b2297e5 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -1470,6 +1470,14 @@ static struct dmi_system_id __initdata acpi_dmi_table_late[] = {
 		     DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq 6715b"),
 		     },
 	 },
+	{
+	 .callback = dmi_ignore_irq0_timer_override,
+	 .ident = "FUJITSU SIEMENS",
+	 .matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU SIEMENS"),
+		     DMI_MATCH(DMI_PRODUCT_NAME, "AMILO PRO V2030"),
+		     },
+	 },
 	{}
 };
 
-- 
1.7.11.1.104.ge7b44f1

[PATCH 6/8] ACPI, APEI, Avoid too much error reporting in runtime

[Len Brown]

From: Huang Ying <ying.huang@intel.com>

This patch fixed the following bug.

https://bugzilla.kernel.org/show_bug.cgi?id=43282

This is caused by a firmware bug checking (checking generic address
register provided by firmware) in runtime.  The checking should be
done in address mapping time instead of runtime to avoid too much
error reporting in runtime.

Reported-by: Pawel Sikora <pluto@agmk.net>
Signed-off-by: Huang Ying <ying.huang@intel.com>
Tested-by: Jean Delvare <khali@linux-fr.org>
Cc: stable@vger.kernel.org
Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/acpi/apei/apei-base.c     | 17 +++++++++++++++--
 drivers/acpi/apei/apei-internal.h |  9 +++++++++
 drivers/acpi/apei/ghes.c          |  6 +++---
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index 5577762..6686b1e 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -243,7 +243,7 @@ static int pre_map_gar_callback(struct apei_exec_context *ctx,
 	u8 ins = entry->instruction;
 
 	if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER)
-		return acpi_os_map_generic_address(&entry->register_region);
+		return apei_map_generic_address(&entry->register_region);
 
 	return 0;
 }
@@ -276,7 +276,7 @@ static int post_unmap_gar_callback(struct apei_exec_context *ctx,
 	u8 ins = entry->instruction;
 
 	if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER)
-		acpi_os_unmap_generic_address(&entry->register_region);
+		apei_unmap_generic_address(&entry->register_region);
 
 	return 0;
 }
@@ -606,6 +606,19 @@ static int apei_check_gar(struct acpi_generic_address *reg, u64 *paddr,
 	return 0;
 }
 
+int apei_map_generic_address(struct acpi_generic_address *reg)
+{
+	int rc;
+	u32 access_bit_width;
+	u64 address;
+
+	rc = apei_check_gar(reg, &address, &access_bit_width);
+	if (rc)
+		return rc;
+	return acpi_os_map_generic_address(reg);
+}
+EXPORT_SYMBOL_GPL(apei_map_generic_address);
+
 /* read GAR in interrupt (including NMI) or process context */
 int apei_read(u64 *val, struct acpi_generic_address *reg)
 {
diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
index cca240a..f220d64 100644
--- a/drivers/acpi/apei/apei-internal.h
+++ b/drivers/acpi/apei/apei-internal.h
@@ -7,6 +7,8 @@
 #define APEI_INTERNAL_H
 
 #include <linux/cper.h>
+#include <linux/acpi.h>
+#include <linux/acpi_io.h>
 
 struct apei_exec_context;
 
@@ -68,6 +70,13 @@ static inline int apei_exec_run_optional(struct apei_exec_context *ctx, u8 actio
 /* IP has been set in instruction function */
 #define APEI_EXEC_SET_IP	1
 
+int apei_map_generic_address(struct acpi_generic_address *reg);
+
+static inline void apei_unmap_generic_address(struct acpi_generic_address *reg)
+{
+	acpi_os_unmap_generic_address(reg);
+}
+
 int apei_read(u64 *val, struct acpi_generic_address *reg);
 int apei_write(u64 val, struct acpi_generic_address *reg);
 
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 9b3cac0..1599566 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -301,7 +301,7 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
 	if (!ghes)
 		return ERR_PTR(-ENOMEM);
 	ghes->generic = generic;
-	rc = acpi_os_map_generic_address(&generic->error_status_address);
+	rc = apei_map_generic_address(&generic->error_status_address);
 	if (rc)
 		goto err_free;
 	error_block_length = generic->error_block_length;
@@ -321,7 +321,7 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
 	return ghes;
 
 err_unmap:
-	acpi_os_unmap_generic_address(&generic->error_status_address);
+	apei_unmap_generic_address(&generic->error_status_address);
 err_free:
 	kfree(ghes);
 	return ERR_PTR(rc);
@@ -330,7 +330,7 @@ err_free:
 static void ghes_fini(struct ghes *ghes)
 {
 	kfree(ghes->estatus);
-	acpi_os_unmap_generic_address(&ghes->generic->error_status_address);
+	apei_unmap_generic_address(&ghes->generic->error_status_address);
 }
 
 enum {
-- 
1.7.11.1.104.ge7b44f1

[PATCH 6/8] ACPI, APEI, Avoid too much error reporting in runtime

[Len Brown]

From: Huang Ying <ying.huang@intel.com>

This patch fixed the following bug.

https://bugzilla.kernel.org/show_bug.cgi?id=43282

This is caused by a firmware bug checking (checking generic address
register provided by firmware) in runtime.  The checking should be
done in address mapping time instead of runtime to avoid too much
error reporting in runtime.

Reported-by: Pawel Sikora <pluto@agmk.net>
Signed-off-by: Huang Ying <ying.huang@intel.com>
Tested-by: Jean Delvare <khali@linux-fr.org>
Cc: stable@vger.kernel.org
Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/acpi/apei/apei-base.c     | 17 +++++++++++++++--
 drivers/acpi/apei/apei-internal.h |  9 +++++++++
 drivers/acpi/apei/ghes.c          |  6 +++---
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index 5577762..6686b1e 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -243,7 +243,7 @@ static int pre_map_gar_callback(struct apei_exec_context *ctx,
 	u8 ins = entry->instruction;
 
 	if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER)
-		return acpi_os_map_generic_address(&entry->register_region);
+		return apei_map_generic_address(&entry->register_region);
 
 	return 0;
 }
@@ -276,7 +276,7 @@ static int post_unmap_gar_callback(struct apei_exec_context *ctx,
 	u8 ins = entry->instruction;
 
 	if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER)
-		acpi_os_unmap_generic_address(&entry->register_region);
+		apei_unmap_generic_address(&entry->register_region);
 
 	return 0;
 }
@@ -606,6 +606,19 @@ static int apei_check_gar(struct acpi_generic_address *reg, u64 *paddr,
 	return 0;
 }
 
+int apei_map_generic_address(struct acpi_generic_address *reg)
+{
+	int rc;
+	u32 access_bit_width;
+	u64 address;
+
+	rc = apei_check_gar(reg, &address, &access_bit_width);
+	if (rc)
+		return rc;
+	return acpi_os_map_generic_address(reg);
+}
+EXPORT_SYMBOL_GPL(apei_map_generic_address);
+
 /* read GAR in interrupt (including NMI) or process context */
 int apei_read(u64 *val, struct acpi_generic_address *reg)
 {
diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
index cca240a..f220d64 100644
--- a/drivers/acpi/apei/apei-internal.h
+++ b/drivers/acpi/apei/apei-internal.h
@@ -7,6 +7,8 @@
 #define APEI_INTERNAL_H
 
 #include <linux/cper.h>
+#include <linux/acpi.h>
+#include <linux/acpi_io.h>
 
 struct apei_exec_context;
 
@@ -68,6 +70,13 @@ static inline int apei_exec_run_optional(struct apei_exec_context *ctx, u8 actio
 /* IP has been set in instruction function */
 #define APEI_EXEC_SET_IP	1
 
+int apei_map_generic_address(struct acpi_generic_address *reg);
+
+static inline void apei_unmap_generic_address(struct acpi_generic_address *reg)
+{
+	acpi_os_unmap_generic_address(reg);
+}
+
 int apei_read(u64 *val, struct acpi_generic_address *reg);
 int apei_write(u64 val, struct acpi_generic_address *reg);
 
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 9b3cac0..1599566 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -301,7 +301,7 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
 	if (!ghes)
 		return ERR_PTR(-ENOMEM);
 	ghes->generic = generic;
-	rc = acpi_os_map_generic_address(&generic->error_status_address);
+	rc = apei_map_generic_address(&generic->error_status_address);
 	if (rc)
 		goto err_free;
 	error_block_length = generic->error_block_length;
@@ -321,7 +321,7 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
 	return ghes;
 
 err_unmap:
-	acpi_os_unmap_generic_address(&generic->error_status_address);
+	apei_unmap_generic_address(&generic->error_status_address);
 err_free:
 	kfree(ghes);
 	return ERR_PTR(rc);
@@ -330,7 +330,7 @@ err_free:
 static void ghes_fini(struct ghes *ghes)
 {
 	kfree(ghes->estatus);
-	acpi_os_unmap_generic_address(&ghes->generic->error_status_address);
+	apei_unmap_generic_address(&ghes->generic->error_status_address);
 }
 
 enum {
-- 
1.7.11.1.104.ge7b44f1

[PATCH 7/8] ACPI video: Still use ACPI backlight control if _DOS doesn't exist

[Len Brown]

From: Zhang Rui <rui.zhang@intel.com>

This fixes a regression in 3.4-rc1 caused by commit
ea9f8856bd6d4ed45885b06a338f7362cd6c60e5
(ACPI video: Harden video bus adding.)

Some platforms don't have _DOS control method, but the ACPI
backlight still works.
We should not invoke _DOS for these platforms.

https://bugzilla.kernel.org/show_bug.cgi?id=43168

Cc: Igor Murzov <intergalactic.anonymous@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/acpi/video.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 9577b6f..4134b30 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -558,6 +558,8 @@ acpi_video_bus_DOS(struct acpi_video_bus *video, int bios_flag, int lcd_flag)
 	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
 	struct acpi_object_list args = { 1, &arg0 };
 
+	if (!video->cap._DOS)
+		return 0;
 
 	if (bios_flag < 0 || bios_flag > 3 || lcd_flag < 0 || lcd_flag > 1)
 		return -EINVAL;
-- 
1.7.11.1.104.ge7b44f1

[PATCH 8/8] acpi_pad: fix power_saving thread deadlock

[Len Brown]

From: Stuart Hayes <Stuart_Hayes@Dell.com>

The acpi_pad driver can get stuck in destroy_power_saving_task()
waiting for kthread_stop() to stop a power_saving thread.  The problem
is that the isolated_cpus_lock mutex is owned when
destroy_power_saving_task() calls kthread_stop(), which waits for a
power_saving thread to end, and the power_saving thread tries to
acquire the isolated_cpus_lock when it calls round_robin_cpu().  This
patch fixes the issue by making round_robin_cpu() use its own mutex.

https://bugzilla.kernel.org/show_bug.cgi?id=42981

Cc: stable@vger.kernel.org
Signed-off-by: Stuart Hayes <Stuart_Hayes@Dell.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/acpi/acpi_pad.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_pad.c b/drivers/acpi/acpi_pad.c
index a43fa1a..1502c502 100644
--- a/drivers/acpi/acpi_pad.c
+++ b/drivers/acpi/acpi_pad.c
@@ -36,6 +36,7 @@
 #define ACPI_PROCESSOR_AGGREGATOR_DEVICE_NAME "Processor Aggregator"
 #define ACPI_PROCESSOR_AGGREGATOR_NOTIFY 0x80
 static DEFINE_MUTEX(isolated_cpus_lock);
+static DEFINE_MUTEX(round_robin_lock);
 
 static unsigned long power_saving_mwait_eax;
 
@@ -107,7 +108,7 @@ static void round_robin_cpu(unsigned int tsk_index)
 	if (!alloc_cpumask_var(&tmp, GFP_KERNEL))
 		return;
 
-	mutex_lock(&isolated_cpus_lock);
+	mutex_lock(&round_robin_lock);
 	cpumask_clear(tmp);
 	for_each_cpu(cpu, pad_busy_cpus)
 		cpumask_or(tmp, tmp, topology_thread_cpumask(cpu));
@@ -116,7 +117,7 @@ static void round_robin_cpu(unsigned int tsk_index)
 	if (cpumask_empty(tmp))
 		cpumask_andnot(tmp, cpu_online_mask, pad_busy_cpus);
 	if (cpumask_empty(tmp)) {
-		mutex_unlock(&isolated_cpus_lock);
+		mutex_unlock(&round_robin_lock);
 		return;
 	}
 	for_each_cpu(cpu, tmp) {
@@ -131,7 +132,7 @@ static void round_robin_cpu(unsigned int tsk_index)
 	tsk_in_cpu[tsk_index] = preferred_cpu;
 	cpumask_set_cpu(preferred_cpu, pad_busy_cpus);
 	cpu_weight[preferred_cpu]++;
-	mutex_unlock(&isolated_cpus_lock);
+	mutex_unlock(&round_robin_lock);
 
 	set_cpus_allowed_ptr(current, cpumask_of(preferred_cpu));
 }
-- 
1.7.11.1.104.ge7b44f1

[PATCH 8/8] acpi_pad: fix power_saving thread deadlock

[Len Brown]

From: Stuart Hayes <Stuart_Hayes@Dell.com>

The acpi_pad driver can get stuck in destroy_power_saving_task()
waiting for kthread_stop() to stop a power_saving thread.  The problem
is that the isolated_cpus_lock mutex is owned when
destroy_power_saving_task() calls kthread_stop(), which waits for a
power_saving thread to end, and the power_saving thread tries to
acquire the isolated_cpus_lock when it calls round_robin_cpu().  This
patch fixes the issue by making round_robin_cpu() use its own mutex.

https://bugzilla.kernel.org/show_bug.cgi?id=42981

Cc: stable@vger.kernel.org
Signed-off-by: Stuart Hayes <Stuart_Hayes@Dell.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/acpi/acpi_pad.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_pad.c b/drivers/acpi/acpi_pad.c
index a43fa1a..1502c502 100644
--- a/drivers/acpi/acpi_pad.c
+++ b/drivers/acpi/acpi_pad.c
@@ -36,6 +36,7 @@
 #define ACPI_PROCESSOR_AGGREGATOR_DEVICE_NAME "Processor Aggregator"
 #define ACPI_PROCESSOR_AGGREGATOR_NOTIFY 0x80
 static DEFINE_MUTEX(isolated_cpus_lock);
+static DEFINE_MUTEX(round_robin_lock);
 
 static unsigned long power_saving_mwait_eax;
 
@@ -107,7 +108,7 @@ static void round_robin_cpu(unsigned int tsk_index)
 	if (!alloc_cpumask_var(&tmp, GFP_KERNEL))
 		return;
 
-	mutex_lock(&isolated_cpus_lock);
+	mutex_lock(&round_robin_lock);
 	cpumask_clear(tmp);
 	for_each_cpu(cpu, pad_busy_cpus)
 		cpumask_or(tmp, tmp, topology_thread_cpumask(cpu));
@@ -116,7 +117,7 @@ static void round_robin_cpu(unsigned int tsk_index)
 	if (cpumask_empty(tmp))
 		cpumask_andnot(tmp, cpu_online_mask, pad_busy_cpus);
 	if (cpumask_empty(tmp)) {
-		mutex_unlock(&isolated_cpus_lock);
+		mutex_unlock(&round_robin_lock);
 		return;
 	}
 	for_each_cpu(cpu, tmp) {
@@ -131,7 +132,7 @@ static void round_robin_cpu(unsigned int tsk_index)
 	tsk_in_cpu[tsk_index] = preferred_cpu;
 	cpumask_set_cpu(preferred_cpu, pad_busy_cpus);
 	cpu_weight[preferred_cpu]++;
-	mutex_unlock(&isolated_cpus_lock);
+	mutex_unlock(&round_robin_lock);
 
 	set_cpus_allowed_ptr(current, cpumask_of(preferred_cpu));
 }
-- 
1.7.11.1.104.ge7b44f1

Re: [PATCH 1/8] ACPI sysfs.c strlen fix

[Alan Stern]

On Sat, 30 Jun 2012, Len Brown wrote:

> From: Pavel Vasilyev <pavel@pavlinux.ru>
> 
> Current code is ignoring the last character of "enable" and "disable"
> in comparisons.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=33732
> 
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
>  drivers/acpi/sysfs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
> index 9f66181..240a244 100644
> --- a/drivers/acpi/sysfs.c
> +++ b/drivers/acpi/sysfs.c
> @@ -173,7 +173,7 @@ static int param_set_trace_state(const char *val, struct kernel_param *kp)
>  {
>  	int result = 0;
>  
> -	if (!strncmp(val, "enable", strlen("enable") - 1)) {
> +	if (!strncmp(val, "enable", strlen("enable"))) {

Ironically, this is the less efficient of two possible fixes.

It would have been better to leave the "- 1" in place and change strlen 
to sizeof.  That's probably what the original author had in mind.

Alan Stern

Re: [linux-pm] [PATCH 1/8] ACPI sysfs.c strlen fix

[Alan Stern]

On Sat, 30 Jun 2012, Len Brown wrote:

> From: Pavel Vasilyev <pavel@pavlinux.ru>
> 
> Current code is ignoring the last character of "enable" and "disable"
> in comparisons.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=33732
> 
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
>  drivers/acpi/sysfs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
> index 9f66181..240a244 100644
> --- a/drivers/acpi/sysfs.c
> +++ b/drivers/acpi/sysfs.c
> @@ -173,7 +173,7 @@ static int param_set_trace_state(const char *val, struct kernel_param *kp)
>  {
>  	int result = 0;
>  
> -	if (!strncmp(val, "enable", strlen("enable") - 1)) {
> +	if (!strncmp(val, "enable", strlen("enable"))) {

Ironically, this is the less efficient of two possible fixes.

It would have been better to leave the "- 1" in place and change strlen 
to sizeof.  That's probably what the original author had in mind.

Alan Stern

Re: [PATCH 2/8] ACPI, x86: fix Dell M6600 ACPI reboot regression via DMI

[Ingo Molnar]

* Len Brown <lenb@kernel.org> wrote:

> From: Zhang Rui <rui.zhang@intel.com>
> 
> Dell Precision M6600 is known to require PCI reboot, so add it to
> the reboot blacklist in pci_reboot_dmi_table[].
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=42749
> 
> cc: x86@kernel.org
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
>  arch/x86/kernel/reboot.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

[PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Len Brown]

...both give the number of chars in the string
without the '\0', as strncmp() wants,
but sizeof() is compile-time.

Reported-by: Alan Stern <stern@rowland.harvard.edu>
Cc: Pavel Vasilyev <pavel@pavlinux.ru>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/acpi/sysfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 240a244..7c3f98b 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -173,7 +173,7 @@ static int param_set_trace_state(const char *val,
struct kernel_param *kp)
 {
 	int result = 0;

-	if (!strncmp(val, "enable", strlen("enable"))) {
+	if (!strncmp(val, "enable", sizeof("enable") - 1)) {
 		result = acpi_debug_trace(trace_method_name, trace_debug_level,
 					  trace_debug_layer, 0);
 		if (result)
@@ -181,7 +181,7 @@ static int param_set_trace_state(const char *val,
struct kernel_param *kp)
 		goto exit;
 	}

-	if (!strncmp(val, "disable", strlen("disable"))) {
+	if (!strncmp(val, "disable", sizeof("disable") - 1)) {
 		int name = 0;
 		result = acpi_debug_trace((char *)&name, trace_debug_level,
 					  trace_debug_layer, 0);
-- 
1.7.12.rc0

[PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Len Brown]

...both give the number of chars in the string
without the '\0', as strncmp() wants,
but sizeof() is compile-time.

Reported-by: Alan Stern <stern@rowland.harvard.edu>
Cc: Pavel Vasilyev <pavel@pavlinux.ru>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/acpi/sysfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 240a244..7c3f98b 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -173,7 +173,7 @@ static int param_set_trace_state(const char *val,
struct kernel_param *kp)
 {
 	int result = 0;

-	if (!strncmp(val, "enable", strlen("enable"))) {
+	if (!strncmp(val, "enable", sizeof("enable") - 1)) {
 		result = acpi_debug_trace(trace_method_name, trace_debug_level,
 					  trace_debug_layer, 0);
 		if (result)
@@ -181,7 +181,7 @@ static int param_set_trace_state(const char *val,
struct kernel_param *kp)
 		goto exit;
 	}

-	if (!strncmp(val, "disable", strlen("disable"))) {
+	if (!strncmp(val, "disable", sizeof("disable") - 1)) {
 		int name = 0;
 		result = acpi_debug_trace((char *)&name, trace_debug_level,
 					  trace_debug_layer, 0);
-- 
1.7.12.rc0

Re: [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Ian Campbell]

On Thu, 2012-07-26 at 21:39 -0400, Len Brown wrote:
> ...both give the number of chars in the string
> without the '\0', as strncmp() wants,
> but sizeof() is compile-time.

I thought gcc optimised strlen("string literal") into a compile time
constant too. It does in a little userspace test I just wrote, but I
didn't look at its behaviour with the kernel's strlen.

Ian.

Re: [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[richard -rw- weinberger]

On Mon, Jul 30, 2012 at 4:26 PM, Ian Campbell <ijc@hellion.org.uk> wrote:
> I thought gcc optimised strlen("string literal") into a compile time
> constant too. It does in a little userspace test I just wrote, but I
> didn't look at its behaviour with the kernel's strlen.

It depends whether you use the built-in strlen() or not.
AFAIK the kernel does not use the built-in variant.

-- 
Thanks,
//richard

Re: [linux-pm] [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Pavel Machek]

On Thu 2012-07-26 21:39:38, Len Brown wrote:
> ...both give the number of chars in the string
> without the '\0', as strncmp() wants,
> but sizeof() is compile-time.

What about introducing something like streq() to do this
automatically? This is ugly....

#define streq(a, b) ... if (_buildin_constant(b)) ...

?

> -	if (!strncmp(val, "enable", strlen("enable"))) {
> +	if (!strncmp(val, "enable", sizeof("enable") - 1)) {

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Andreas Schwab]

Len Brown <lenb@kernel.org> writes:

> ...both give the number of chars in the string
> without the '\0', as strncmp() wants,
> but sizeof() is compile-time.

Does this actually change anything?  The compiler is able to expand
strlen at compile time if the argument is a constant, provided that that
builtin strlen isn't disabled.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Alan Stern]

On Mon, 6 Aug 2012, Pavel Machek wrote:

> On Thu 2012-07-26 21:39:38, Len Brown wrote:
> > ...both give the number of chars in the string
> > without the '\0', as strncmp() wants,
> > but sizeof() is compile-time.
> 
> What about introducing something like streq() to do this
> automatically? This is ugly....
> 
> #define streq(a, b) ... if (_buildin_constant(b)) ...
> 
> ?
> 
> > -	if (!strncmp(val, "enable", strlen("enable"))) {
> > +	if (!strncmp(val, "enable", sizeof("enable") - 1)) {

While you're at it, there's no point using strncmp when you know the 
length of one of the strings beforehand.  Just use memcmp, and don't 
subtract 1 from the sizeof value.

Alan Stern

Re: [linux-pm] [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Alan Stern]

On Mon, 6 Aug 2012, Pavel Machek wrote:

> On Thu 2012-07-26 21:39:38, Len Brown wrote:
> > ...both give the number of chars in the string
> > without the '\0', as strncmp() wants,
> > but sizeof() is compile-time.
> 
> What about introducing something like streq() to do this
> automatically? This is ugly....
> 
> #define streq(a, b) ... if (_buildin_constant(b)) ...
> 
> ?
> 
> > -	if (!strncmp(val, "enable", strlen("enable"))) {
> > +	if (!strncmp(val, "enable", sizeof("enable") - 1)) {

While you're at it, there's no point using strncmp when you know the 
length of one of the strings beforehand.  Just use memcmp, and don't 
subtract 1 from the sizeof value.

Alan Stern

Re: [linux-pm] [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Pavel Vasilyev]

06.08.2012 18:36, Alan Stern пишет:
> On Mon, 6 Aug 2012, Pavel Machek wrote:
>
>> On Thu 2012-07-26 21:39:38, Len Brown wrote:
>>> ...both give the number of chars in the string
>>> without the '\0', as strncmp() wants,
>>> but sizeof() is compile-time.
>>
>> What about introducing something like streq() to do this
>> automatically? This is ugly....
>>
>> #define streq(a, b) ... if (_buildin_constant(b)) ...
>>
>> ?
>>
>>> -	if (!strncmp(val, "enable", strlen("enable"))) {
>>> +	if (!strncmp(val, "enable", sizeof("enable") - 1)) {
>
> While you're at it, there's no point using strncmp when you know the
> length of one of the strings beforehand.  Just use memcmp, and don't
> subtract 1 from the sizeof value.

http://www.gossamer-threads.com/lists/engine?do=post_attachment;postatt_id=41157;list=linux

:)




-- 

                                                          Pavel.

Re: [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Alan Stern]

On Mon, 6 Aug 2012, Pavel Vasilyev wrote:

> 06.08.2012 18:36, Alan Stern пишет:
> > On Mon, 6 Aug 2012, Pavel Machek wrote:
> >
> >> On Thu 2012-07-26 21:39:38, Len Brown wrote:
> >>> ...both give the number of chars in the string
> >>> without the '\0', as strncmp() wants,
> >>> but sizeof() is compile-time.
> >>
> >> What about introducing something like streq() to do this
> >> automatically? This is ugly....
> >>
> >> #define streq(a, b) ... if (_buildin_constant(b)) ...
> >>
> >> ?
> >>
> >>> -	if (!strncmp(val, "enable", strlen("enable"))) {
> >>> +	if (!strncmp(val, "enable", sizeof("enable") - 1)) {
> >
> > While you're at it, there's no point using strncmp when you know the
> > length of one of the strings beforehand.  Just use memcmp, and don't
> > subtract 1 from the sizeof value.
> 
> http://www.gossamer-threads.com/lists/engine?do=post_attachment;postatt_id=41157;list=linux

Interestingly, many (all?) of the changes in that patch are wrong 
because they don't try to match the terminating '\0'.  As a result, 
they will match against extensions of the target string as well as the 
target string itself.

Alan Stern

Re: [linux-pm] [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Alan Stern]

On Mon, 6 Aug 2012, Pavel Vasilyev wrote:

> 06.08.2012 18:36, Alan Stern пишет:
> > On Mon, 6 Aug 2012, Pavel Machek wrote:
> >
> >> On Thu 2012-07-26 21:39:38, Len Brown wrote:
> >>> ...both give the number of chars in the string
> >>> without the '\0', as strncmp() wants,
> >>> but sizeof() is compile-time.
> >>
> >> What about introducing something like streq() to do this
> >> automatically? This is ugly....
> >>
> >> #define streq(a, b) ... if (_buildin_constant(b)) ...
> >>
> >> ?
> >>
> >>> -	if (!strncmp(val, "enable", strlen("enable"))) {
> >>> +	if (!strncmp(val, "enable", sizeof("enable") - 1)) {
> >
> > While you're at it, there's no point using strncmp when you know the
> > length of one of the strings beforehand.  Just use memcmp, and don't
> > subtract 1 from the sizeof value.
> 
> http://www.gossamer-threads.com/lists/engine?do=post_attachment;postatt_id=41157;list=linux

Interestingly, many (all?) of the changes in that patch are wrong 
because they don't try to match the terminating '\0'.  As a result, 
they will match against extensions of the target string as well as the 
target string itself.

Alan Stern

Re: [linux-pm] [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Pavel Vasilyev]

06.08.2012 20:28, Alan Stern пишет:
> On Mon, 6 Aug 2012, Pavel Vasilyev wrote:
>
>> 06.08.2012 18:36, Alan Stern пишет:
>>> On Mon, 6 Aug 2012, Pavel Machek wrote:
>>>
>>>> On Thu 2012-07-26 21:39:38, Len Brown wrote:
>>>>> ...both give the number of chars in the string
>>>>> without the '\0', as strncmp() wants,
>>>>> but sizeof() is compile-time.
>>>>
>>>> What about introducing something like streq() to do this
>>>> automatically? This is ugly....
>>>>
>>>> #define streq(a, b) ... if (_buildin_constant(b)) ...
>>>>
>>>> ?
>>>>
>>>>> -	if (!strncmp(val, "enable", strlen("enable"))) {
>>>>> +	if (!strncmp(val, "enable", sizeof("enable") - 1)) {
>>>
>>> While you're at it, there's no point using strncmp when you know the
>>> length of one of the strings beforehand.  Just use memcmp, and don't
>>> subtract 1 from the sizeof value.
>>
>> http://www.gossamer-threads.com/lists/engine?do=post_attachment;postatt_id=41157;list=linux
>
> Interestingly, many (all?) of the changes in that patch are wrong
> because they don't try to match the terminating '\0'.  As a result,
> they will match against extensions of the target string as well as the
> target string itself.
>

strNcmp compare N bytes - http://lxr.linux.no/#linux+v3.5/lib/string.c#L270
memcmp compare N bytes  - http://lxr.linux.no/#linux+v3.5/lib/string.c#L651

-- 

                                                          Pavel.

Re: [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Alan Stern]

On Mon, 6 Aug 2012, Pavel Vasilyev wrote:

> >> http://www.gossamer-threads.com/lists/engine?do=post_attachment;postatt_id=41157;list=linux
> >
> > Interestingly, many (all?) of the changes in that patch are wrong
> > because they don't try to match the terminating '\0'.  As a result,
> > they will match against extensions of the target string as well as the
> > target string itself.
> >
> 
> strNcmp compare N bytes - http://lxr.linux.no/#linux+v3.5/lib/string.c#L270
> memcmp compare N bytes  - http://lxr.linux.no/#linux+v3.5/lib/string.c#L651

Yes.  So if s contains "abcde" then

	memcmp(s, "abc", 3) and strncmp(s, "abc", 3) will both return 0, and
	memcmp(s, "abc", 4) and strncmp(s, "abc", 4) will both return 1.

Alan Stern

Re: [linux-pm] [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Alan Stern]

On Mon, 6 Aug 2012, Pavel Vasilyev wrote:

> >> http://www.gossamer-threads.com/lists/engine?do=post_attachment;postatt_id=41157;list=linux
> >
> > Interestingly, many (all?) of the changes in that patch are wrong
> > because they don't try to match the terminating '\0'.  As a result,
> > they will match against extensions of the target string as well as the
> > target string itself.
> >
> 
> strNcmp compare N bytes - http://lxr.linux.no/#linux+v3.5/lib/string.c#L270
> memcmp compare N bytes  - http://lxr.linux.no/#linux+v3.5/lib/string.c#L651

Yes.  So if s contains "abcde" then

	memcmp(s, "abc", 3) and strncmp(s, "abc", 3) will both return 0, and
	memcmp(s, "abc", 4) and strncmp(s, "abc", 4) will both return 1.

Alan Stern

RE: [linux-pm] [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Daniel Taylor]

Silly question:  when did sizeof("string") get changed to be anything
other than the size of the pointer ("string" is, after all, an array
of characters)? 

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org 
> [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Alan Stern
> Sent: Monday, August 06, 2012 1:00 PM
> To: Pavel Vasilyev
> Cc: Pavel Machek; Len Brown; linux-acpi@vger.kernel.org; 
> linux-pm@lists.linux-foundation.org; 
> linux-kernel@vger.kernel.org; Len Brown
> Subject: Re: [linux-pm] [PATCH] ACPI: replace 
> strlen("string") with sizeof("string") -1
> 
> On Mon, 6 Aug 2012, Pavel Vasilyev wrote:
> 
> > >> 
> http://www.gossamer-threads.com/lists/engine?do=post_attachmen
> t;postatt_id=41157;list=linux
> > >
> > > Interestingly, many (all?) of the changes in that patch are wrong
> > > because they don't try to match the terminating '\0'.  As 
> a result,
> > > they will match against extensions of the target string 
> as well as the
> > > target string itself.
> > >
> > 
> > strNcmp compare N bytes - 
> http://lxr.linux.no/#linux+v3.5/lib/string.c#L270
> > memcmp compare N bytes  - 
> http://lxr.linux.no/#linux+v3.5/lib/string.c#L651
> 
> Yes.  So if s contains "abcde" then
> 
> 	memcmp(s, "abc", 3) and strncmp(s, "abc", 3) will both 
> return 0, and
> 	memcmp(s, "abc", 4) and strncmp(s, "abc", 4) will both return 1.
> 
> Alan Stern
> 
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

RE: [linux-pm] [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Daniel Taylor]

Silly question:  when did sizeof("string") get changed to be anything
other than the size of the pointer ("string" is, after all, an array
of characters)? 

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org 
> [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Alan Stern
> Sent: Monday, August 06, 2012 1:00 PM
> To: Pavel Vasilyev
> Cc: Pavel Machek; Len Brown; linux-acpi@vger.kernel.org; 
> linux-pm@lists.linux-foundation.org; 
> linux-kernel@vger.kernel.org; Len Brown
> Subject: Re: [linux-pm] [PATCH] ACPI: replace 
> strlen("string") with sizeof("string") -1
> 
> On Mon, 6 Aug 2012, Pavel Vasilyev wrote:
> 
> > >> 
> http://www.gossamer-threads.com/lists/engine?do=post_attachmen
> t;postatt_id=41157;list=linux
> > >
> > > Interestingly, many (all?) of the changes in that patch are wrong
> > > because they don't try to match the terminating '\0'.  As 
> a result,
> > > they will match against extensions of the target string 
> as well as the
> > > target string itself.
> > >
> > 
> > strNcmp compare N bytes - 
> http://lxr.linux.no/#linux+v3.5/lib/string.c#L270
> > memcmp compare N bytes  - 
> http://lxr.linux.no/#linux+v3.5/lib/string.c#L651
> 
> Yes.  So if s contains "abcde" then
> 
> 	memcmp(s, "abc", 3) and strncmp(s, "abc", 3) will both 
> return 0, and
> 	memcmp(s, "abc", 4) and strncmp(s, "abc", 4) will both return 1.
> 
> Alan Stern
> 
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [linux-pm] [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Pavel Vasilyev]

06.08.2012 23:59, Alan Stern пишет:
> On Mon, 6 Aug 2012, Pavel Vasilyev wrote:
>
>>>> http://www.gossamer-threads.com/lists/engine?do=post_attachment;postatt_id=41157;list=linux
>>>
>>> Interestingly, many (all?) of the changes in that patch are wrong
>>> because they don't try to match the terminating '\0'.  As a result,
>>> they will match against extensions of the target string as well as the
>>> target string itself.
>>>
>>
>> strNcmp compare N bytes - http://lxr.linux.no/#linux+v3.5/lib/string.c#L270
>> memcmp compare N bytes  - http://lxr.linux.no/#linux+v3.5/lib/string.c#L651
>
> Yes.  So if s contains "abcde" then
>
> 	memcmp(s, "abc", 3) and strncmp(s, "abc", 3) will both return 0, and
> 	memcmp(s, "abc", 4) and strncmp(s, "abc", 4) will both return 1.

No matter what is contained in *s, "abcde" or "abcxxx",
are important first N bytes. The second example, you see,
a little bit stupid, and devoid of logic. :)


-- 

                                                          Pavel.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux-pm] [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Pavel Vasilyev]

06.08.2012 23:59, Alan Stern пишет:
> On Mon, 6 Aug 2012, Pavel Vasilyev wrote:
>
>>>> http://www.gossamer-threads.com/lists/engine?do=post_attachment;postatt_id=41157;list=linux
>>>
>>> Interestingly, many (all?) of the changes in that patch are wrong
>>> because they don't try to match the terminating '\0'.  As a result,
>>> they will match against extensions of the target string as well as the
>>> target string itself.
>>>
>>
>> strNcmp compare N bytes - http://lxr.linux.no/#linux+v3.5/lib/string.c#L270
>> memcmp compare N bytes  - http://lxr.linux.no/#linux+v3.5/lib/string.c#L651
>
> Yes.  So if s contains "abcde" then
>
> 	memcmp(s, "abc", 3) and strncmp(s, "abc", 3) will both return 0, and
> 	memcmp(s, "abc", 4) and strncmp(s, "abc", 4) will both return 1.

No matter what is contained in *s, "abcde" or "abcxxx",
are important first N bytes. The second example, you see,
a little bit stupid, and devoid of logic. :)


-- 

                                                          Pavel.

Re: [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Bernd Petrovitsch]

On Mon, 2012-08-06 at 22:57 +0000, Daniel Taylor wrote:
> Silly question:  when did sizeof("string") get changed to be anything
> other than the size of the pointer ("string" is, after all, an array
> of characters)? 

It is since K&R times that way.
If you do not know the difference between a pointer and an array (and
these are vastly different), go learn something new about C.

	Bernd
-- 
Bernd Petrovitsch                  Email : bernd@petrovitsch.priv.at
                     LUGA : http://www.luga.at

RE: [linux-pm] [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Bernd Petrovitsch]

On Mon, 2012-08-06 at 22:57 +0000, Daniel Taylor wrote:
> Silly question:  when did sizeof("string") get changed to be anything
> other than the size of the pointer ("string" is, after all, an array
> of characters)? 

It is since K&R times that way.
If you do not know the difference between a pointer and an array (and
these are vastly different), go learn something new about C.

	Bernd
-- 
Bernd Petrovitsch                  Email : bernd@petrovitsch.priv.at
                     LUGA : http://www.luga.at

Re: [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Alan Stern]

On Tue, 7 Aug 2012, Pavel Vasilyev wrote:

> 06.08.2012 23:59, Alan Stern пишет:
> > On Mon, 6 Aug 2012, Pavel Vasilyev wrote:
> >
> >>>> http://www.gossamer-threads.com/lists/engine?do=post_attachment;postatt_id=41157;list=linux
> >>>
> >>> Interestingly, many (all?) of the changes in that patch are wrong
> >>> because they don't try to match the terminating '\0'.  As a result,
> >>> they will match against extensions of the target string as well as the
> >>> target string itself.
> >>>
> >>
> >> strNcmp compare N bytes - http://lxr.linux.no/#linux+v3.5/lib/string.c#L270
> >> memcmp compare N bytes  - http://lxr.linux.no/#linux+v3.5/lib/string.c#L651
> >
> > Yes.  So if s contains "abcde" then
> >
> > 	memcmp(s, "abc", 3) and strncmp(s, "abc", 3) will both return 0, and
> > 	memcmp(s, "abc", 4) and strncmp(s, "abc", 4) will both return 1.
> 
> No matter what is contained in *s, "abcde" or "abcxxx",
> are important first N bytes. The second example, you see,
> a little bit stupid, and devoid of logic. :)

Maybe yes, maybe no.  It all depends on what you want.

For example, if you're looking for "on" or "off", what should you do
when the user writes "onoff"?  You could accept it as meaning the same
as "on", but if you were being careful then you would want to reject it
as a meaningless value.

Alan Stern

Re: [linux-pm] [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Alan Stern]

On Tue, 7 Aug 2012, Pavel Vasilyev wrote:

> 06.08.2012 23:59, Alan Stern пишет:
> > On Mon, 6 Aug 2012, Pavel Vasilyev wrote:
> >
> >>>> http://www.gossamer-threads.com/lists/engine?do=post_attachment;postatt_id=41157;list=linux
> >>>
> >>> Interestingly, many (all?) of the changes in that patch are wrong
> >>> because they don't try to match the terminating '\0'.  As a result,
> >>> they will match against extensions of the target string as well as the
> >>> target string itself.
> >>>
> >>
> >> strNcmp compare N bytes - http://lxr.linux.no/#linux+v3.5/lib/string.c#L270
> >> memcmp compare N bytes  - http://lxr.linux.no/#linux+v3.5/lib/string.c#L651
> >
> > Yes.  So if s contains "abcde" then
> >
> > 	memcmp(s, "abc", 3) and strncmp(s, "abc", 3) will both return 0, and
> > 	memcmp(s, "abc", 4) and strncmp(s, "abc", 4) will both return 1.
> 
> No matter what is contained in *s, "abcde" or "abcxxx",
> are important first N bytes. The second example, you see,
> a little bit stupid, and devoid of logic. :)

Maybe yes, maybe no.  It all depends on what you want.

For example, if you're looking for "on" or "off", what should you do
when the user writes "onoff"?  You could accept it as meaning the same
as "on", but if you were being careful then you would want to reject it
as a meaningless value.

Alan Stern

Re: [linux-pm] [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Pavel Vasilyev]

07.08.2012 21:24, Alan Stern пишет:
> On Tue, 7 Aug 2012, Pavel Vasilyev wrote:
>
>> 06.08.2012 23:59, Alan Stern пишет:
>>> On Mon, 6 Aug 2012, Pavel Vasilyev wrote:
>>>
>>>>>> http://www.gossamer-threads.com/lists/engine?do=post_attachment;postatt_id=41157;list=linux
>>>>>
>>>>> Interestingly, many (all?) of the changes in that patch are wrong
>>>>> because they don't try to match the terminating '\0'.  As a result,
>>>>> they will match against extensions of the target string as well as the
>>>>> target string itself.
>>>>>
>>>>
>>>> strNcmp compare N bytes - http://lxr.linux.no/#linux+v3.5/lib/string.c#L270
>>>> memcmp compare N bytes  - http://lxr.linux.no/#linux+v3.5/lib/string.c#L651
>>>
>>> Yes.  So if s contains "abcde" then
>>>
>>> 	memcmp(s, "abc", 3) and strncmp(s, "abc", 3) will both return 0, and
>>> 	memcmp(s, "abc", 4) and strncmp(s, "abc", 4) will both return 1.
>>
>> No matter what is contained in *s, "abcde" or "abcxxx",
>> are important first N bytes. The second example, you see,
>> a little bit stupid, and devoid of logic. :)
>
> Maybe yes, maybe no.  It all depends on what you want.
>
> For example, if you're looking for "on" or "off", what should you do
> when the user writes "onoff"?  You could accept it as meaning the same
> as "on", but if you were being careful then you would want to reject it
> as a meaningless value.


The users should't be allowed to think!
There is "on" - the size of 2 bytes, or "off" - 3 bytes,
other variations - user error.

We do not create a kernel with artificial intelligence? ;)

-- 
                                                          Pavel.

RE: [linux-pm] [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Daniel Taylor]

Said it was a silly question.

It's funny.

I've been using "0123456789abcdef"[index] for a long time, so I "know"
that "string" is a array of char, but it never occurred to me that
"string" would work in sizeof() the same way as

char string[] = { '0', '1', '2', '3', '4', '5', '6', '7',
                  '8', '9', 'a', 'b', 'c', 'd', 'e', 'f', '\0' };

int stringlength = sizeof(string);

Learned something.

Thanks,

Dan
 

> -----Original Message-----
> From: Bernd Petrovitsch [mailto:bernd@petrovitsch.priv.at] 
> Sent: Tuesday, August 07, 2012 6:20 AM
> To: Daniel Taylor
> Cc: 'Alan Stern'; Pavel Vasilyev; Pavel Machek; Len Brown; 
> linux-acpi@vger.kernel.org; 
> linux-pm@lists.linux-foundation.org; 
> linux-kernel@vger.kernel.org; Len Brown
> Subject: RE: [linux-pm] [PATCH] ACPI: replace 
> strlen("string") with sizeof("string") -1
> 
> On Mon, 2012-08-06 at 22:57 +0000, Daniel Taylor wrote:
> > Silly question:  when did sizeof("string") get changed to 
> be anything
> > other than the size of the pointer ("string" is, after all, an array
> > of characters)? 
> 
> It is since K&R times that way.
> If you do not know the difference between a pointer and an array (and
> these are vastly different), go learn something new about C.
> 
> 	Bernd
> -- 
> Bernd Petrovitsch                  Email : bernd@petrovitsch.priv.at
>                      LUGA : http://www.luga.at
> 
> 

RE: [linux-pm] [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Daniel Taylor]

Said it was a silly question.

It's funny.

I've been using "0123456789abcdef"[index] for a long time, so I "know"
that "string" is a array of char, but it never occurred to me that
"string" would work in sizeof() the same way as

char string[] = { '0', '1', '2', '3', '4', '5', '6', '7',
                  '8', '9', 'a', 'b', 'c', 'd', 'e', 'f', '\0' };

int stringlength = sizeof(string);

Learned something.

Thanks,

Dan
 

> -----Original Message-----
> From: Bernd Petrovitsch [mailto:bernd@petrovitsch.priv.at] 
> Sent: Tuesday, August 07, 2012 6:20 AM
> To: Daniel Taylor
> Cc: 'Alan Stern'; Pavel Vasilyev; Pavel Machek; Len Brown; 
> linux-acpi@vger.kernel.org; 
> linux-pm@lists.linux-foundation.org; 
> linux-kernel@vger.kernel.org; Len Brown
> Subject: RE: [linux-pm] [PATCH] ACPI: replace 
> strlen("string") with sizeof("string") -1
> 
> On Mon, 2012-08-06 at 22:57 +0000, Daniel Taylor wrote:
> > Silly question:  when did sizeof("string") get changed to 
> be anything
> > other than the size of the pointer ("string" is, after all, an array
> > of characters)? 
> 
> It is since K&R times that way.
> If you do not know the difference between a pointer and an array (and
> these are vastly different), go learn something new about C.
> 
> 	Bernd
> -- 
> Bernd Petrovitsch                  Email : bernd@petrovitsch.priv.at
>                      LUGA : http://www.luga.at
> 
> 

Re: [linux-pm] [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Alan Stern]

On Wed, 8 Aug 2012, Pavel Vasilyev wrote:

> >>> Yes.  So if s contains "abcde" then
> >>>
> >>> 	memcmp(s, "abc", 3) and strncmp(s, "abc", 3) will both return 0, and
> >>> 	memcmp(s, "abc", 4) and strncmp(s, "abc", 4) will both return 1.
> >>
> >> No matter what is contained in *s, "abcde" or "abcxxx",
> >> are important first N bytes. The second example, you see,
> >> a little bit stupid, and devoid of logic. :)
> >
> > Maybe yes, maybe no.  It all depends on what you want.
> >
> > For example, if you're looking for "on" or "off", what should you do
> > when the user writes "onoff"?  You could accept it as meaning the same
> > as "on", but if you were being careful then you would want to reject it
> > as a meaningless value.
> 
> 
> The users should't be allowed to think!
> There is "on" - the size of 2 bytes, or "off" - 3 bytes,
> other variations - user error.
> 
> We do not create a kernel with artificial intelligence? ;)

Let me rephrase the previous statement, as it appears you did not 
understand what I meant.

If the kernel is testing for "on" or "off", what should it do when the
user writes "onoff"?  The kernel could accept this as meaning the same
as "on", but if the kernel was being careful then it should reject
"onoff" as a meaningless value.  A 2-byte comparison for "on" would
accept "onoff" whereas a 3-byte comparison would not.

Alan Stern

Re: [linux-pm] [PATCH] ACPI: replace strlen("string") with sizeof("string") -1

[Alan Stern]

On Wed, 8 Aug 2012, Pavel Vasilyev wrote:

> >>> Yes.  So if s contains "abcde" then
> >>>
> >>> 	memcmp(s, "abc", 3) and strncmp(s, "abc", 3) will both return 0, and
> >>> 	memcmp(s, "abc", 4) and strncmp(s, "abc", 4) will both return 1.
> >>
> >> No matter what is contained in *s, "abcde" or "abcxxx",
> >> are important first N bytes. The second example, you see,
> >> a little bit stupid, and devoid of logic. :)
> >
> > Maybe yes, maybe no.  It all depends on what you want.
> >
> > For example, if you're looking for "on" or "off", what should you do
> > when the user writes "onoff"?  You could accept it as meaning the same
> > as "on", but if you were being careful then you would want to reject it
> > as a meaningless value.
> 
> 
> The users should't be allowed to think!
> There is "on" - the size of 2 bytes, or "off" - 3 bytes,
> other variations - user error.
> 
> We do not create a kernel with artificial intelligence? ;)

Let me rephrase the previous statement, as it appears you did not 
understand what I meant.

If the kernel is testing for "on" or "off", what should it do when the
user writes "onoff"?  The kernel could accept this as meaning the same
as "on", but if the kernel was being careful then it should reject
"onoff" as a meaningless value.  A 2-byte comparison for "on" would
accept "onoff" whereas a 3-byte comparison would not.

Alan Stern