Message-ID: <bc0337f1-4b8e-7a3a-22fd-ff6f8cbaffca@codeaurora.org>
Date:   Tue, 24 Nov 2020 18:13:21 +0530
From:   Maulik Shah <mkshah@...eaurora.org>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Douglas Anderson <dianders@...omium.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Linus Walleij <linus.walleij@...aro.org>,
        Srinivas Ramana <sramana@...eaurora.org>,
        Neeraj Upadhyay <neeraju@...eaurora.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        linux-gpio@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Andy Gross <agross@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] pinctrl: qcom: Clear possible pending irq when
 remuxing GPIOs

Hi Marc,

On 11/24/2020 4:45 PM, Marc Zyngier wrote:
> On 2020-11-24 10:37, Maulik Shah wrote:
>
> [...]
>
>>>   static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>>>                     unsigned function,
>>>                     unsigned group)
>>>   {
>>>       struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
>>> +    struct gpio_chip *gc = &pctrl->chip;
>>> +    unsigned int irq = irq_find_mapping(gc->irq.domain, group);
>>>       const struct msm_pingroup *g;
>>>       unsigned long flags;
>>>       u32 val, mask;
>>> +    u32 oldval;
>>> +    u32 old_i;
>>>       int i;
>>>         g = &pctrl->soc->groups[group];
>>> @@ -187,15 +215,26 @@ static int msm_pinmux_set_mux(struct 
>>> pinctrl_dev *pctldev,
>>>       if (WARN_ON(i == g->nfuncs))
>>>           return -EINVAL;
>>>   -    raw_spin_lock_irqsave(&pctrl->lock, flags);
>>> +    disable_irq(irq);
>>>   -    val = msm_readl_ctl(pctrl, g);
>>> +    raw_spin_lock_irqsave(&pctrl->lock, flags);
>>> +    oldval = val = msm_readl_ctl(pctrl, g);
>>>       val &= ~mask;
>>>       val |= i << g->mux_bit;
>>>       msm_writel_ctl(val, pctrl, g);
>>> -
>>>       raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>>>   +    /*
>>> +     * Clear IRQs if switching to/from GPIO mode since muxing to/from
>>> +     * the GPIO path can cause phantom edges.
>>> +     */
>>> +    old_i = (oldval & mask) >> g->mux_bit;
>>> +    if (old_i != i &&
>>> +        (i == pctrl->soc->gpio_func || old_i == 
>>> pctrl->soc->gpio_func))
>>> +        msm_pinctrl_clear_pending_irq(pctrl, group, irq);
>>
>> disable_irq() and enable_irq() should be moved inside this if loop. as
>> only use for this is to mask the IRQ when switching back to gpio IRQ
>> mode?
>>
>> i also don't think we should leave IRQ enabled at the end of this
>> function by default, probably need to check if IRQ was already
>> unmasked before disabling it, then only call enable_irq().
>
> Why? It looks to me that this reproduces the behaviour of 
> IRQCHIP_SET_TYPE_MASKED, which is highly desirable. What
> problem are you trying to address with this?

Correct, here trying to reproduce the behaviour of 
IRQCHIP_SET_TYPE_MASKED which i guess is ok once its moved inside if 
loop as this is the place its switching to IRQ mode.

but there is a problem to leave it enabled at the end of set_direction 
callbacks, see below.

>
>>
>>> +
>>> +    enable_irq(irq);
>>> +
>>>       return 0;
>>>   }
>>>   @@ -456,32 +495,45 @@ static const struct pinconf_ops 
>>> msm_pinconf_ops = {
>>>   static int msm_gpio_direction_input(struct gpio_chip *chip, 
>>> unsigned offset)
>>>   {
>>>       const struct msm_pingroup *g;
>>> +    unsigned int irq = irq_find_mapping(chip->irq.domain, offset);
>>>       struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
>>>       unsigned long flags;
>>> +    u32 oldval;
>>>       u32 val;
>>>         g = &pctrl->soc->groups[offset];
>>>   +    disable_irq(irq);
>>> +
>>>       raw_spin_lock_irqsave(&pctrl->lock, flags);
>>>   -    val = msm_readl_ctl(pctrl, g);
>>> +    oldval = val = msm_readl_ctl(pctrl, g);
>>>       val &= ~BIT(g->oe_bit);
>>>       msm_writel_ctl(val, pctrl, g);
>>>         raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>>>   +    if (oldval != val)
>>> +        msm_pinctrl_clear_pending_irq(pctrl, offset, irq);
>>> +
>>> +    enable_irq(irq);
>>
>> i do not think we need disable_irq() and enable_irq() here, changing
>> direction to input does not mean its being used for interrupt only, it
>> may be set to use something like Rx mode in UART.
>>
>> the client driver should enable IRQ when needed.
>
> And the kernel doesn't expect random interrupts to fire. Again, what
> are you trying to fix by removing these?

I see leaving IRQ enabled here can cause problems. For example in 
qcom_geni_serial.c driver before requesting IRQ, it sets the 
IRQ_NOAUTOEN flag to not keep it enabled.

see the below snippet
         irq_set_status_flags(uport->irq, IRQ_NOAUTOEN);
         ret = devm_request_irq(uport->dev, uport->irq, 
qcom_geni_serial_isr,
                         IRQF_TRIGGER_HIGH, port->name, uport);

later when this devm_request_irq() invokes .irq_request_resources 
callback it will reach msm_gpio_irq_reqres() from
where msm_gpio_direction_input() is called which leaves the irq enabled 
at the end with enable_irq() which was not expected by driver.

It will cause is IRQ storm since the UART geni driver uses GPIO in Rx 
mode when out of suspend. The IRQ mode in GPIO is enabled
with suspend entry only. During resume the IRQ will again be disabled 
and GPIO will be switched to Rx mode.

Thanks,
Maulik
>
>         M.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Powered by blists - more mailing lists