* [PATCH 0/3] Add support for pm8941-pwrkey.c
@ 2022-01-20 20:41 Anjelique Melendez
2022-01-20 20:41 ` [PATCH 1/3] input: misc: pm8941-pwrkey: add software key press debouncing support Anjelique Melendez
` (3 more replies)
0 siblings, 4 replies; 19+ messages in thread
From: Anjelique Melendez @ 2022-01-20 20:41 UTC (permalink / raw)
To: dmitry.torokhov
Cc: linux-input, linux-kernel, linux-arm-msm, collinsd,
bjorn.andersson, swboyd, skakit, Anjelique Melendez
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
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 1/3] input: misc: pm8941-pwrkey: add software key press debouncing support 2022-01-20 20:41 [PATCH 0/3] Add support for pm8941-pwrkey.c Anjelique Melendez @ 2022-01-20 20:41 ` Anjelique Melendez 2022-01-21 4:08 ` Stephen Boyd 2022-01-20 20:41 ` [PATCH 2/3] input: misc: pm8941-pwrkey: simulate missed key press events Anjelique Melendez ` (2 subsequent siblings) 3 siblings, 1 reply; 19+ messages in thread From: Anjelique Melendez @ 2022-01-20 20:41 UTC (permalink / raw) To: dmitry.torokhov Cc: linux-input, linux-kernel, linux-arm-msm, collinsd, bjorn.andersson, swboyd, skakit, Anjelique Melendez From: David Collins <collinsd@codeaurora.org> 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 <collinsd@codeaurora.org> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> --- 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] input: misc: pm8941-pwrkey: add software key press debouncing support 2022-01-20 20:41 ` [PATCH 1/3] input: misc: pm8941-pwrkey: add software key press debouncing support Anjelique Melendez @ 2022-01-21 4:08 ` Stephen Boyd 2022-01-22 0:04 ` Anjelique Melendez 0 siblings, 1 reply; 19+ messages in thread From: Stephen Boyd @ 2022-01-21 4:08 UTC (permalink / raw) To: Anjelique Melendez, dmitry.torokhov Cc: linux-input, linux-kernel, linux-arm-msm, collinsd, bjorn.andersson, skakit Quoting Anjelique Melendez (2022-01-20 12:41:33) > From: David Collins <collinsd@codeaurora.org> > > 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 <collinsd@codeaurora.org> > Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> > --- > 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 = { ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] input: misc: pm8941-pwrkey: add software key press debouncing support 2022-01-21 4:08 ` Stephen Boyd @ 2022-01-22 0:04 ` Anjelique Melendez 2022-01-24 19:33 ` Stephen Boyd 0 siblings, 1 reply; 19+ messages in thread From: Anjelique Melendez @ 2022-01-22 0:04 UTC (permalink / raw) To: Stephen Boyd, dmitry.torokhov Cc: linux-input, linux-kernel, linux-arm-msm, collinsd, bjorn.andersson, skakit On 1/20/2022 8:08 PM, Stephen Boyd wrote: > Quoting Anjelique Melendez (2022-01-20 12:41:33) >> From: David Collins <collinsd@codeaurora.org> >> >> 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 <collinsd@codeaurora.org> >> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> >> --- >> 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 = { ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] input: misc: pm8941-pwrkey: add software key press debouncing support 2022-01-22 0:04 ` Anjelique Melendez @ 2022-01-24 19:33 ` Stephen Boyd 2022-01-25 19:24 ` Anjelique Melendez 0 siblings, 1 reply; 19+ messages in thread From: Stephen Boyd @ 2022-01-24 19:33 UTC (permalink / raw) To: Anjelique Melendez, dmitry.torokhov Cc: linux-input, linux-kernel, linux-arm-msm, collinsd, bjorn.andersson, skakit 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] input: misc: pm8941-pwrkey: add software key press debouncing support 2022-01-24 19:33 ` Stephen Boyd @ 2022-01-25 19:24 ` Anjelique Melendez 0 siblings, 0 replies; 19+ messages in thread From: Anjelique Melendez @ 2022-01-25 19:24 UTC (permalink / raw) To: Stephen Boyd, dmitry.torokhov Cc: linux-input, linux-kernel, linux-arm-msm, collinsd, bjorn.andersson, skakit 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/3] input: misc: pm8941-pwrkey: simulate missed key press events 2022-01-20 20:41 [PATCH 0/3] Add support for pm8941-pwrkey.c Anjelique Melendez 2022-01-20 20:41 ` [PATCH 1/3] input: misc: pm8941-pwrkey: add software key press debouncing support Anjelique Melendez @ 2022-01-20 20:41 ` Anjelique Melendez 2022-01-20 20:41 ` [PATCH 3/3] input: misc: pm8941-pwrkey: avoid potential null pointer dereference Anjelique Melendez 2022-01-21 3:51 ` [PATCH 0/3] Add support for pm8941-pwrkey.c Stephen Boyd 3 siblings, 0 replies; 19+ messages in thread From: Anjelique Melendez @ 2022-01-20 20:41 UTC (permalink / raw) To: dmitry.torokhov Cc: linux-input, linux-kernel, linux-arm-msm, collinsd, bjorn.andersson, swboyd, skakit, Anjelique Melendez From: David Collins <collinsd@codeaurora.org> 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 <collinsd@codeaurora.org> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> --- 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/3] input: misc: pm8941-pwrkey: avoid potential null pointer dereference 2022-01-20 20:41 [PATCH 0/3] Add support for pm8941-pwrkey.c Anjelique Melendez 2022-01-20 20:41 ` [PATCH 1/3] input: misc: pm8941-pwrkey: add software key press debouncing support Anjelique Melendez 2022-01-20 20:41 ` [PATCH 2/3] input: misc: pm8941-pwrkey: simulate missed key press events Anjelique Melendez @ 2022-01-20 20:41 ` Anjelique Melendez 2022-01-20 22:18 ` Trilok Soni 2022-01-20 23:01 ` Bjorn Andersson 2022-01-21 3:51 ` [PATCH 0/3] Add support for pm8941-pwrkey.c Stephen Boyd 3 siblings, 2 replies; 19+ messages in thread From: Anjelique Melendez @ 2022-01-20 20:41 UTC (permalink / raw) To: dmitry.torokhov Cc: linux-input, linux-kernel, linux-arm-msm, collinsd, bjorn.andersson, swboyd, skakit, Anjelique Melendez From: David Collins <collinsd@codeaurora.org> 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 <collinsd@codeaurora.org> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> --- 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] input: misc: pm8941-pwrkey: avoid potential null pointer dereference 2022-01-20 20:41 ` [PATCH 3/3] input: misc: pm8941-pwrkey: avoid potential null pointer dereference Anjelique Melendez @ 2022-01-20 22:18 ` Trilok Soni 2022-01-21 0:15 ` Anjelique Melendez 2022-01-20 23:01 ` Bjorn Andersson 1 sibling, 1 reply; 19+ messages in thread From: Trilok Soni @ 2022-01-20 22:18 UTC (permalink / raw) To: Anjelique Melendez, dmitry.torokhov Cc: linux-input, linux-kernel, linux-arm-msm, collinsd, bjorn.andersson, swboyd, skakit On 1/20/2022 12:41 PM, Anjelique Melendez wrote: > From: David Collins <collinsd@codeaurora.org> > > 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 <collinsd@codeaurora.org> > Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> > --- > 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] input: misc: pm8941-pwrkey: avoid potential null pointer dereference 2022-01-20 22:18 ` Trilok Soni @ 2022-01-21 0:15 ` Anjelique Melendez 0 siblings, 0 replies; 19+ messages in thread From: Anjelique Melendez @ 2022-01-21 0:15 UTC (permalink / raw) To: Trilok Soni, dmitry.torokhov Cc: linux-input, linux-kernel, linux-arm-msm, collinsd, bjorn.andersson, swboyd, skakit On 1/20/2022 2:18 PM, Trilok Soni wrote: > On 1/20/2022 12:41 PM, Anjelique Melendez wrote: >> From: David Collins <collinsd@codeaurora.org> >> >> 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 <collinsd@codeaurora.org> >> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> >> --- >> 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! ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] input: misc: pm8941-pwrkey: avoid potential null pointer dereference 2022-01-20 20:41 ` [PATCH 3/3] input: misc: pm8941-pwrkey: avoid potential null pointer dereference Anjelique Melendez 2022-01-20 22:18 ` Trilok Soni @ 2022-01-20 23:01 ` Bjorn Andersson 2022-01-21 0:25 ` Anjelique Melendez 1 sibling, 1 reply; 19+ messages in thread From: Bjorn Andersson @ 2022-01-20 23:01 UTC (permalink / raw) To: Anjelique Melendez Cc: dmitry.torokhov, linux-input, linux-kernel, linux-arm-msm, collinsd, swboyd, skakit On Thu 20 Jan 12:41 PST 2022, Anjelique Melendez wrote: > From: David Collins <collinsd@codeaurora.org> > > 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 <collinsd@codeaurora.org> > Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> > --- > 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] input: misc: pm8941-pwrkey: avoid potential null pointer dereference 2022-01-20 23:01 ` Bjorn Andersson @ 2022-01-21 0:25 ` Anjelique Melendez 2022-01-21 4:18 ` Stephen Boyd 0 siblings, 1 reply; 19+ messages in thread From: Anjelique Melendez @ 2022-01-21 0:25 UTC (permalink / raw) To: Bjorn Andersson Cc: dmitry.torokhov, linux-input, linux-kernel, linux-arm-msm, collinsd, swboyd, skakit On 1/20/2022 3:01 PM, Bjorn Andersson wrote: > On Thu 20 Jan 12:41 PST 2022, Anjelique Melendez wrote: > >> From: David Collins <collinsd@codeaurora.org> >> >> 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 <collinsd@codeaurora.org> >> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> >> --- >> 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 >> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] input: misc: pm8941-pwrkey: avoid potential null pointer dereference 2022-01-21 0:25 ` Anjelique Melendez @ 2022-01-21 4:18 ` Stephen Boyd 2022-01-24 22:26 ` Bjorn Andersson 0 siblings, 1 reply; 19+ messages in thread From: Stephen Boyd @ 2022-01-21 4:18 UTC (permalink / raw) To: Anjelique Melendez, Bjorn Andersson Cc: dmitry.torokhov, linux-input, linux-kernel, linux-arm-msm, collinsd, skakit 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 <collinsd@codeaurora.org> > >> > >> 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 <collinsd@codeaurora.org> > >> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> > >> --- > >> 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] input: misc: pm8941-pwrkey: avoid potential null pointer dereference 2022-01-21 4:18 ` Stephen Boyd @ 2022-01-24 22:26 ` Bjorn Andersson 2022-01-25 1:55 ` Stephen Boyd 0 siblings, 1 reply; 19+ messages in thread From: Bjorn Andersson @ 2022-01-24 22:26 UTC (permalink / raw) To: Stephen Boyd Cc: Anjelique Melendez, dmitry.torokhov, linux-input, linux-kernel, linux-arm-msm, collinsd, skakit 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 <collinsd@codeaurora.org> > > >> > > >> 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 <collinsd@codeaurora.org> > > >> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> > > >> --- > > >> 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/20220118185612.2067031-2-bjorn.andersson@linaro.org/ 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] input: misc: pm8941-pwrkey: avoid potential null pointer dereference 2022-01-24 22:26 ` Bjorn Andersson @ 2022-01-25 1:55 ` Stephen Boyd 2022-01-25 18:37 ` Bjorn Andersson 0 siblings, 1 reply; 19+ messages in thread From: Stephen Boyd @ 2022-01-25 1:55 UTC (permalink / raw) To: Bjorn Andersson Cc: Anjelique Melendez, dmitry.torokhov, linux-input, linux-kernel, linux-arm-msm, collinsd, skakit 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 <collinsd@codeaurora.org> > > > >> > > > >> 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 <collinsd@codeaurora.org> > > > >> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> > > > >> --- > > > >> 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] input: misc: pm8941-pwrkey: avoid potential null pointer dereference 2022-01-25 1:55 ` Stephen Boyd @ 2022-01-25 18:37 ` Bjorn Andersson 2022-01-27 19:51 ` Anjelique Melendez 0 siblings, 1 reply; 19+ messages in thread From: Bjorn Andersson @ 2022-01-25 18:37 UTC (permalink / raw) To: Stephen Boyd Cc: Anjelique Melendez, dmitry.torokhov, linux-input, linux-kernel, linux-arm-msm, collinsd, skakit 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 <collinsd@codeaurora.org> > > > > >> > > > > >> 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 <collinsd@codeaurora.org> > > > > >> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> > > > > >> --- > > > > >> 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] input: misc: pm8941-pwrkey: avoid potential null pointer dereference 2022-01-25 18:37 ` Bjorn Andersson @ 2022-01-27 19:51 ` Anjelique Melendez 0 siblings, 0 replies; 19+ messages in thread From: Anjelique Melendez @ 2022-01-27 19:51 UTC (permalink / raw) To: Bjorn Andersson, Stephen Boyd Cc: dmitry.torokhov, linux-input, linux-kernel, linux-arm-msm, collinsd, skakit 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 <collinsd@codeaurora.org> >>>>>>> >>>>>>> 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 <collinsd@codeaurora.org> >>>>>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> >>>>>>> --- >>>>>>> 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/3] Add support for pm8941-pwrkey.c 2022-01-20 20:41 [PATCH 0/3] Add support for pm8941-pwrkey.c Anjelique Melendez ` (2 preceding siblings ...) 2022-01-20 20:41 ` [PATCH 3/3] input: misc: pm8941-pwrkey: avoid potential null pointer dereference Anjelique Melendez @ 2022-01-21 3:51 ` Stephen Boyd 2022-01-22 0:04 ` Anjelique Melendez 3 siblings, 1 reply; 19+ messages in thread From: Stephen Boyd @ 2022-01-21 3:51 UTC (permalink / raw) To: Anjelique Melendez, dmitry.torokhov Cc: linux-input, linux-kernel, linux-arm-msm, collinsd, bjorn.andersson, skakit "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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/3] Add support for pm8941-pwrkey.c 2022-01-21 3:51 ` [PATCH 0/3] Add support for pm8941-pwrkey.c Stephen Boyd @ 2022-01-22 0:04 ` Anjelique Melendez 0 siblings, 0 replies; 19+ messages in thread From: Anjelique Melendez @ 2022-01-22 0:04 UTC (permalink / raw) To: Stephen Boyd, dmitry.torokhov Cc: linux-input, linux-kernel, linux-arm-msm, collinsd, bjorn.andersson, skakit 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2022-01-27 19:51 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-20 20:41 [PATCH 0/3] Add support for pm8941-pwrkey.c Anjelique Melendez 2022-01-20 20:41 ` [PATCH 1/3] input: misc: pm8941-pwrkey: add software key press debouncing support Anjelique Melendez 2022-01-21 4:08 ` Stephen Boyd 2022-01-22 0:04 ` Anjelique Melendez 2022-01-24 19:33 ` Stephen Boyd 2022-01-25 19:24 ` Anjelique Melendez 2022-01-20 20:41 ` [PATCH 2/3] input: misc: pm8941-pwrkey: simulate missed key press events Anjelique Melendez 2022-01-20 20:41 ` [PATCH 3/3] input: misc: pm8941-pwrkey: avoid potential null pointer dereference Anjelique Melendez 2022-01-20 22:18 ` Trilok Soni 2022-01-21 0:15 ` Anjelique Melendez 2022-01-20 23:01 ` Bjorn Andersson 2022-01-21 0:25 ` Anjelique Melendez 2022-01-21 4:18 ` Stephen Boyd 2022-01-24 22:26 ` Bjorn Andersson 2022-01-25 1:55 ` Stephen Boyd 2022-01-25 18:37 ` Bjorn Andersson 2022-01-27 19:51 ` Anjelique Melendez 2022-01-21 3:51 ` [PATCH 0/3] Add support for pm8941-pwrkey.c Stephen Boyd 2022-01-22 0:04 ` Anjelique Melendez
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).