Input: pmic8xxx-pwrkey - Set sane default for debounce time

PATCH Applied Stephen Boyd 3 participants 6 messages Mar 31, 2014 – May 14, 2014

Subsystems (3)

Arm Qualcomm mailing list HID Input drivers

lore

Share & tools

lore

Stephen Boyd

author

2014-03-31 18:14 UTC #0 lore raw

PATCH 1/1 Input: pmic8xxx-pwrkey - Set sane default for debounce time

If the debounce time is 0 our usage of ilog2() later on in this driver will cause undefined behavior. If CONFIG_OF=n this fact is evident to the compiler, and it emits a call to ____ilog2_NaN() which doesn't exist. Fix this by setting a sane default for debounce.

Reported-by Arnd Bergmann <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 drivers/input/misc/pmic8xxx-pwrkey.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/input/misc/pmic8xxx-pwrkey.c +1 -1

index 1cb8fda7a166..27add04676e1 100644
@@ -92,7 +92,7 @@ static int pmic8xxx_pwrkey_probe(struct platform_device *pdev)
 	bool pull_up;
 
 	if (of_property_read_u32(pdev->dev.of_node, "debounce", &kpd_delay))
-		kpd_delay = 0;
+		kpd_delay = 15625;
 
 	pull_up = of_property_read_bool(pdev->dev.of_node, "pull-up");

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Dmitry Torokhov

maintainer

2014-03-31 18:23 UTC #1 lore raw

Quoted 17 lines · Stephen Boyd

If the debounce time is 0 our usage of ilog2() later on in this driver will cause undefined behavior. If CONFIG_OF=n this fact is evident to the compiler, and it emits a call to ____ilog2_NaN() which doesn't exist. Fix this by setting a sane default for debounce.
Reported-by Arnd Bergmann <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
 drivers/input/misc/pmic8xxx-pwrkey.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/input/misc/pmic8xxx-pwrkey.c +0 -0
index 1cb8fda7a166..27add04676e1 100644

drivers/input/misc/pmic8xxx-pwrkey.c static int pmic8xxx_pwrkey_probe(struct platform_device *pdev) · line 95

Quoted 5 lines

drivers/input/misc/pmic8xxx-pwrkey.c +1 -1

@@ -92,7 +92,7 @@ static int pmic8xxx_pwrkey_probe(struct platform_device *pdev)
 	bool pull_up;
 
 	if (of_property_read_u32(pdev->dev.of_node, "debounce", &kpd_delay))
-		kpd_delay = 0;
+		kpd_delay = 15625;

What if somebody supplied 0 via DT? Can we check and return -EINVAL?

Quoted 7 lines

drivers/input/misc/pmic8xxx-pwrkey.c +0 -1
 	pull_up = of_property_read_bool(pdev->dev.of_node, "pull-up");
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation

Signature · 1 line

Dmitry

Stephen Boyd

author

2014-03-31 18:29 UTC #2 lore raw

Quoted 20 lines · Dmitry Torokhov

Quoted 19 lines · Stephen Boyd
If the debounce time is 0 our usage of ilog2() later on in this driver will cause undefined behavior. If CONFIG_OF=n this fact is evident to the compiler, and it emits a call to ____ilog2_NaN() which doesn't exist. Fix this by setting a sane default for debounce.
Reported-by Arnd Bergmann <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
 drivers/input/misc/pmic8xxx-pwrkey.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/input/misc/pmic8xxx-pwrkey.c +0 -0
index 1cb8fda7a166..27add04676e1 100644
@@ -92,7 +92,7 @@ static int pmic8xxx_pwrkey_probe(struct platform_device *pdev)
 	bool pull_up;
 

Quoted 4 lines

drivers/input/misc/pmic8xxx-pwrkey.c static int pmic8xxx_pwrkey_probe(struct platform_device *pdev) · line 95
Quoted 3 lines
drivers/input/misc/pmic8xxx-pwrkey.c +1 -1
 	if (of_property_read_u32(pdev->dev.of_node, "debounce", &kpd_delay))
-		kpd_delay = 0;
+		kpd_delay = 15625;
What if somebody supplied 0 via DT? Can we check and return -EINVAL?

Sure. Here's a v2.

-----8<------
From: Stephen Boyd <[email protected]>
Subject: [PATCH v2] Input: pmic8xxx-pwrkey - Set sane default for debounce
time

Reported-by Arnd Bergmann <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 drivers/input/misc/pmic8xxx-pwrkey.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/input/misc/pmic8xxx-pwrkey.c +2 -2

index 1cb8fda7a166..3fb161902cc6 100644
@@ -92,11 +92,11 @@ static int pmic8xxx_pwrkey_probe(struct platform_device *pdev)
 	bool pull_up;
 
 	if (of_property_read_u32(pdev->dev.of_node, "debounce", &kpd_delay))
-		kpd_delay = 0;
+		kpd_delay = 15625;
 
 	pull_up = of_property_read_bool(pdev->dev.of_node, "pull-up");
 
-	if (kpd_delay > 62500) {
+	if (kpd_delay > 62500 || kpd_delay == 0) {
 		dev_err(&pdev->dev, "invalid power key trigger delay\n");
 		return -EINVAL;
 	}

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Dmitry Torokhov

maintainer Applied

2014-05-14 19:02 UTC #3 lore raw

Quoted 33 lines · Stephen Boyd

Quoted 20 lines · Dmitry Torokhov
Quoted 19 lines · Stephen Boyd
If the debounce time is 0 our usage of ilog2() later on in this driver will cause undefined behavior. If CONFIG_OF=n this fact is evident to the compiler, and it emits a call to ____ilog2_NaN() which doesn't exist. Fix this by setting a sane default for debounce.
Reported-by Arnd Bergmann <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
 drivers/input/misc/pmic8xxx-pwrkey.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/input/misc/pmic8xxx-pwrkey.c +0 -0
index 1cb8fda7a166..27add04676e1 100644
@@ -92,7 +92,7 @@ static int pmic8xxx_pwrkey_probe(struct platform_device *pdev)
 	bool pull_up;
 
Quoted 4 lines
drivers/input/misc/pmic8xxx-pwrkey.c static int pmic8xxx_pwrkey_probe(struct platform_device *pdev) · line 95
Quoted 3 lines
drivers/input/misc/pmic8xxx-pwrkey.c +1 -1
 	if (of_property_read_u32(pdev->dev.of_node, "debounce", &kpd_delay))
-		kpd_delay = 0;
+		kpd_delay = 15625;
What if somebody supplied 0 via DT? Can we check and return -EINVAL?
Sure. Here's a v2.
-----8<------
From: Stephen Boyd <[email protected]>
Subject: [PATCH v2] Input: pmic8xxx-pwrkey - Set sane default for debounce
time

Quoted 8 lines

If the debounce time is 0 our usage of ilog2() later on in this driver will cause undefined behavior. If CONFIG_OF=n this fact is evident to the compiler, and it emits a call to ____ilog2_NaN() which doesn't exist. Fix this by setting a sane default for debounce and failing to probe if debounce is 0 in the DT.
Reported-by Arnd Bergmann <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>

Applied, thank you. Sorry for the delay.

Signature · 1 line

Dmitry

Josh Cartwright

2014-03-31 18:23 UTC #4 lore raw

In reply to Stephen Boyd

Quoted 17 lines · Stephen Boyd

If the debounce time is 0 our usage of ilog2() later on in this driver will cause undefined behavior. If CONFIG_OF=n this fact is evident to the compiler, and it emits a call to ____ilog2_NaN() which doesn't exist. Fix this by setting a sane default for debounce.
Reported-by Arnd Bergmann <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
 drivers/input/misc/pmic8xxx-pwrkey.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/input/misc/pmic8xxx-pwrkey.c +0 -0
index 1cb8fda7a166..27add04676e1 100644

drivers/input/misc/pmic8xxx-pwrkey.c static int pmic8xxx_pwrkey_probe(struct platform_device *pdev) · line 95

Quoted 5 lines

drivers/input/misc/pmic8xxx-pwrkey.c +1 -1

@@ -92,7 +92,7 @@ static int pmic8xxx_pwrkey_probe(struct platform_device *pdev)
 	bool pull_up;
 
 	if (of_property_read_u32(pdev->dev.of_node, "debounce", &kpd_delay))
-		kpd_delay = 0;
+		kpd_delay = 15625;

Should "debounce" even be optional? I'm wondering if we should just make it required...

At the very least this default value should be documented in the DT binding.

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Stephen Boyd

author

2014-03-31 18:48 UTC #5 lore raw

Quoted 20 lines · Josh Cartwright

Quoted 19 lines · Stephen Boyd
If the debounce time is 0 our usage of ilog2() later on in this driver will cause undefined behavior. If CONFIG_OF=n this fact is evident to the compiler, and it emits a call to ____ilog2_NaN() which doesn't exist. Fix this by setting a sane default for debounce.
Reported-by Arnd Bergmann <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
 drivers/input/misc/pmic8xxx-pwrkey.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/input/misc/pmic8xxx-pwrkey.c +0 -0
index 1cb8fda7a166..27add04676e1 100644
@@ -92,7 +92,7 @@ static int pmic8xxx_pwrkey_probe(struct platform_device *pdev)
 	bool pull_up;
 

Quoted 5 lines

drivers/input/misc/pmic8xxx-pwrkey.c static int pmic8xxx_pwrkey_probe(struct platform_device *pdev) · line 95
Quoted 3 lines
drivers/input/misc/pmic8xxx-pwrkey.c +1 -1
 	if (of_property_read_u32(pdev->dev.of_node, "debounce", &kpd_delay))
-		kpd_delay = 0;
+		kpd_delay = 15625;
Should "debounce" even be optional? I'm wondering if we should just make it required...

Hmm maybe. It's not like things won't work without it set though.

Quoted 4 lines

At the very least this default value should be documented in the DT binding.

I never understood this. The default value is something that is entirely software driven. It could change at any time. If DT implementers aren't specifying it they're saying "I don't care what the value is, let the driver decide if they care". They're not saying, "Oh, that default value in the binding is good enough for me so I don't need to specify this".

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation