This change series includes support and fixes in pm8941-pwrkey.c.
Change details and description can be found in each patch. Thanks!
David Collins (3):
input: misc: pm8941-pwrkey: simulate missed key press events
input: misc: pm8941-pwrkey: add software key press debouncing support
input: misc: pm8941-pwrkey: avoid potential null pointer dereference
drivers/input/misc/pm8941-pwrkey.c | 130 ++++++++++++++++++++++++++---
1 file changed, 120 insertions(+), 10 deletions(-)
--
2.34.1
From: David Collins <[email protected]>
The status of the keys connected to the KPDPWR_N and RESIN_N pins
is identified by reading corresponding bits in the interrupt real
time status register. If the status has changed by the time that
the interrupt is handled then a press event will be missed.
Maintain a last known status variable to find unbalanced release
events and simulate press events for each accordingly.
Change-Id: I180469a7048a40e490f21d0f8b9504136131e75b
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Anjelique Melendez <[email protected]>
---
drivers/input/misc/pm8941-pwrkey.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
index b912ce00ce1c..0ce00736e695 100644
--- a/drivers/input/misc/pm8941-pwrkey.c
+++ b/drivers/input/misc/pm8941-pwrkey.c
@@ -77,6 +77,7 @@ struct pm8941_pwrkey {
u32 code;
u32 sw_debounce_time_us;
ktime_t last_release_time;
+ bool last_status;
const struct pm8941_data *data;
};
@@ -166,6 +167,16 @@ static irqreturn_t pm8941_pwrkey_irq(int irq, void *_data)
if (pwrkey->sw_debounce_time_us && !sts)
pwrkey->last_release_time = ktime_get();
+ /*
+ * Simulate a press event in case a release event occurred without a
+ * corresponding press event.
+ */
+ if (!pwrkey->last_status && !sts) {
+ input_report_key(pwrkey->input, pwrkey->code, 1);
+ input_sync(pwrkey->input);
+ }
+ pwrkey->last_status = sts;
+
input_report_key(pwrkey->input, pwrkey->code, sts);
input_sync(pwrkey->input);
--
2.34.1
From: David Collins <[email protected]>
Add a null check for the pwrkey->data pointer after it is assigned
in pm8941_pwrkey_probe(). This avoids a potential null pointer
dereference when pwrkey->data->has_pon_pbs is accessed later in
the probe function.
Change-Id: I589c4851e544d79a1863fd110b32a0b45ac03caf
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Anjelique Melendez <[email protected]>
---
drivers/input/misc/pm8941-pwrkey.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
index 0ce00736e695..ac08ed025802 100644
--- a/drivers/input/misc/pm8941-pwrkey.c
+++ b/drivers/input/misc/pm8941-pwrkey.c
@@ -263,6 +263,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
pwrkey->dev = &pdev->dev;
pwrkey->data = of_device_get_match_data(&pdev->dev);
+ if (!pwrkey->data) {
+ dev_err(&pdev->dev, "match data not found\n");
+ return -ENODEV;
+ }
parent = pdev->dev.parent;
regmap_node = pdev->dev.of_node;
--
2.34.1
From: David Collins <[email protected]>
On certain PMICs, an unexpected assertion of KPDPWR_DEB (the
positive logic hardware debounced power key signal) may be seen
during the falling edge of KPDPWR_N (i.e. a power key press) when
it occurs close to the rising edge of SLEEP_CLK. This then
triggers a spurious KPDPWR interrupt.
Handle this issue by adding software debouncing support to ignore
key events that occur within the hardware debounce delay after the
most recent key release event.
Change-Id: Ifa3809935c01aab9078ba2302397bc9ebf390021
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Anjelique Melendez <[email protected]>
---
drivers/input/misc/pm8941-pwrkey.c | 115 ++++++++++++++++++++++++++---
1 file changed, 105 insertions(+), 10 deletions(-)
diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
index 33609603245d..b912ce00ce1c 100644
--- a/drivers/input/misc/pm8941-pwrkey.c
+++ b/drivers/input/misc/pm8941-pwrkey.c
@@ -9,9 +9,11 @@
#include <linux/input.h>
#include <linux/interrupt.h>
#include <linux/kernel.h>
+#include <linux/ktime.h>
#include <linux/log2.h>
#include <linux/module.h>
#include <linux/of.h>
+#include <linux/of_address.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/reboot.h>
@@ -19,6 +21,16 @@
#define PON_REV2 0x01
+#define PON_SUBTYPE 0x05
+
+#define PON_SUBTYPE_PRIMARY 0x01
+#define PON_SUBTYPE_SECONDARY 0x02
+#define PON_SUBTYPE_1REG 0x03
+#define PON_SUBTYPE_GEN2_PRIMARY 0x04
+#define PON_SUBTYPE_GEN2_SECONDARY 0x05
+#define PON_SUBTYPE_GEN3_PBS 0x08
+#define PON_SUBTYPE_GEN3_HLOS 0x09
+
#define PON_RT_STS 0x10
#define PON_KPDPWR_N_SET BIT(0)
#define PON_RESIN_N_SET BIT(1)
@@ -44,6 +56,8 @@ struct pm8941_data {
unsigned int status_bit;
bool supports_ps_hold_poff_config;
bool supports_debounce_config;
+ bool needs_sw_debounce;
+ bool has_pon_pbs;
const char *name;
const char *phys;
};
@@ -52,13 +66,17 @@ struct pm8941_pwrkey {
struct device *dev;
int irq;
u32 baseaddr;
+ u32 pon_pbs_baseaddr;
struct regmap *regmap;
struct input_dev *input;
unsigned int revision;
+ unsigned int subtype;
struct notifier_block reboot_notifier;
u32 code;
+ u32 sw_debounce_time_us;
+ ktime_t last_release_time;
const struct pm8941_data *data;
};
@@ -126,19 +144,65 @@ static irqreturn_t pm8941_pwrkey_irq(int irq, void *_data)
struct pm8941_pwrkey *pwrkey = _data;
unsigned int sts;
int error;
+ u64 elapsed_us;
+
+ if (pwrkey->sw_debounce_time_us) {
+ elapsed_us = ktime_us_delta(ktime_get(),
+ pwrkey->last_release_time);
+ if (elapsed_us < pwrkey->sw_debounce_time_us) {
+ dev_dbg(pwrkey->dev, "ignoring key event received after %llu us, debounce time=%u us\n",
+ elapsed_us, pwrkey->sw_debounce_time_us);
+ return IRQ_HANDLED;
+ }
+ }
error = regmap_read(pwrkey->regmap,
pwrkey->baseaddr + PON_RT_STS, &sts);
if (error)
return IRQ_HANDLED;
- input_report_key(pwrkey->input, pwrkey->code,
- sts & pwrkey->data->status_bit);
+ sts &= pwrkey->data->status_bit;
+
+ if (pwrkey->sw_debounce_time_us && !sts)
+ pwrkey->last_release_time = ktime_get();
+
+ input_report_key(pwrkey->input, pwrkey->code, sts);
input_sync(pwrkey->input);
return IRQ_HANDLED;
}
+static int pm8941_pwrkey_sw_debounce_init(struct pm8941_pwrkey *pwrkey)
+{
+ unsigned int val, addr;
+ int error;
+
+ if (pwrkey->data->has_pon_pbs && !pwrkey->pon_pbs_baseaddr) {
+ dev_err(pwrkey->dev, "PON_PBS address missing, can't read HW debounce time\n");
+ return 0;
+ }
+
+ if (pwrkey->pon_pbs_baseaddr)
+ addr = pwrkey->pon_pbs_baseaddr + PON_DBC_CTL;
+ else
+ addr = pwrkey->baseaddr + PON_DBC_CTL;
+ error = regmap_read(pwrkey->regmap, addr, &val);
+ if (error)
+ return error;
+
+ if (pwrkey->subtype >= PON_SUBTYPE_GEN2_PRIMARY)
+ pwrkey->sw_debounce_time_us = 2 * USEC_PER_SEC /
+ (1 << (0xf - (val & 0xf)));
+ else
+ pwrkey->sw_debounce_time_us = 2 * USEC_PER_SEC /
+ (1 << (0x7 - (val & 0x7)));
+
+ dev_dbg(pwrkey->dev, "SW debounce time = %u us\n",
+ pwrkey->sw_debounce_time_us);
+
+ return 0;
+}
+
static int __maybe_unused pm8941_pwrkey_suspend(struct device *dev)
{
struct pm8941_pwrkey *pwrkey = dev_get_drvdata(dev);
@@ -167,6 +231,8 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
struct pm8941_pwrkey *pwrkey;
bool pull_up;
struct device *parent;
+ struct device_node *regmap_node;
+ const __be32 *addr;
u32 req_delay;
int error;
@@ -188,8 +254,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
pwrkey->data = of_device_get_match_data(&pdev->dev);
parent = pdev->dev.parent;
+ regmap_node = pdev->dev.of_node;
pwrkey->regmap = dev_get_regmap(parent, NULL);
if (!pwrkey->regmap) {
+ regmap_node = parent->of_node;
/*
* We failed to get regmap for parent. Let's see if we are
* a child of pon node and read regmap and reg from its
@@ -200,15 +268,21 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
dev_err(&pdev->dev, "failed to locate regmap\n");
return -ENODEV;
}
+ }
- error = of_property_read_u32(parent->of_node,
- "reg", &pwrkey->baseaddr);
- } else {
- error = of_property_read_u32(pdev->dev.of_node, "reg",
- &pwrkey->baseaddr);
+ addr = of_get_address(regmap_node, 0, NULL, NULL);
+ if (!addr) {
+ dev_err(&pdev->dev, "reg property missing\n");
+ return -EINVAL;
+ }
+ pwrkey->baseaddr = be32_to_cpu(*addr);
+
+ if (pwrkey->data->has_pon_pbs) {
+ /* PON_PBS base address is optional */
+ addr = of_get_address(regmap_node, 1, NULL, NULL);
+ if (addr)
+ pwrkey->pon_pbs_baseaddr = be32_to_cpu(*addr);
}
- if (error)
- return error;
pwrkey->irq = platform_get_irq(pdev, 0);
if (pwrkey->irq < 0)
@@ -217,7 +291,14 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
error = regmap_read(pwrkey->regmap, pwrkey->baseaddr + PON_REV2,
&pwrkey->revision);
if (error) {
- dev_err(&pdev->dev, "failed to set debounce: %d\n", error);
+ dev_err(&pdev->dev, "failed to read revision: %d\n", error);
+ return error;
+ }
+
+ error = regmap_read(pwrkey->regmap, pwrkey->baseaddr + PON_SUBTYPE,
+ &pwrkey->subtype);
+ if (error) {
+ dev_err(&pdev->dev, "failed to read subtype: %d\n", error);
return error;
}
@@ -255,6 +336,12 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
}
}
+ if (pwrkey->data->needs_sw_debounce) {
+ error = pm8941_pwrkey_sw_debounce_init(pwrkey);
+ if (error)
+ return error;
+ }
+
if (pwrkey->data->pull_up_bit) {
error = regmap_update_bits(pwrkey->regmap,
pwrkey->baseaddr + PON_PULL_CTL,
@@ -316,6 +403,8 @@ static const struct pm8941_data pwrkey_data = {
.phys = "pm8941_pwrkey/input0",
.supports_ps_hold_poff_config = true,
.supports_debounce_config = true,
+ .needs_sw_debounce = true,
+ .has_pon_pbs = false,
};
static const struct pm8941_data resin_data = {
@@ -325,6 +414,8 @@ static const struct pm8941_data resin_data = {
.phys = "pm8941_resin/input0",
.supports_ps_hold_poff_config = true,
.supports_debounce_config = true,
+ .needs_sw_debounce = true,
+ .has_pon_pbs = false,
};
static const struct pm8941_data pon_gen3_pwrkey_data = {
@@ -333,6 +424,8 @@ static const struct pm8941_data pon_gen3_pwrkey_data = {
.phys = "pmic_pwrkey/input0",
.supports_ps_hold_poff_config = false,
.supports_debounce_config = false,
+ .needs_sw_debounce = true,
+ .has_pon_pbs = true,
};
static const struct pm8941_data pon_gen3_resin_data = {
@@ -341,6 +434,8 @@ static const struct pm8941_data pon_gen3_resin_data = {
.phys = "pmic_resin/input0",
.supports_ps_hold_poff_config = false,
.supports_debounce_config = false,
+ .needs_sw_debounce = true,
+ .has_pon_pbs = true,
};
static const struct of_device_id pm8941_pwr_key_id_table[] = {
--
2.34.1
On 1/20/2022 12:41 PM, Anjelique Melendez wrote:
> From: David Collins <[email protected]>
>
> Add a null check for the pwrkey->data pointer after it is assigned
> in pm8941_pwrkey_probe(). This avoids a potential null pointer
> dereference when pwrkey->data->has_pon_pbs is accessed later in
> the probe function.
>
> Change-Id: I589c4851e544d79a1863fd110b32a0b45ac03caf
> Signed-off-by: David Collins <[email protected]>
> Signed-off-by: Anjelique Melendez <[email protected]>
> ---
> drivers/input/misc/pm8941-pwrkey.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
> index 0ce00736e695..ac08ed025802 100644
> --- a/drivers/input/misc/pm8941-pwrkey.c
> +++ b/drivers/input/misc/pm8941-pwrkey.c
> @@ -263,6 +263,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>
> pwrkey->dev = &pdev->dev;
> pwrkey->data = of_device_get_match_data(&pdev->dev);
> + if (!pwrkey->data) {
> + dev_err(&pdev->dev, "match data not found\n");
> + return -ENODEV;
> + }
>
I don't understand why this patch is 3rd in the series. Isn't it
independent from the debounce time? If not, then why it is not fixed as
part of the patch which adds this debounce time support?
---Trilok Soni
On Thu 20 Jan 12:41 PST 2022, Anjelique Melendez wrote:
> From: David Collins <[email protected]>
>
> Add a null check for the pwrkey->data pointer after it is assigned
> in pm8941_pwrkey_probe(). This avoids a potential null pointer
> dereference when pwrkey->data->has_pon_pbs is accessed later in
> the probe function.
>
> Change-Id: I589c4851e544d79a1863fd110b32a0b45ac03caf
> Signed-off-by: David Collins <[email protected]>
> Signed-off-by: Anjelique Melendez <[email protected]>
> ---
> drivers/input/misc/pm8941-pwrkey.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
> index 0ce00736e695..ac08ed025802 100644
> --- a/drivers/input/misc/pm8941-pwrkey.c
> +++ b/drivers/input/misc/pm8941-pwrkey.c
> @@ -263,6 +263,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>
> pwrkey->dev = &pdev->dev;
> pwrkey->data = of_device_get_match_data(&pdev->dev);
> + if (!pwrkey->data) {
The only way this can happen is if you add a new compatible and forget
to specify data and when that happens you will get a print in the log
somewhere, which once you realize that you don't have your pwrkey you
might be able to find among all the other prints.
If you instead don't NULL check this pointer you will get a large splat
in the log, with callstack and all, immediately hinting you that
pwrkey->data is NULL.
In other words, there's already a print, a much larger print and I don't
think there's value in handling this mistake gracefully.
Regards,
Bjorn
> + dev_err(&pdev->dev, "match data not found\n");
> + return -ENODEV;
> + }
>
> parent = pdev->dev.parent;
> regmap_node = pdev->dev.of_node;
> --
> 2.34.1
>
On 1/20/2022 2:18 PM, Trilok Soni wrote:
> On 1/20/2022 12:41 PM, Anjelique Melendez wrote:
>> From: David Collins <[email protected]>
>>
>> Add a null check for the pwrkey->data pointer after it is assigned
>> in pm8941_pwrkey_probe(). This avoids a potential null pointer
>> dereference when pwrkey->data->has_pon_pbs is accessed later in
>> the probe function.
>>
>> Change-Id: I589c4851e544d79a1863fd110b32a0b45ac03caf
>> Signed-off-by: David Collins <[email protected]>
>> Signed-off-by: Anjelique Melendez <[email protected]>
>> ---
>> drivers/input/misc/pm8941-pwrkey.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
>> index 0ce00736e695..ac08ed025802 100644
>> --- a/drivers/input/misc/pm8941-pwrkey.c
>> +++ b/drivers/input/misc/pm8941-pwrkey.c
>> @@ -263,6 +263,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>> pwrkey->dev = &pdev->dev;
>> pwrkey->data = of_device_get_match_data(&pdev->dev);
>> + if (!pwrkey->data) {
>> + dev_err(&pdev->dev, "match data not found\n");
>> + return -ENODEV;
>> + }
>>
>
> I don't understand why this patch is 3rd in the series. Isn't it independent from the debounce time? If not, then why it is not fixed as part of the patch which adds this debounce time support?
>
> ---Trilok Soni
You are correct that this patch is independent from debounce time. In the following version I will move this patch up to be the first patch!
On 1/20/2022 3:01 PM, Bjorn Andersson wrote:
> On Thu 20 Jan 12:41 PST 2022, Anjelique Melendez wrote:
>
>> From: David Collins <[email protected]>
>>
>> Add a null check for the pwrkey->data pointer after it is assigned
>> in pm8941_pwrkey_probe(). This avoids a potential null pointer
>> dereference when pwrkey->data->has_pon_pbs is accessed later in
>> the probe function.
>>
>> Change-Id: I589c4851e544d79a1863fd110b32a0b45ac03caf
>> Signed-off-by: David Collins <[email protected]>
>> Signed-off-by: Anjelique Melendez <[email protected]>
>> ---
>> drivers/input/misc/pm8941-pwrkey.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
>> index 0ce00736e695..ac08ed025802 100644
>> --- a/drivers/input/misc/pm8941-pwrkey.c
>> +++ b/drivers/input/misc/pm8941-pwrkey.c
>> @@ -263,6 +263,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>>
>> pwrkey->dev = &pdev->dev;
>> pwrkey->data = of_device_get_match_data(&pdev->dev);
>> + if (!pwrkey->data) {
> The only way this can happen is if you add a new compatible and forget
> to specify data and when that happens you will get a print in the log
> somewhere, which once you realize that you don't have your pwrkey you
> might be able to find among all the other prints.
>
> If you instead don't NULL check this pointer you will get a large splat
> in the log, with callstack and all, immediately hinting you that
> pwrkey->data is NULL.
>
>
> In other words, there's already a print, a much larger print and I don't
> think there's value in handling this mistake gracefully.
>
> Regards,
> Bjorn
We would like to the null pointer check in place to avoid static analysis
warnings that can be easily fixed.
>
>> + dev_err(&pdev->dev, "match data not found\n");
>> + return -ENODEV;
>> + }
>>
>> parent = pdev->dev.parent;
>> regmap_node = pdev->dev.of_node;
>> --
>> 2.34.1
>>
"Add support" in the subject sounds like it is new. Maybe "extend
pm8941-pwrkey driver" would be more appropriate.
Quoting Anjelique Melendez (2022-01-20 12:41:30)
> This change series includes support and fixes in pm8941-pwrkey.c.
> Change details and description can be found in each patch. Thanks!
>
> David Collins (3):
> input: misc: pm8941-pwrkey: simulate missed key press events
> input: misc: pm8941-pwrkey: add software key press debouncing support
> input: misc: pm8941-pwrkey: avoid potential null pointer dereference
Quoting Anjelique Melendez (2022-01-20 12:41:33)
> From: David Collins <[email protected]>
>
> On certain PMICs, an unexpected assertion of KPDPWR_DEB (the
> positive logic hardware debounced power key signal) may be seen
> during the falling edge of KPDPWR_N (i.e. a power key press) when
> it occurs close to the rising edge of SLEEP_CLK. This then
> triggers a spurious KPDPWR interrupt.
>
> Handle this issue by adding software debouncing support to ignore
> key events that occur within the hardware debounce delay after the
> most recent key release event.
>
> Change-Id: Ifa3809935c01aab9078ba2302397bc9ebf390021
Please remove change-id when upstreaming.
> Signed-off-by: David Collins <[email protected]>
> Signed-off-by: Anjelique Melendez <[email protected]>
> ---
> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
> index 33609603245d..b912ce00ce1c 100644
> --- a/drivers/input/misc/pm8941-pwrkey.c
> +++ b/drivers/input/misc/pm8941-pwrkey.c
> @@ -126,19 +144,65 @@ static irqreturn_t pm8941_pwrkey_irq(int irq, void *_data)
> struct pm8941_pwrkey *pwrkey = _data;
> unsigned int sts;
> int error;
> + u64 elapsed_us;
> +
> + if (pwrkey->sw_debounce_time_us) {
> + elapsed_us = ktime_us_delta(ktime_get(),
> + pwrkey->last_release_time);
> + if (elapsed_us < pwrkey->sw_debounce_time_us) {
Perhaps storing last_release_time + sw_debounce_time_us via
ktime_add_us() in the struct would be better. Then this line would be
if (ktime_before(debounce_end, ktime_get()))
and we'd avoid a division when converting to microseconds to compare
time.
> + dev_dbg(pwrkey->dev, "ignoring key event received after %llu us, debounce time=%u us\n",
> + elapsed_us, pwrkey->sw_debounce_time_us);
> + return IRQ_HANDLED;
> + }
> + }
>
> error = regmap_read(pwrkey->regmap,
> pwrkey->baseaddr + PON_RT_STS, &sts);
Nitpick: I'd prefer this be on one line. And 'ret' or 'err' be used as
it's shorter.
> if (error)
> return IRQ_HANDLED;
>
> - input_report_key(pwrkey->input, pwrkey->code,
> - sts & pwrkey->data->status_bit);
> + sts &= pwrkey->data->status_bit;
> +
> + if (pwrkey->sw_debounce_time_us && !sts)
> + pwrkey->last_release_time = ktime_get();
> +
> + input_report_key(pwrkey->input, pwrkey->code, sts);
> input_sync(pwrkey->input);
>
> return IRQ_HANDLED;
> }
>
> +static int pm8941_pwrkey_sw_debounce_init(struct pm8941_pwrkey *pwrkey)
> +{
> + unsigned int val, addr;
> + int error;
> +
> + if (pwrkey->data->has_pon_pbs && !pwrkey->pon_pbs_baseaddr) {
> + dev_err(pwrkey->dev, "PON_PBS address missing, can't read HW debounce time\n");
> + return 0;
> + }
> +
> + if (pwrkey->pon_pbs_baseaddr)
> + addr = pwrkey->pon_pbs_baseaddr + PON_DBC_CTL;
> + else
> + addr = pwrkey->baseaddr + PON_DBC_CTL;
> + error = regmap_read(pwrkey->regmap, addr, &val);
> + if (error)
> + return error;
> +
> + if (pwrkey->subtype >= PON_SUBTYPE_GEN2_PRIMARY)
> + pwrkey->sw_debounce_time_us = 2 * USEC_PER_SEC /
> + (1 << (0xf - (val & 0xf)));
> + else
> + pwrkey->sw_debounce_time_us = 2 * USEC_PER_SEC /
> + (1 << (0x7 - (val & 0x7)));
Can we have one more local variable like 'mask' or 'offset'. Then the
code would be easier to read
if (pwrkey->subtype >= PON_SUBTYPE_GEN2_PRIMARY)
mask = 0xf;
else
mask = 0x7
pwrkey->sw_debounce_time_us = 2 * USEC_PER_SEC / (1 << mask - (val & 0x7));
> +
> + dev_dbg(pwrkey->dev, "SW debounce time = %u us\n",
> + pwrkey->sw_debounce_time_us);
> +
> + return 0;
> +}
> +
> static int __maybe_unused pm8941_pwrkey_suspend(struct device *dev)
> {
> struct pm8941_pwrkey *pwrkey = dev_get_drvdata(dev);
> @@ -167,6 +231,8 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
> struct pm8941_pwrkey *pwrkey;
> bool pull_up;
> struct device *parent;
> + struct device_node *regmap_node;
> + const __be32 *addr;
> u32 req_delay;
> int error;
>
> @@ -188,8 +254,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
> pwrkey->data = of_device_get_match_data(&pdev->dev);
>
> parent = pdev->dev.parent;
> + regmap_node = pdev->dev.of_node;
> pwrkey->regmap = dev_get_regmap(parent, NULL);
> if (!pwrkey->regmap) {
> + regmap_node = parent->of_node;
> /*
> * We failed to get regmap for parent. Let's see if we are
> * a child of pon node and read regmap and reg from its
> @@ -200,15 +268,21 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
> dev_err(&pdev->dev, "failed to locate regmap\n");
> return -ENODEV;
> }
> + }
>
> - error = of_property_read_u32(parent->of_node,
> - "reg", &pwrkey->baseaddr);
> - } else {
> - error = of_property_read_u32(pdev->dev.of_node, "reg",
> - &pwrkey->baseaddr);
> + addr = of_get_address(regmap_node, 0, NULL, NULL);
> + if (!addr) {
> + dev_err(&pdev->dev, "reg property missing\n");
> + return -EINVAL;
> + }
> + pwrkey->baseaddr = be32_to_cpu(*addr);
Can this hunk be split off? A new API is used and it doesn't look
relevant to this patch.
> +
> + if (pwrkey->data->has_pon_pbs) {
> + /* PON_PBS base address is optional */
> + addr = of_get_address(regmap_node, 1, NULL, NULL);
> + if (addr)
> + pwrkey->pon_pbs_baseaddr = be32_to_cpu(*addr);
> }
> - if (error)
> - return error;
>
> pwrkey->irq = platform_get_irq(pdev, 0);
> if (pwrkey->irq < 0)
> @@ -217,7 +291,14 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
> error = regmap_read(pwrkey->regmap, pwrkey->baseaddr + PON_REV2,
> &pwrkey->revision);
> if (error) {
> - dev_err(&pdev->dev, "failed to set debounce: %d\n", error);
> + dev_err(&pdev->dev, "failed to read revision: %d\n", error);
Nice error message fix!
> + return error;
> + }
> +
> + error = regmap_read(pwrkey->regmap, pwrkey->baseaddr + PON_SUBTYPE,
> + &pwrkey->subtype);
> + if (error) {
> + dev_err(&pdev->dev, "failed to read subtype: %d\n", error);
> return error;
> }
>
> @@ -255,6 +336,12 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
> }
> }
>
> + if (pwrkey->data->needs_sw_debounce) {
> + error = pm8941_pwrkey_sw_debounce_init(pwrkey);
> + if (error)
> + return error;
> + }
> +
> if (pwrkey->data->pull_up_bit) {
> error = regmap_update_bits(pwrkey->regmap,
> pwrkey->baseaddr + PON_PULL_CTL,
> @@ -316,6 +403,8 @@ static const struct pm8941_data pwrkey_data = {
> .phys = "pm8941_pwrkey/input0",
> .supports_ps_hold_poff_config = true,
> .supports_debounce_config = true,
> + .needs_sw_debounce = true,
needs_sw_debounce is always true? Why is it even an option then?
> + .has_pon_pbs = false,
> };
>
> static const struct pm8941_data resin_data = {
Quoting Anjelique Melendez (2022-01-20 16:25:26)
>
> On 1/20/2022 3:01 PM, Bjorn Andersson wrote:
> > On Thu 20 Jan 12:41 PST 2022, Anjelique Melendez wrote:
> >
> >> From: David Collins <[email protected]>
> >>
> >> Add a null check for the pwrkey->data pointer after it is assigned
> >> in pm8941_pwrkey_probe(). This avoids a potential null pointer
> >> dereference when pwrkey->data->has_pon_pbs is accessed later in
> >> the probe function.
> >>
> >> Change-Id: I589c4851e544d79a1863fd110b32a0b45ac03caf
> >> Signed-off-by: David Collins <[email protected]>
> >> Signed-off-by: Anjelique Melendez <[email protected]>
> >> ---
> >> drivers/input/misc/pm8941-pwrkey.c | 4 ++++
> >> 1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
> >> index 0ce00736e695..ac08ed025802 100644
> >> --- a/drivers/input/misc/pm8941-pwrkey.c
> >> +++ b/drivers/input/misc/pm8941-pwrkey.c
> >> @@ -263,6 +263,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
> >>
> >> pwrkey->dev = &pdev->dev;
> >> pwrkey->data = of_device_get_match_data(&pdev->dev);
> >> + if (!pwrkey->data) {
> > The only way this can happen is if you add a new compatible and forget
> > to specify data and when that happens you will get a print in the log
> > somewhere, which once you realize that you don't have your pwrkey you
> > might be able to find among all the other prints.
> >
> > If you instead don't NULL check this pointer you will get a large splat
> > in the log, with callstack and all, immediately hinting you that
> > pwrkey->data is NULL.
> >
> >
> > In other words, there's already a print, a much larger print and I don't
> > think there's value in handling this mistake gracefully.
> >
> > Regards,
> > Bjorn
>
>
> We would like to the null pointer check in place to avoid static analysis
>
> warnings that can be easily fixed.
>
Many drivers check that their device_get_match_data() returns a valid
pointer. I'd like to see that API used in addition to checking the
return value for NULL so that we can keep the static analysis tools
happy. Yes it's an impossible case assuming the driver writer didn't
mess up but it shuts SA up and we don't really have a better solution
to tell tools that device_get_match_data() can't return NULL.
On 1/20/2022 8:08 PM, Stephen Boyd wrote:
> Quoting Anjelique Melendez (2022-01-20 12:41:33)
>> From: David Collins <[email protected]>
>>
>> On certain PMICs, an unexpected assertion of KPDPWR_DEB (the
>> positive logic hardware debounced power key signal) may be seen
>> during the falling edge of KPDPWR_N (i.e. a power key press) when
>> it occurs close to the rising edge of SLEEP_CLK. This then
>> triggers a spurious KPDPWR interrupt.
>>
>> Handle this issue by adding software debouncing support to ignore
>> key events that occur within the hardware debounce delay after the
>> most recent key release event.
>>
>> Change-Id: Ifa3809935c01aab9078ba2302397bc9ebf390021
> Please remove change-id when upstreaming.
Will remove change-id from other 2 patches as well.
>
>> Signed-off-by: David Collins <[email protected]>
>> Signed-off-by: Anjelique Melendez <[email protected]>
>> ---
>> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
>> index 33609603245d..b912ce00ce1c 100644
>> --- a/drivers/input/misc/pm8941-pwrkey.c
>> +++ b/drivers/input/misc/pm8941-pwrkey.c
>> @@ -126,19 +144,65 @@ static irqreturn_t pm8941_pwrkey_irq(int irq, void *_data)
>> struct pm8941_pwrkey *pwrkey = _data;
>> unsigned int sts;
>> int error;
>> + u64 elapsed_us;
>> +
>> + if (pwrkey->sw_debounce_time_us) {
>> + elapsed_us = ktime_us_delta(ktime_get(),
>> + pwrkey->last_release_time);
>> + if (elapsed_us < pwrkey->sw_debounce_time_us) {
> Perhaps storing last_release_time + sw_debounce_time_us via
> ktime_add_us() in the struct would be better. Then this line would be
>
> if (ktime_before(debounce_end, ktime_get()))
>
> and we'd avoid a division when converting to microseconds to compare
> time.
Sure this can be done!
>> + dev_dbg(pwrkey->dev, "ignoring key event received after %llu us, debounce time=%u us\n",
>> + elapsed_us, pwrkey->sw_debounce_time_us);
>> + return IRQ_HANDLED;
>> + }
>> + }
>>
>> error = regmap_read(pwrkey->regmap,
>> pwrkey->baseaddr + PON_RT_STS, &sts);
> Nitpick: I'd prefer this be on one line. And 'ret' or 'err' be used as
> it's shorter.
ACK
>
>> if (error)
>> return IRQ_HANDLED;
>>
>> - input_report_key(pwrkey->input, pwrkey->code,
>> - sts & pwrkey->data->status_bit);
>> + sts &= pwrkey->data->status_bit;
>> +
>> + if (pwrkey->sw_debounce_time_us && !sts)
>> + pwrkey->last_release_time = ktime_get();
>> +
>> + input_report_key(pwrkey->input, pwrkey->code, sts);
>> input_sync(pwrkey->input);
>>
>> return IRQ_HANDLED;
>> }
>>
>> +static int pm8941_pwrkey_sw_debounce_init(struct pm8941_pwrkey *pwrkey)
>> +{
>> + unsigned int val, addr;
>> + int error;
>> +
>> + if (pwrkey->data->has_pon_pbs && !pwrkey->pon_pbs_baseaddr) {
>> + dev_err(pwrkey->dev, "PON_PBS address missing, can't read HW debounce time\n");
>> + return 0;
>> + }
>> +
>> + if (pwrkey->pon_pbs_baseaddr)
>> + addr = pwrkey->pon_pbs_baseaddr + PON_DBC_CTL;
>> + else
>> + addr = pwrkey->baseaddr + PON_DBC_CTL;
>> + error = regmap_read(pwrkey->regmap, addr, &val);
>> + if (error)
>> + return error;
>> +
>> + if (pwrkey->subtype >= PON_SUBTYPE_GEN2_PRIMARY)
>> + pwrkey->sw_debounce_time_us = 2 * USEC_PER_SEC /
>> + (1 << (0xf - (val & 0xf)));
>> + else
>> + pwrkey->sw_debounce_time_us = 2 * USEC_PER_SEC /
>> + (1 << (0x7 - (val & 0x7)));
> Can we have one more local variable like 'mask' or 'offset'. Then the
> code would be easier to read
>
> if (pwrkey->subtype >= PON_SUBTYPE_GEN2_PRIMARY)
> mask = 0xf;
> else
> mask = 0x7
>
> pwrkey->sw_debounce_time_us = 2 * USEC_PER_SEC / (1 << mask - (val & 0x7));
Sure not a problem!
>> +
>> + dev_dbg(pwrkey->dev, "SW debounce time = %u us\n",
>> + pwrkey->sw_debounce_time_us);
>> +
>> + return 0;
>> +}
>> +
>> static int __maybe_unused pm8941_pwrkey_suspend(struct device *dev)
>> {
>> struct pm8941_pwrkey *pwrkey = dev_get_drvdata(dev);
>> @@ -167,6 +231,8 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>> struct pm8941_pwrkey *pwrkey;
>> bool pull_up;
>> struct device *parent;
>> + struct device_node *regmap_node;
>> + const __be32 *addr;
>> u32 req_delay;
>> int error;
>>
>> @@ -188,8 +254,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>> pwrkey->data = of_device_get_match_data(&pdev->dev);
>>
>> parent = pdev->dev.parent;
>> + regmap_node = pdev->dev.of_node;
>> pwrkey->regmap = dev_get_regmap(parent, NULL);
>> if (!pwrkey->regmap) {
>> + regmap_node = parent->of_node;
>> /*
>> * We failed to get regmap for parent. Let's see if we are
>> * a child of pon node and read regmap and reg from its
>> @@ -200,15 +268,21 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>> dev_err(&pdev->dev, "failed to locate regmap\n");
>> return -ENODEV;
>> }
>> + }
>>
>> - error = of_property_read_u32(parent->of_node,
>> - "reg", &pwrkey->baseaddr);
>> - } else {
>> - error = of_property_read_u32(pdev->dev.of_node, "reg",
>> - &pwrkey->baseaddr);
>> + addr = of_get_address(regmap_node, 0, NULL, NULL);
>> + if (!addr) {
>> + dev_err(&pdev->dev, "reg property missing\n");
>> + return -EINVAL;
>> + }
>> + pwrkey->baseaddr = be32_to_cpu(*addr);
> Can this hunk be split off? A new API is used and it doesn't look
> relevant to this patch.
In PMK8350 and following chips the reg property will have the pon hlos address first,
followed by a second pon pbs address. This change is needed so that both the older chipsets
and the newer can be used regardless of how many reg addresses are being used.
>
>> +
>> + if (pwrkey->data->has_pon_pbs) {
>> + /* PON_PBS base address is optional */
>> + addr = of_get_address(regmap_node, 1, NULL, NULL);
>> + if (addr)
>> + pwrkey->pon_pbs_baseaddr = be32_to_cpu(*addr);
>> }
>> - if (error)
>> - return error;
>>
>> pwrkey->irq = platform_get_irq(pdev, 0);
>> if (pwrkey->irq < 0)
>> @@ -217,7 +291,14 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>> error = regmap_read(pwrkey->regmap, pwrkey->baseaddr + PON_REV2,
>> &pwrkey->revision);
>> if (error) {
>> - dev_err(&pdev->dev, "failed to set debounce: %d\n", error);
>> + dev_err(&pdev->dev, "failed to read revision: %d\n", error);
> Nice error message fix!
>
>> + return error;
>> + }
>> +
>> + error = regmap_read(pwrkey->regmap, pwrkey->baseaddr + PON_SUBTYPE,
>> + &pwrkey->subtype);
>> + if (error) {
>> + dev_err(&pdev->dev, "failed to read subtype: %d\n", error);
>> return error;
>> }
>>
>> @@ -255,6 +336,12 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>> }
>> }
>>
>> + if (pwrkey->data->needs_sw_debounce) {
>> + error = pm8941_pwrkey_sw_debounce_init(pwrkey);
>> + if (error)
>> + return error;
>> + }
>> +
>> if (pwrkey->data->pull_up_bit) {
>> error = regmap_update_bits(pwrkey->regmap,
>> pwrkey->baseaddr + PON_PULL_CTL,
>> @@ -316,6 +403,8 @@ static const struct pm8941_data pwrkey_data = {
>> .phys = "pm8941_pwrkey/input0",
>> .supports_ps_hold_poff_config = true,
>> .supports_debounce_config = true,
>> + .needs_sw_debounce = true,
> needs_sw_debounce is always true? Why is it even an option then?
As of right now the "needs_sw_debounce" property is being used for a sw work around for a hw
problem. We anticipate that chips in the future will fix this hw problem and we would then have
devices where needs_sw_debounce would be set to false.
>
>> + .has_pon_pbs = false,
>> };
>>
>> static const struct pm8941_data resin_data = {
On 1/20/2022 7:51 PM, Stephen Boyd wrote:
> "Add support" in the subject sounds like it is new. Maybe "extend
> pm8941-pwrkey driver" would be more appropriate.
Will update subject in upcoming version.
> Quoting Anjelique Melendez (2022-01-20 12:41:30)
>> This change series includes support and fixes in pm8941-pwrkey.c.
>> Change details and description can be found in each patch. Thanks!
>>
>> David Collins (3):
>> input: misc: pm8941-pwrkey: simulate missed key press events
>> input: misc: pm8941-pwrkey: add software key press debouncing support
>> input: misc: pm8941-pwrkey: avoid potential null pointer dereference
Quoting Anjelique Melendez (2022-01-21 16:04:13)
>
>
> On 1/20/2022 8:08 PM, Stephen Boyd wrote:
> > Quoting Anjelique Melendez (2022-01-20 12:41:33)
> >> @@ -200,15 +268,21 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
> >> dev_err(&pdev->dev, "failed to locate regmap\n");
> >> return -ENODEV;
> >> }
> >> + }
> >>
> >> - error = of_property_read_u32(parent->of_node,
> >> - "reg", &pwrkey->baseaddr);
> >> - } else {
> >> - error = of_property_read_u32(pdev->dev.of_node, "reg",
> >> - &pwrkey->baseaddr);
> >> + addr = of_get_address(regmap_node, 0, NULL, NULL);
> >> + if (!addr) {
> >> + dev_err(&pdev->dev, "reg property missing\n");
> >> + return -EINVAL;
> >> + }
> >> + pwrkey->baseaddr = be32_to_cpu(*addr);
> > Can this hunk be split off? A new API is used and it doesn't look
> > relevant to this patch.
>
> In PMK8350 and following chips the reg property will have the pon hlos address first,
> followed by a second pon pbs address. This change is needed so that both the older chipsets
> and the newer can be used regardless of how many reg addresses are being used.
Got it, but do we ned to change to of_get_address() in this patch? I was
suggesting that the change to the new API be done first so that it's
clearer what's going on with the change in address location.
>
> >
> >> +
> >> + if (pwrkey->data->has_pon_pbs) {
> >> + /* PON_PBS base address is optional */
> >> + addr = of_get_address(regmap_node, 1, NULL, NULL);
> >> + if (addr)
> >> + pwrkey->pon_pbs_baseaddr = be32_to_cpu(*addr);
> >> }
> >> - if (error)
> >> - return error;
> >>
> >> pwrkey->irq = platform_get_irq(pdev, 0);
> >> if (pwrkey->irq < 0)
> >> @@ -217,7 +291,14 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
> >> error = regmap_read(pwrkey->regmap, pwrkey->baseaddr + PON_REV2,
> >> &pwrkey->revision);
> >> if (error) {
> >> - dev_err(&pdev->dev, "failed to set debounce: %d\n", error);
> >> + dev_err(&pdev->dev, "failed to read revision: %d\n", error);
> > Nice error message fix!
This can be split off to a different patch as well.
> >
> >> + return error;
> >> + }
> >> +
> >> + error = regmap_read(pwrkey->regmap, pwrkey->baseaddr + PON_SUBTYPE,
> >> + &pwrkey->subtype);
> >> + if (error) {
> >> + dev_err(&pdev->dev, "failed to read subtype: %d\n", error);
> >> return error;
> >> }
> >>
> >> @@ -255,6 +336,12 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
> >> }
> >> }
> >>
> >> + if (pwrkey->data->needs_sw_debounce) {
> >> + error = pm8941_pwrkey_sw_debounce_init(pwrkey);
> >> + if (error)
> >> + return error;
> >> + }
> >> +
> >> if (pwrkey->data->pull_up_bit) {
> >> error = regmap_update_bits(pwrkey->regmap,
> >> pwrkey->baseaddr + PON_PULL_CTL,
> >> @@ -316,6 +403,8 @@ static const struct pm8941_data pwrkey_data = {
> >> .phys = "pm8941_pwrkey/input0",
> >> .supports_ps_hold_poff_config = true,
> >> .supports_debounce_config = true,
> >> + .needs_sw_debounce = true,
> > needs_sw_debounce is always true? Why is it even an option then?
>
> As of right now the "needs_sw_debounce" property is being used for a sw work around for a hw
> problem. We anticipate that chips in the future will fix this hw problem and we would then have
> devices where needs_sw_debounce would be set to false.
Hmm ok. Why can't future chips be supported in this series? What happens
if nobody ever adds support for the new chips? We're left with this
condition that looks like dead code.
On Thu 20 Jan 20:18 PST 2022, Stephen Boyd wrote:
> Quoting Anjelique Melendez (2022-01-20 16:25:26)
> >
> > On 1/20/2022 3:01 PM, Bjorn Andersson wrote:
> > > On Thu 20 Jan 12:41 PST 2022, Anjelique Melendez wrote:
> > >
> > >> From: David Collins <[email protected]>
> > >>
> > >> Add a null check for the pwrkey->data pointer after it is assigned
> > >> in pm8941_pwrkey_probe(). This avoids a potential null pointer
> > >> dereference when pwrkey->data->has_pon_pbs is accessed later in
> > >> the probe function.
> > >>
> > >> Change-Id: I589c4851e544d79a1863fd110b32a0b45ac03caf
> > >> Signed-off-by: David Collins <[email protected]>
> > >> Signed-off-by: Anjelique Melendez <[email protected]>
> > >> ---
> > >> drivers/input/misc/pm8941-pwrkey.c | 4 ++++
> > >> 1 file changed, 4 insertions(+)
> > >>
> > >> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
> > >> index 0ce00736e695..ac08ed025802 100644
> > >> --- a/drivers/input/misc/pm8941-pwrkey.c
> > >> +++ b/drivers/input/misc/pm8941-pwrkey.c
> > >> @@ -263,6 +263,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
> > >>
> > >> pwrkey->dev = &pdev->dev;
> > >> pwrkey->data = of_device_get_match_data(&pdev->dev);
> > >> + if (!pwrkey->data) {
> > > The only way this can happen is if you add a new compatible and forget
> > > to specify data and when that happens you will get a print in the log
> > > somewhere, which once you realize that you don't have your pwrkey you
> > > might be able to find among all the other prints.
> > >
> > > If you instead don't NULL check this pointer you will get a large splat
> > > in the log, with callstack and all, immediately hinting you that
> > > pwrkey->data is NULL.
> > >
> > >
> > > In other words, there's already a print, a much larger print and I don't
> > > think there's value in handling this mistake gracefully.
> > >
> > > Regards,
> > > Bjorn
> >
> >
> > We would like to the null pointer check in place to avoid static analysis
> >
> > warnings that can be easily fixed.
> >
>
> Many drivers check that their device_get_match_data() returns a valid
> pointer. I'd like to see that API used in addition to checking the
> return value for NULL so that we can keep the static analysis tools
> happy. Yes it's an impossible case assuming the driver writer didn't
> mess up but it shuts SA up and we don't really have a better solution
> to tell tools that device_get_match_data() can't return NULL.
I'm not saying that device_get_match_data() can't return NULL, I'm
saying that in the very specific cases that it would return NULL it's
useful to have a kernel panic - as that's a much faster way to figure
out that something is wrong.
And as a timely coincidence I tried to introduce such a check last week,
for a case where the cause of the dereference issue definitely wasn't
obvious to me and Greg among others told me that it's wrong:
https://lore.kernel.org/linux-arm-msm/[email protected]/
And just to be clear, I don't care about this case in particular, but I
fear that we have a lot of SA warnings to shut up throughout the kernel.
Regards,
Bjorn
Quoting Bjorn Andersson (2022-01-24 14:26:34)
> On Thu 20 Jan 20:18 PST 2022, Stephen Boyd wrote:
>
> > Quoting Anjelique Melendez (2022-01-20 16:25:26)
> > >
> > > On 1/20/2022 3:01 PM, Bjorn Andersson wrote:
> > > > On Thu 20 Jan 12:41 PST 2022, Anjelique Melendez wrote:
> > > >
> > > >> From: David Collins <[email protected]>
> > > >>
> > > >> Add a null check for the pwrkey->data pointer after it is assigned
> > > >> in pm8941_pwrkey_probe(). This avoids a potential null pointer
> > > >> dereference when pwrkey->data->has_pon_pbs is accessed later in
> > > >> the probe function.
> > > >>
> > > >> Change-Id: I589c4851e544d79a1863fd110b32a0b45ac03caf
> > > >> Signed-off-by: David Collins <[email protected]>
> > > >> Signed-off-by: Anjelique Melendez <[email protected]>
> > > >> ---
> > > >> drivers/input/misc/pm8941-pwrkey.c | 4 ++++
> > > >> 1 file changed, 4 insertions(+)
> > > >>
> > > >> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
> > > >> index 0ce00736e695..ac08ed025802 100644
> > > >> --- a/drivers/input/misc/pm8941-pwrkey.c
> > > >> +++ b/drivers/input/misc/pm8941-pwrkey.c
> > > >> @@ -263,6 +263,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
> > > >>
> > > >> pwrkey->dev = &pdev->dev;
> > > >> pwrkey->data = of_device_get_match_data(&pdev->dev);
> > > >> + if (!pwrkey->data) {
> > > > The only way this can happen is if you add a new compatible and forget
> > > > to specify data and when that happens you will get a print in the log
> > > > somewhere, which once you realize that you don't have your pwrkey you
> > > > might be able to find among all the other prints.
> > > >
> > > > If you instead don't NULL check this pointer you will get a large splat
> > > > in the log, with callstack and all, immediately hinting you that
> > > > pwrkey->data is NULL.
> > > >
> > > >
> > > > In other words, there's already a print, a much larger print and I don't
> > > > think there's value in handling this mistake gracefully.
> > > >
> > > > Regards,
> > > > Bjorn
> > >
> > >
> > > We would like to the null pointer check in place to avoid static analysis
> > >
> > > warnings that can be easily fixed.
> > >
> >
> > Many drivers check that their device_get_match_data() returns a valid
> > pointer. I'd like to see that API used in addition to checking the
> > return value for NULL so that we can keep the static analysis tools
> > happy. Yes it's an impossible case assuming the driver writer didn't
> > mess up but it shuts SA up and we don't really have a better solution
> > to tell tools that device_get_match_data() can't return NULL.
>
> I'm not saying that device_get_match_data() can't return NULL,
Indeed, I wasn't implying that you were saying that.
> I'm
> saying that in the very specific cases that it would return NULL it's
> useful to have a kernel panic - as that's a much faster way to figure
> out that something is wrong.
I see it as more annoying, but maybe that's my workflow? When my kernel
oopses I have to go back to a recovery kernel, which takes me a few more
seconds to "repair" my device. If the driver only failed to probe then
I'd probably be able to boot far enough to get networking and more
easily replace my kernel with a working device. And I'd have userspace
access so I could poke around and figure out why the driver failed to
probe. Now obviously a big stacktrace would be helpful to know that it's
the power key driver that's busted, but it's not like we're calling some
internal API here. We're trying to probe a driver and if that oopses
because the driver writer failed at their job then it's bad on them for
writing a bad patch but also annoying for the integrator who has to deal
with the mess they created. I'd rather have a half working system here
vs. a totally broken one.
On Mon 24 Jan 17:55 PST 2022, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2022-01-24 14:26:34)
> > On Thu 20 Jan 20:18 PST 2022, Stephen Boyd wrote:
> >
> > > Quoting Anjelique Melendez (2022-01-20 16:25:26)
> > > >
> > > > On 1/20/2022 3:01 PM, Bjorn Andersson wrote:
> > > > > On Thu 20 Jan 12:41 PST 2022, Anjelique Melendez wrote:
> > > > >
> > > > >> From: David Collins <[email protected]>
> > > > >>
> > > > >> Add a null check for the pwrkey->data pointer after it is assigned
> > > > >> in pm8941_pwrkey_probe(). This avoids a potential null pointer
> > > > >> dereference when pwrkey->data->has_pon_pbs is accessed later in
> > > > >> the probe function.
> > > > >>
> > > > >> Change-Id: I589c4851e544d79a1863fd110b32a0b45ac03caf
> > > > >> Signed-off-by: David Collins <[email protected]>
> > > > >> Signed-off-by: Anjelique Melendez <[email protected]>
> > > > >> ---
> > > > >> drivers/input/misc/pm8941-pwrkey.c | 4 ++++
> > > > >> 1 file changed, 4 insertions(+)
> > > > >>
> > > > >> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
> > > > >> index 0ce00736e695..ac08ed025802 100644
> > > > >> --- a/drivers/input/misc/pm8941-pwrkey.c
> > > > >> +++ b/drivers/input/misc/pm8941-pwrkey.c
> > > > >> @@ -263,6 +263,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
> > > > >>
> > > > >> pwrkey->dev = &pdev->dev;
> > > > >> pwrkey->data = of_device_get_match_data(&pdev->dev);
> > > > >> + if (!pwrkey->data) {
> > > > > The only way this can happen is if you add a new compatible and forget
> > > > > to specify data and when that happens you will get a print in the log
> > > > > somewhere, which once you realize that you don't have your pwrkey you
> > > > > might be able to find among all the other prints.
> > > > >
> > > > > If you instead don't NULL check this pointer you will get a large splat
> > > > > in the log, with callstack and all, immediately hinting you that
> > > > > pwrkey->data is NULL.
> > > > >
> > > > >
> > > > > In other words, there's already a print, a much larger print and I don't
> > > > > think there's value in handling this mistake gracefully.
> > > > >
> > > > > Regards,
> > > > > Bjorn
> > > >
> > > >
> > > > We would like to the null pointer check in place to avoid static analysis
> > > >
> > > > warnings that can be easily fixed.
> > > >
> > >
> > > Many drivers check that their device_get_match_data() returns a valid
> > > pointer. I'd like to see that API used in addition to checking the
> > > return value for NULL so that we can keep the static analysis tools
> > > happy. Yes it's an impossible case assuming the driver writer didn't
> > > mess up but it shuts SA up and we don't really have a better solution
> > > to tell tools that device_get_match_data() can't return NULL.
> >
> > I'm not saying that device_get_match_data() can't return NULL,
>
> Indeed, I wasn't implying that you were saying that.
>
> > I'm
> > saying that in the very specific cases that it would return NULL it's
> > useful to have a kernel panic - as that's a much faster way to figure
> > out that something is wrong.
>
> I see it as more annoying, but maybe that's my workflow? When my kernel
> oopses I have to go back to a recovery kernel, which takes me a few more
> seconds to "repair" my device. If the driver only failed to probe then
> I'd probably be able to boot far enough to get networking and more
> easily replace my kernel with a working device. And I'd have userspace
> access so I could poke around and figure out why the driver failed to
> probe. Now obviously a big stacktrace would be helpful to know that it's
> the power key driver that's busted, but it's not like we're calling some
> internal API here. We're trying to probe a driver and if that oopses
> because the driver writer failed at their job then it's bad on them for
> writing a bad patch but also annoying for the integrator who has to deal
> with the mess they created. I'd rather have a half working system here
> vs. a totally broken one.
Forgot about your recovery cycle, on most of my boards I just load a new
kernel every boot, so there's no cost of recovering from a panic, it
might even save me some time if it crashes completely before userspace
starts consuming cycles.
My only concern is that this "sets" a quite fuzzy precedence. I don't
want us to just fix SA warnings all over the place, but I don't want it
to be inconvenient to work on the kernel...
Regards,
Bjorn
On 1/24/2022 11:33 AM, Stephen Boyd wrote:
> Quoting Anjelique Melendez (2022-01-21 16:04:13)
>>
>> On 1/20/2022 8:08 PM, Stephen Boyd wrote:
>>> Quoting Anjelique Melendez (2022-01-20 12:41:33)
>>>> @@ -200,15 +268,21 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>>>> dev_err(&pdev->dev, "failed to locate regmap\n");
>>>> return -ENODEV;
>>>> }
>>>> + }
>>>>
>>>> - error = of_property_read_u32(parent->of_node,
>>>> - "reg", &pwrkey->baseaddr);
>>>> - } else {
>>>> - error = of_property_read_u32(pdev->dev.of_node, "reg",
>>>> - &pwrkey->baseaddr);
>>>> + addr = of_get_address(regmap_node, 0, NULL, NULL);
>>>> + if (!addr) {
>>>> + dev_err(&pdev->dev, "reg property missing\n");
>>>> + return -EINVAL;
>>>> + }
>>>> + pwrkey->baseaddr = be32_to_cpu(*addr);
>>> Can this hunk be split off? A new API is used and it doesn't look
>>> relevant to this patch.
>> In PMK8350 and following chips the reg property will have the pon hlos address first,
>> followed by a second pon pbs address. This change is needed so that both the older chipsets
>> and the newer can be used regardless of how many reg addresses are being used.
> Got it, but do we ned to change to of_get_address() in this patch? I was
> suggesting that the change to the new API be done first so that it's
> clearer what's going on with the change in address location.
Ok, makes sense. Will separate this into it's own patch for v2.
>>>> +
>>>> + if (pwrkey->data->has_pon_pbs) {
>>>> + /* PON_PBS base address is optional */
>>>> + addr = of_get_address(regmap_node, 1, NULL, NULL);
>>>> + if (addr)
>>>> + pwrkey->pon_pbs_baseaddr = be32_to_cpu(*addr);
>>>> }
>>>> - if (error)
>>>> - return error;
>>>>
>>>> pwrkey->irq = platform_get_irq(pdev, 0);
>>>> if (pwrkey->irq < 0)
>>>> @@ -217,7 +291,14 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>>>> error = regmap_read(pwrkey->regmap, pwrkey->baseaddr + PON_REV2,
>>>> &pwrkey->revision);
>>>> if (error) {
>>>> - dev_err(&pdev->dev, "failed to set debounce: %d\n", error);
>>>> + dev_err(&pdev->dev, "failed to read revision: %d\n", error);
>>> Nice error message fix!
> This can be split off to a different patch as well.
Will do.
>>>> + return error;
>>>> + }
>>>> +
>>>> + error = regmap_read(pwrkey->regmap, pwrkey->baseaddr + PON_SUBTYPE,
>>>> + &pwrkey->subtype);
>>>> + if (error) {
>>>> + dev_err(&pdev->dev, "failed to read subtype: %d\n", error);
>>>> return error;
>>>> }
>>>>
>>>> @@ -255,6 +336,12 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>>>> }
>>>> }
>>>>
>>>> + if (pwrkey->data->needs_sw_debounce) {
>>>> + error = pm8941_pwrkey_sw_debounce_init(pwrkey);
>>>> + if (error)
>>>> + return error;
>>>> + }
>>>> +
>>>> if (pwrkey->data->pull_up_bit) {
>>>> error = regmap_update_bits(pwrkey->regmap,
>>>> pwrkey->baseaddr + PON_PULL_CTL,
>>>> @@ -316,6 +403,8 @@ static const struct pm8941_data pwrkey_data = {
>>>> .phys = "pm8941_pwrkey/input0",
>>>> .supports_ps_hold_poff_config = true,
>>>> .supports_debounce_config = true,
>>>> + .needs_sw_debounce = true,
>>> needs_sw_debounce is always true? Why is it even an option then?
>> As of right now the "needs_sw_debounce" property is being used for a sw work around for a hw
>> problem. We anticipate that chips in the future will fix this hw problem and we would then have
>> devices where needs_sw_debounce would be set to false.
> Hmm ok. Why can't future chips be supported in this series? What happens
> if nobody ever adds support for the new chips? We're left with this
> condition that looks like dead code.
Sure, makes sense. Will remove "needs_sw_debounce" property for V2.
On 1/25/2022 10:37 AM, Bjorn Andersson wrote:
> On Mon 24 Jan 17:55 PST 2022, Stephen Boyd wrote:
>
>> Quoting Bjorn Andersson (2022-01-24 14:26:34)
>>> On Thu 20 Jan 20:18 PST 2022, Stephen Boyd wrote:
>>>
>>>> Quoting Anjelique Melendez (2022-01-20 16:25:26)
>>>>> On 1/20/2022 3:01 PM, Bjorn Andersson wrote:
>>>>>> On Thu 20 Jan 12:41 PST 2022, Anjelique Melendez wrote:
>>>>>>
>>>>>>> From: David Collins <[email protected]>
>>>>>>>
>>>>>>> Add a null check for the pwrkey->data pointer after it is assigned
>>>>>>> in pm8941_pwrkey_probe(). This avoids a potential null pointer
>>>>>>> dereference when pwrkey->data->has_pon_pbs is accessed later in
>>>>>>> the probe function.
>>>>>>>
>>>>>>> Change-Id: I589c4851e544d79a1863fd110b32a0b45ac03caf
>>>>>>> Signed-off-by: David Collins <[email protected]>
>>>>>>> Signed-off-by: Anjelique Melendez <[email protected]>
>>>>>>> ---
>>>>>>> drivers/input/misc/pm8941-pwrkey.c | 4 ++++
>>>>>>> 1 file changed, 4 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
>>>>>>> index 0ce00736e695..ac08ed025802 100644
>>>>>>> --- a/drivers/input/misc/pm8941-pwrkey.c
>>>>>>> +++ b/drivers/input/misc/pm8941-pwrkey.c
>>>>>>> @@ -263,6 +263,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>>>>>>>
>>>>>>> pwrkey->dev = &pdev->dev;
>>>>>>> pwrkey->data = of_device_get_match_data(&pdev->dev);
>>>>>>> + if (!pwrkey->data) {
>>>>>> The only way this can happen is if you add a new compatible and forget
>>>>>> to specify data and when that happens you will get a print in the log
>>>>>> somewhere, which once you realize that you don't have your pwrkey you
>>>>>> might be able to find among all the other prints.
>>>>>>
>>>>>> If you instead don't NULL check this pointer you will get a large splat
>>>>>> in the log, with callstack and all, immediately hinting you that
>>>>>> pwrkey->data is NULL.
>>>>>>
>>>>>>
>>>>>> In other words, there's already a print, a much larger print and I don't
>>>>>> think there's value in handling this mistake gracefully.
>>>>>>
>>>>>> Regards,
>>>>>> Bjorn
>>>>>
>>>>> We would like to the null pointer check in place to avoid static analysis
>>>>>
>>>>> warnings that can be easily fixed.
>>>>>
>>>> Many drivers check that their device_get_match_data() returns a valid
>>>> pointer. I'd like to see that API used in addition to checking the
>>>> return value for NULL so that we can keep the static analysis tools
>>>> happy. Yes it's an impossible case assuming the driver writer didn't
>>>> mess up but it shuts SA up and we don't really have a better solution
>>>> to tell tools that device_get_match_data() can't return NULL.
>>> I'm not saying that device_get_match_data() can't return NULL,
>> Indeed, I wasn't implying that you were saying that.
>>
>>> I'm
>>> saying that in the very specific cases that it would return NULL it's
>>> useful to have a kernel panic - as that's a much faster way to figure
>>> out that something is wrong.
>> I see it as more annoying, but maybe that's my workflow? When my kernel
>> oopses I have to go back to a recovery kernel, which takes me a few more
>> seconds to "repair" my device. If the driver only failed to probe then
>> I'd probably be able to boot far enough to get networking and more
>> easily replace my kernel with a working device. And I'd have userspace
>> access so I could poke around and figure out why the driver failed to
>> probe. Now obviously a big stacktrace would be helpful to know that it's
>> the power key driver that's busted, but it's not like we're calling some
>> internal API here. We're trying to probe a driver and if that oopses
>> because the driver writer failed at their job then it's bad on them for
>> writing a bad patch but also annoying for the integrator who has to deal
>> with the mess they created. I'd rather have a half working system here
>> vs. a totally broken one.
> Forgot about your recovery cycle, on most of my boards I just load a new
> kernel every boot, so there's no cost of recovering from a panic, it
> might even save me some time if it crashes completely before userspace
> starts consuming cycles.
>
> My only concern is that this "sets" a quite fuzzy precedence. I don't
> want us to just fix SA warnings all over the place, but I don't want it
> to be inconvenient to work on the kernel...
>
> Regards,
> Bjorn
I will drop this patch for now so that further discussion can be had. Can send as a separate patch
later.