[PATCH] irqchip/loongson-liointc: Support to set IRQ_TYPE_EDGE_BOTH

[PATCH] irqchip/loongson-liointc: Support to set IRQ_TYPE_EDGE_BOTH

[Huacai Chen]

Some peripheral subsystems request IRQ_TYPE_EDGE_BOTH interrupt type and
report request failures on LIOINTC. To avoid such failures we support to
set IRQ_TYPE_EDGE_BOTH type on LIOINTC, by setting LIOINTC_REG_INTC_EDGE
to true and keep LIOINTC_REG_INTC_POL as is.

Cc: stable@vger.kernel.org
Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
---
 drivers/irqchip/irq-loongson-liointc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/irqchip/irq-loongson-liointc.c b/drivers/irqchip/irq-loongson-liointc.c
index 2b1bd4a96665..c0c8ef8d27cf 100644
--- a/drivers/irqchip/irq-loongson-liointc.c
+++ b/drivers/irqchip/irq-loongson-liointc.c
@@ -128,6 +128,10 @@ static int liointc_set_type(struct irq_data *data, unsigned int type)
 		liointc_set_bit(gc, LIOINTC_REG_INTC_EDGE, mask, false);
 		liointc_set_bit(gc, LIOINTC_REG_INTC_POL, mask, true);
 		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		liointc_set_bit(gc, LIOINTC_REG_INTC_EDGE, mask, true);
+		/* Requester need "both", keep LIOINTC_REG_INTC_POL as is */
+		break;
 	case IRQ_TYPE_EDGE_RISING:
 		liointc_set_bit(gc, LIOINTC_REG_INTC_EDGE, mask, true);
 		liointc_set_bit(gc, LIOINTC_REG_INTC_POL, mask, false);
-- 
2.47.1

Re: [PATCH] irqchip/loongson-liointc: Support to set IRQ_TYPE_EDGE_BOTH

[Thomas Gleixner]

On Wed, Apr 02 2025 at 17:25, Huacai Chen wrote:
> Some peripheral subsystems request IRQ_TYPE_EDGE_BOTH interrupt type and
> report request failures on LIOINTC. To avoid such failures we support to
> set IRQ_TYPE_EDGE_BOTH type on LIOINTC, by setting LIOINTC_REG_INTC_EDGE
> to true and keep LIOINTC_REG_INTC_POL as is.

That's broken because it will either trigger on the rising or the
falling edge depending on the setting of LIOINTC_REG_INTC_POL.

But it won't trigger on both. So no, you cannot claim that this fixes
anything.

Thanks,

        tglx

Re: [PATCH] irqchip/loongson-liointc: Support to set IRQ_TYPE_EDGE_BOTH

[Huacai Chen]

Hi, Thomas,

On Thu, Apr 3, 2025 at 11:48 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Wed, Apr 02 2025 at 17:25, Huacai Chen wrote:
> > Some peripheral subsystems request IRQ_TYPE_EDGE_BOTH interrupt type and
> > report request failures on LIOINTC. To avoid such failures we support to
> > set IRQ_TYPE_EDGE_BOTH type on LIOINTC, by setting LIOINTC_REG_INTC_EDGE
> > to true and keep LIOINTC_REG_INTC_POL as is.
>
> That's broken because it will either trigger on the rising or the
> falling edge depending on the setting of LIOINTC_REG_INTC_POL.
Yes, this patch does exactly this.

>
> But it won't trigger on both. So no, you cannot claim that this fixes
> anything.
Yes, it won't trigger on both (not perfect), but it allows drivers
that request "both" work (better than fail to request), and there are
other irqchip drivers that do similar things.

For example,

drivers/irqchip/irq-mips-gic.c
        case IRQ_TYPE_EDGE_BOTH:
                pol = 0; /* Doesn't matter */
                trig = GIC_TRIG_EDGE;
                dual = GIC_DUAL_DUAL;
                break;

drivers/irqchip/irq-qcom-mpm.c
        if (type & IRQ_TYPE_EDGE_BOTH)
                type = IRQ_TYPE_EDGE_RISING;

 Huacai

>
> Thanks,
>
>         tglx

Re: [PATCH] irqchip/loongson-liointc: Support to set IRQ_TYPE_EDGE_BOTH

[Thomas Gleixner]

On Sun, Apr 06 2025 at 17:46, Huacai Chen wrote:
> On Thu, Apr 3, 2025 at 11:48 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> But it won't trigger on both. So no, you cannot claim that this fixes
>> anything.
> Yes, it won't trigger on both (not perfect), but it allows drivers
> that request "both" work (better than fail to request), and there are

By some definition of 'work'. There is probably a good technical reason
why those drivers expect EDGE_BOTH to work correctly and otherwise fail
to load.

You completely fail to explain, why this hack actually 'works' and what
the implications are for such drivers.

> other irqchip drivers that do similar things.

Justifying bogosity with already existing bogosity is not a technical
argument.

Thanks,

        tglx

Re: [PATCH] irqchip/loongson-liointc: Support to set IRQ_TYPE_EDGE_BOTH

[Huacai Chen]

On Sun, Apr 6, 2025 at 6:18 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Sun, Apr 06 2025 at 17:46, Huacai Chen wrote:
> > On Thu, Apr 3, 2025 at 11:48 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> But it won't trigger on both. So no, you cannot claim that this fixes
> >> anything.
> > Yes, it won't trigger on both (not perfect), but it allows drivers
> > that request "both" work (better than fail to request), and there are
>
> By some definition of 'work'. There is probably a good technical reason
> why those drivers expect EDGE_BOTH to work correctly and otherwise fail
> to load.
The real problem we encounter is the MMC driver. In
drivers/mmc/core/slot-gpio.c there is
devm_request_threaded_irq(host->parent, irq,
                        NULL, ctx->cd_gpio_isr,
                        IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
IRQF_ONESHOT,
                        ctx->cd_label, host);

"IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING" is an alias of
"IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING", and
"IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING" is
"IRQ_TYPE_EDGE_BOTH".

Except MMC, "grep IRQ_TYPE_EDGE_BOTH drivers" can give some more examples.

Huacai

>
> You completely fail to explain, why this hack actually 'works' and what
> the implications are for such drivers.
>
> > other irqchip drivers that do similar things.
>
> Justifying bogosity with already existing bogosity is not a technical
> argument.
>
> Thanks,
>
>         tglx

Re: [PATCH] irqchip/loongson-liointc: Support to set IRQ_TYPE_EDGE_BOTH

[Thomas Gleixner]

On Sun, Apr 06 2025 at 20:46, Huacai Chen wrote:
> On Sun, Apr 6, 2025 at 6:18 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> On Sun, Apr 06 2025 at 17:46, Huacai Chen wrote:
>> > On Thu, Apr 3, 2025 at 11:48 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> >> But it won't trigger on both. So no, you cannot claim that this fixes
>> >> anything.
>> > Yes, it won't trigger on both (not perfect), but it allows drivers
>> > that request "both" work (better than fail to request), and there are
>>
>> By some definition of 'work'. There is probably a good technical reason
>> why those drivers expect EDGE_BOTH to work correctly and otherwise fail
>> to load.
> The real problem we encounter is the MMC driver. In
> drivers/mmc/core/slot-gpio.c there is
> devm_request_threaded_irq(host->parent, irq,
>                         NULL, ctx->cd_gpio_isr,
>                         IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
> IRQF_ONESHOT,
>                         ctx->cd_label, host);
>
> "IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING" is an alias of
> "IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING", and
> "IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING" is
> "IRQ_TYPE_EDGE_BOTH".

I know that.

> Except MMC, "grep IRQ_TYPE_EDGE_BOTH drivers" can give some more examples.

Sure, but you still do not explain why this works even when the driver
is obviously depending on EDGE_BOTH. If it does not then the driver is
bogus.

Looking at it, there is obviously a reason for this particular driver to
request BOTH. Why?

This is the card change detection and it uses a GPIO. Insert raises one
edge and remove the opposite one.

Which means whatever edge you chose randomly the detection will only
work in one direction. Please don't tell me that this is correct by any
meaning of correct. It's not. 

The driver is perfectly fine, when the request fails. It then does the
obvious right thing to poll the card detection pin.

So your change makes it worse as it screws up the detection mechanism.

What are you actually making "work"?

Thanks,

        tglx

Re: [PATCH] irqchip/loongson-liointc: Support to set IRQ_TYPE_EDGE_BOTH

[Huacai Chen]

On Sun, Apr 6, 2025 at 10:20 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Sun, Apr 06 2025 at 20:46, Huacai Chen wrote:
> > On Sun, Apr 6, 2025 at 6:18 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> On Sun, Apr 06 2025 at 17:46, Huacai Chen wrote:
> >> > On Thu, Apr 3, 2025 at 11:48 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> >> But it won't trigger on both. So no, you cannot claim that this fixes
> >> >> anything.
> >> > Yes, it won't trigger on both (not perfect), but it allows drivers
> >> > that request "both" work (better than fail to request), and there are
> >>
> >> By some definition of 'work'. There is probably a good technical reason
> >> why those drivers expect EDGE_BOTH to work correctly and otherwise fail
> >> to load.
> > The real problem we encounter is the MMC driver. In
> > drivers/mmc/core/slot-gpio.c there is
> > devm_request_threaded_irq(host->parent, irq,
> >                         NULL, ctx->cd_gpio_isr,
> >                         IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
> > IRQF_ONESHOT,
> >                         ctx->cd_label, host);
> >
> > "IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING" is an alias of
> > "IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING", and
> > "IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING" is
> > "IRQ_TYPE_EDGE_BOTH".
>
> I know that.
>
> > Except MMC, "grep IRQ_TYPE_EDGE_BOTH drivers" can give some more examples.
>
> Sure, but you still do not explain why this works even when the driver
> is obviously depending on EDGE_BOTH. If it does not then the driver is
> bogus.
>
> Looking at it, there is obviously a reason for this particular driver to
> request BOTH. Why?
>
> This is the card change detection and it uses a GPIO. Insert raises one
> edge and remove the opposite one.
>
> Which means whatever edge you chose randomly the detection will only
> work in one direction. Please don't tell me that this is correct by any
> meaning of correct. It's not.
From experiments, either setting to EDGE_RISING or EDGE_FALLING, card
detection (inserting and removing) works. Maybe the driver request
"BOTH", but it really need "ANY"? I've searched git log, but I haven't
get any useful information.


Huacai

>
> The driver is perfectly fine, when the request fails. It then does the
> obvious right thing to poll the card detection pin.
>
> So your change makes it worse as it screws up the detection mechanism.
>
> What are you actually making "work"?
>
> Thanks,
>
>         tglx

Re: [PATCH] irqchip/loongson-liointc: Support to set IRQ_TYPE_EDGE_BOTH

[Thomas Gleixner]

On Tue, Apr 08 2025 at 20:37, Huacai Chen wrote:
> On Sun, Apr 6, 2025 at 10:20 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> This is the card change detection and it uses a GPIO. Insert raises one
>> edge and remove the opposite one.
>>
>> Which means whatever edge you chose randomly the detection will only
>> work in one direction. Please don't tell me that this is correct by any
>> meaning of correct. It's not.
>
> From experiments, either setting to EDGE_RISING or EDGE_FALLING, card
> detection (inserting and removing) works.

You might get lucky in that case because the switch bounces and there is
no debounce mechanism in place.

> Maybe the driver request "BOTH", but it really need "ANY"? I've
> searched git log, but I haven't get any useful information.

There is no maybe at all and speculation is not a technical argument.

It's well defined by the hardware:

Present           ________________
                 |               |
Not present _____|               |_______________

How can you detect that with a single edge?

If the switch bounces then the signal is:

Present           __  ___________  __
                 | | |          | | |
Not present _____| |_|          |_| |_______________

which makes it "work" by accident, but not by design.

There is ZERO guarantee that this will work with all other drivers which
request EDGE_BOTH.

Thanks,

        tglx