Re: [RFC v3] irqchip: qcom: pdc: Introduce irq_set_wake call

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 4/24/2020 8:07 AM, Stephen Boyd wrote:
Quoting Maulik Shah (2020-04-22 05:14:21)
We discussed this in RFC v2, pasting your reply from same.

That looks like a bug. It appears that gpiolib is only hooking the irq
disable path here so that it can keep track of what irqs are not in use
by gpiolib and allow them to be used for GPIO purposes when the irqs are
disabled. See commit 461c1a7d4733 ("gpiolib: override
irq_enable/disable"). That code in gpiolib should probably see if lazy
mode has been disabled on the irq and do similar things to what genirq
does and then let genirq mask the gpios if they trigger during the time
when the irq is disabled. Regardless of this design though, I don't
understand why this matters.
Yeah. I'm saying that the gpiolib code that forces all gpio irqs to be
non-lazy is broken. Please send a patch to fix gpiolib so that irqs can
be lazy again.

Let me reiterate.

In below genirq code, if irq_chip did not choose to implement .irq_disable (and the mask argument is coming as false by default during first disable) from the caller of this,

Then yes control takes lazy path since it doesn't enter to  if (desc->irq_data.chip->irq_disable)  or else if (mask) case and the __irq_disable call simply returns  without executing anything.

This leaves IRQ enabled in HW during suspend even though driver done disable_irq()

static void __irq_disable(struct irq_desc *desc, bool mask)
{
        if (irqd_irq_disabled(&desc->irq_data)) {
                if (mask)
                        mask_irq(desc);
        } else {
                irq_state_set_disabled(desc);
                if (desc->irq_data.chip->irq_disable) {
desc->irq_data.chip->irq_disable(&desc->irq_data);
                        irq_state_set_masked(desc);
                } else if (mask) {
                        mask_irq(desc);
                }
        }
}

With above understanding, we have two more points to consider

1. The gpiolib registers for .irq_disable for every gpio chip, making it unlazy for every gpio chip.

2. The irq_chip that registers with gpiolib (from pinctrl-msm.c, also has .irq_disable implemented, and its parent PDC also has .irq_disable implemented)

Even if we fix point (1), by making gpiolib replicating genirq code,
It still will call irq_disable (from replicated code in gpiolib now) since irq_chip that registers with gpiolib has chosen to implement .irq_disable.

For other irq_chip's which don't implement .irq_disable, it will help.
But since from point (2) we know that PDC irq_chip implements .irq_disable, and would disable in HW the moment .irq_disable call comes in.


I thought it didn't matter before but now that I've
learned more it sounds like we have to use lazy irqs here so that irqs
stay enabled on the path to suspend while they're disabled but marked
for wakeup.

The description for irq_disable() says...

 * If the chip does not implement the irq_disable callback, we
 * use a lazy disable approach. That means we mark the interrupt
 * disabled, but leave the hardware unmasked. That's an
 * optimization...

Since PDC irq_chip implements .irq_disable callback, IRQ gets disabled in HW immediatly.

The comment says that lazy approch will be used if .irq_disable callback is not implemented.

IMO, client driver's should not assume/expect that underlying irq_chip will take lazy path only,
As it depends on whether irq_chip implemented .irq_disable callback or not.

If a driver is relying on lazy disable approach till date, then the driver should get fixed.
(by removing the unnecessary disable_irq() during suspend)

i suspect this might be the case in EC driver, on the platforms where it worked fine till today, irq may not have been disabled in HW (may be that working platform's irq_chip may not have implemented irq_disable hence support lazy disable which leaves IRQ unmasked in HW even after disable_irq() and able to resume from suspend)


Otherwise I don't know how to make this work.
There are 3 possible options to make it work

1. Fix the EC driver (to stop calling disable_irq() during suspend) / support unlazy disable in EC driver

2. Support lazy disable (requires fixes in gpiolib and irq_chip also should not implement .irq_disable callback)

With above explanation, The PDC irq_chip want to keep continue implement .irq_disable callback (we have different functionality in .irq_mask and .irq_disable implementation)

so option 2 is ruled out IMO.

3. We use this patch

The current patch makes this work by re-enabling such IRQs in HW during suspend's very end point, and restores state during resume.

i can not think of any other option.

Let me know if you now agree to go with option (1)

Otherwise, for option (3),

i can post next revision after addressing some minor fixes/suggestions.
I have tried to make this further simpler by not operating on both PDC-GPIO and PDC irq domains, but it seems we need to use both irq domains and take the action accordingly during pdc_suspend() and pdc_resume().

I have missed the default case in switch (noticed same during rpmh patch doug has posted :-)), as it can get called for CLUSTER notification.
Will fix in next revision.

Thanks,
Maulik

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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux