linux-kernel - Re: [RFC v7 3/6] irqchip: add mpfs gpio interrupt mux

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20240801-palpitate-swinger-7bc8ae8deaaf@spud>
Date: Thu, 1 Aug 2024 16:09:49 +0100
From: Conor Dooley <conor@...nel.org>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Conor Dooley <conor.dooley@...rochip.com>, linux-kernel@...r.kernel.org,
	Marc Zyngier <maz@...nel.org>,
	Daire McNamara <daire.mcnamara@...rochip.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	linux-riscv@...ts.infradead.org, linux-gpio@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [RFC v7 3/6] irqchip: add mpfs gpio interrupt mux

On Mon, Jul 29, 2024 at 12:41:25PM +0200, Thomas Gleixner wrote:
> On Tue, Jul 23 2024 at 12:27, Conor Dooley wrote:
> > +
> > +struct mpfs_irq_mux_bank_config {
> > +	u32 mask;
> > +	u8 shift;
> > +};
> 
> Please see:
> 
>   https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
> 
> vs. coding style.
> 
> > +/*
> > + * Returns an unsigned long, where a set bit indicates the corresponding
> > + * interrupt is in non-direct/muxed mode for that bank/GPIO controller.
> > + */
> > +static inline unsigned long mpfs_irq_mux_get_muxed_irqs(struct mpfs_irq_mux *priv,
> > +							unsigned int bank)
> > +{
> > +	unsigned long mux_config = priv->mux_config, muxed_irqs = -1;
> > +	struct mpfs_irq_mux_bank_config bank_config = mpfs_irq_mux_bank_configs[bank];
> > +
> > +	/*
> > +	 * If a bit is set in the mux, GPIO the corresponding interrupt from
> > +	 * controller 2 is direct and that controllers 0 or 1 is muxed.
> 
> This is not a coherent sentence.

It should read "controller 0 or 1;s interrupt is muxed". Does that make
more sense to you?

> > +	 * Invert the bits in the configuration register, so that set bits
> > +	 * equate to non-direct mode, for GPIO controller 2.
> > +	 */
> > +	if (bank == 2u)
> > +		mux_config = ~mux_config;
> > +
> 
> > +static int mpfs_irq_mux_nondirect_alloc(struct irq_domain *d, unsigned int virq,
> > +					struct irq_fwspec *fwspec, struct mpfs_irq_mux *priv)
> > +{
> > +	unsigned int bank = fwspec->param[0] / MPFS_MAX_IRQS_PER_GPIO;
> > +
> > +	if (bank > 2)
> > +		return -EINVAL;
> > +
> > +	priv->nondirect_irqchips[bank].domain = d;
> > +
> > +	irq_domain_set_hwirq_and_chip(d, virq, fwspec->param[0],
> > +				      &mpfs_irq_mux_nondirect_irq_chip, priv);
> > +	irq_set_chained_handler_and_data(virq, handle_untracked_irq,
> 
> Why does this use handle_untracked_irq()?

I'll have to go and dig back in my notes as to why it is untracked. It
was probably something like irqd_set() in handle_irq_event() blowing up
on the irq_data being invalid (which I figure could relate back to my
questions in the cover letter about issues with irqd_to_hwirq()) - but
I'll double check what exactly prompted it when I get back from my
holidays, but...

> This sets up a chained handler
> but handle_untracked_irq() is a regular interrupt handler.

...what I was likely using before was handle_simple_irq() which isn't
chained either. You're expecting to see mpfs_irq_mux_nondirect_handler()
here I suppose?

>+static void mpfs_irq_mux_nondirect_handler(struct irq_desc *desc)
>+{
>+	struct mpfs_irq_mux_irqchip *irqchip_data = irq_desc_get_handler_data(desc);
>+	struct mpfs_irq_mux *priv = container_of(irqchip_data, struct mpfs_irq_mux,
>+						 nondirect_irqchips[irqchip_data->bank]);
>+	unsigned long muxed_irqs;
>+	int pos;
>+
>+	chained_irq_enter(irq_desc_get_chip(desc), desc);
>+
>+	muxed_irqs = mpfs_irq_mux_get_muxed_irqs(priv, irqchip_data->bank);
>+
>+	for_each_set_bit(pos, &muxed_irqs, MPFS_MAX_IRQS_PER_GPIO)
>+		generic_handle_domain_irq(irqchip_data->domain, irqchip_data->offset + pos);
>+
>+	chained_irq_exit(irq_desc_get_chip(desc), desc);
>+}

Given you've only commented on one significant issue and two minor items,
is it safe to conclude that the overall approach doesn't have you
screaming and running for the hills?

Cheers,
Conor.

> > +					 &priv->nondirect_irqchips[bank]);

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)