| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 20 Jan 2022 20:08:13 -0800
From: Stephen Boyd <swboyd@...omium.org>
To: Anjelique Melendez <quic_amelende@...cinc.com>,
dmitry.torokhov@...il.com
Cc: linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, collinsd@...eaurora.org,
bjorn.andersson@...aro.org, skakit@...eaurora.org
Subject: Re: [PATCH 1/3] input: misc: pm8941-pwrkey: add software key press
debouncing support
Quoting Anjelique Melendez (2022-01-20 12:41:33)
> From: David Collins <collinsd@...eaurora.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@...eaurora.org>
> Signed-off-by: Anjelique Melendez <quic_amelende@...cinc.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 = {
Powered by blists - more mailing lists