| 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: Fri, 21 Sep 2018 00:11:28 +0100
From: Marc Zyngier <marc.zyngier@....com>
To: Lina Iyer <ilina@...eaurora.org>
Cc: bjorn.andersson@...aro.org, sboyd@...nel.org, evgreen@...omium.org,
linus.walleij@...aro.org, rplsssn@...eaurora.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
rnayak@...eaurora.org, devicetree@...r.kernel.org,
andy.gross@...aro.org, dianders@...omium.org
Subject: Re: [PATCH v3 1/5] drivers: pinctrl: qcom: add wakeup capability to GPIO
Hi Lina,
On Tue, 04 Sep 2018 22:18:06 +0100,
Lina Iyer <ilina@...eaurora.org> wrote:
>
> QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on
> domain can wakeup the SoC, when interrupts and GPIOs are routed to its
> interrupt controller. Only select GPIOs that are deemed wakeup capable
> are routed to specific PDC pins. During low power state, the pinmux
> interrupt controller may be non-functional but the PDC would be. The PDC
> can detect the wakeup GPIO is triggered and bring the TLMM to an
> operational state.
>
> Interrupts that are level triggered will be detected at the TLMM when
> the controller becomes operational. Edge interrupts however need to be
> replayed again.
>
> Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ,
> but keep it disabled. During suspend, we can enable the PDC IRQ instead
> of the GPIO IRQ, which may or not be detected.
>
> Signed-off-by: Lina Iyer <ilina@...eaurora.org>
> ---
> Changes in v3:
> - free action->name
> Changes in v2:
> - Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ
> Changes in v1:
> - Trigger GPIO in h/w from PDC IRQ handler
> - Avoid big tables for GPIO-PDC map, pick from DT instead
> - Use handler_data
> ---
> drivers/pinctrl/qcom/pinctrl-msm.c | 98 ++++++++++++++++++++++++++++++
> 1 file changed, 98 insertions(+)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 0e22f52b2a19..6527a0a9edd1 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -687,11 +687,15 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> const struct msm_pingroup *g;
> unsigned long flags;
> u32 val;
> + struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>
> g = &pctrl->soc->groups[d->hwirq];
>
> raw_spin_lock_irqsave(&pctrl->lock, flags);
>
> + if (pdc_irqd)
> + irq_set_irq_type(pdc_irqd->irq, type);
> +
> /*
> * For hw without possibility of detecting both edges
> */
> @@ -779,9 +783,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> unsigned long flags;
> + struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>
> raw_spin_lock_irqsave(&pctrl->lock, flags);
>
> + if (pdc_irqd)
> + irq_set_irq_wake(pdc_irqd->irq, on);
> +
> irq_set_irq_wake(pctrl->irq, on);
>
> raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> @@ -863,6 +871,94 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
> return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
> }
>
> +static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
> +{
> + struct irq_data *irqd = data;
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> + struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> + const struct msm_pingroup *g;
> + unsigned long flags;
> + u32 val;
> +
> + if (!irqd_is_level_type(irqd)) {
> + g = &pctrl->soc->groups[irqd->hwirq];
> + raw_spin_lock_irqsave(&pctrl->lock, flags);
> + val = BIT(g->intr_status_bit);
> + writel(val, pctrl->regs + g->intr_status_reg);
write_relaxed, please.
> + raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> + }
Overall, this requires some form of documentation (I'll have forgotten
about the whole thing quickly enough).
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int msm_gpio_pdc_pin_request(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> + struct platform_device *pdev = to_platform_device(pctrl->dev);
> + const char *pin_name;
> + int irq;
> + int ret;
> +
> + pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq);
> + if (!pin_name)
> + return -ENOMEM;
> +
> + irq = platform_get_irq_byname(pdev, pin_name);
> + if (irq < 0) {
> + kfree(pin_name);
> + return 0;
> + }
> +
> + ret = request_irq(irq, wake_irq_gpio_handler, irqd_get_trigger_type(d),
> + pin_name, d);
> + if (ret) {
> + pr_warn("GPIO-%lu could not be set up as wakeup", d->hwirq);
This message doesn't correspond to what you're doing here.
> + kfree(pin_name);
> + return ret;
> + }
> +
> + irq_set_handler_data(d->irq, irq_get_irq_data(irq));
> + disable_irq(irq);
Who enables this interrupt?
There is a gap between request_irq and disable_irq, where you can take
the interrupt, and not having set the handler data. Horrible things
will happen in this situation.
A slightly better way of doing that would be:
// Prevent the interrupt from being enabled on request
irq_set_status_flags(d->irq, IRQ_NOAUTOEN);
ret = request_irq(...);
irq_set_handler(...);
and let the enable_irq() do its thing when it happens (where?).
> +
> + return 0;
> +}
> +
> +static int msm_gpio_pdc_pin_release(struct irq_data *d)
> +{
> + struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
> + const void *name;
> +
> + if (pdc_irqd) {
> + irq_set_handler_data(d->irq, NULL);
> + name = free_irq(pdc_irqd->irq, d);
> + kfree(name);
> + }
> +
> + return 0;
> +}
> +
> +static int msm_gpio_irq_reqres(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +
> + if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
> + dev_err(gc->parent, "unable to lock HW IRQ %lu for IRQ\n",
> + irqd_to_hwirq(d));
> + return -EINVAL;
> + }
> +
> + return msm_gpio_pdc_pin_request(d);
> +}
> +
> +static void msm_gpio_irq_relres(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +
> + msm_gpio_pdc_pin_release(d);
> + gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
> +}
> +
> static int msm_gpio_init(struct msm_pinctrl *pctrl)
> {
> struct gpio_chip *chip;
> @@ -887,6 +983,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
> pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
> pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
> pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
> + pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
> + pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
>
> ret = gpiochip_add_data(&pctrl->chip, pctrl);
> if (ret) {
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
Thanks,
M.
--
Jazz is not dead, it just smell funny.
Powered by blists - more mailing lists