linux-kernel - Re: [PATCH v5 3/9] irqchip: rtd1195-mux: Implement irq_get_irqchip

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 12 Dec 2019 13:48:41 +0000
From:   Marc Zyngier <maz@...nel.org>
To:     Andreas Färber <afaerber@...e.de>
Cc:     <linux-realtek-soc@...ts.infradead.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>
Subject: Re: [PATCH v5 3/9] irqchip: rtd1195-mux: Implement  irq_get_irqchip_state

On 2019-11-21 05:02, Andreas Färber wrote:
> Implement the .irq_get_irqchip_state callback to retrieve pending,
> active and masked interrupt status.
>
> Signed-off-by: Andreas Färber <afaerber@...e.de>
> ---
>  v5: New
>
>  drivers/irqchip/irq-rtd1195-mux.c | 36 
> ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/drivers/irqchip/irq-rtd1195-mux.c
> b/drivers/irqchip/irq-rtd1195-mux.c
> index 0e86973aafca..2f1bcfd9d5d6 100644
> --- a/drivers/irqchip/irq-rtd1195-mux.c
> +++ b/drivers/irqchip/irq-rtd1195-mux.c
> @@ -7,6 +7,7 @@
>
>  #include <linux/bitops.h>
>  #include <linux/io.h>
> +#include <linux/interrupt.h>
>  #include <linux/irqchip.h>
>  #include <linux/irqchip/chained_irq.h>
>  #include <linux/irqdomain.h>
> @@ -96,10 +97,45 @@ static void rtd1195_mux_unmask_irq(struct 
> irq_data *data)
>  	raw_spin_unlock_irqrestore(&mux->lock, flags);
>  }
>
> +static int rtd1195_mux_get_irqchip_state(struct irq_data *data,
> +	enum irqchip_irq_state which, bool *state)
> +{
> +	struct rtd1195_irq_mux_data *mux = 
> irq_data_get_irq_chip_data(data);
> +	u32 val;
> +
> +	switch (which) {
> +	case IRQCHIP_STATE_PENDING:
> +		/*
> +		 * UMSK_ISR provides the unmasked pending interrupts,
> +		 * except UART and I2C.
> +		 */
> +		val = readl_relaxed(mux->reg_umsk_isr);
> +		*state = !!(val & BIT(data->hwirq));
> +		break;
> +	case IRQCHIP_STATE_ACTIVE:
> +		/*
> +		 * ISR provides the masked pending interrupts,
> +		 * including UART and I2C.
> +		 */
> +		val = readl_relaxed(mux->reg_isr);
> +		*state = !!(val & BIT(data->hwirq));
> +		break;

ACTIVE has a very specific meaning: it indicates that the interrupt is
being handled right now. What this tells you is whether the interrupt
is pending and unmasked, which is an entirely different thing.

This will lead to irq_disable() misbehaving (it will assume that
the interrupt is active while it is only pending).

Given what the HW exposes (or rather, what this driver exposes of the 
HW),
I don't think you can implement this state.

> +	case IRQCHIP_STATE_MASKED:
> +		val = mux->info->isr_to_int_en_mask[data->hwirq];
> +		*state = !(mux->scpu_int_en & val);

Shouldn't you take the corresponding spinlock given that you can
have a pending update in parallel?

         M.

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct irq_chip rtd1195_mux_irq_chip = {
>  	.irq_ack		= rtd1195_mux_ack_irq,
>  	.irq_mask		= rtd1195_mux_mask_irq,
>  	.irq_unmask		= rtd1195_mux_unmask_irq,
> +	.irq_get_irqchip_state	= rtd1195_mux_get_irqchip_state,
>  };
>
>  static int rtd1195_mux_irq_domain_map(struct irq_domain *d,

-- 
Jazz is not dead. It just smells funny...