linux-kernel - RE: [PATCH 2/4] irqchip: add RZ/{T2H,N2H} Interrupt Controller (ICU) driver

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:
 <TYYPR01MB139554C11AC25646D5503985385D0A@TYYPR01MB13955.jpnprd01.prod.outlook.com>
Date: Mon, 24 Nov 2025 15:28:41 +0000
From: Cosmin-Gabriel Tanislav <cosmin-gabriel.tanislav.xa@...esas.com>
To: Thomas Gleixner <tglx@...utronix.de>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	Geert Uytterhoeven <geert+renesas@...der.be>, magnus.damm
	<magnus.damm@...il.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>
Subject: RE: [PATCH 2/4] irqchip: add RZ/{T2H,N2H} Interrupt Controller (ICU)
 driver

> From: Thomas Gleixner <tglx@...utronix.de>
> Sent: Monday, November 24, 2025 3:49 PM
> 
> On Mon, Nov 24 2025 at 12:50, Cosmin-Gabriel Tanislav wrote:
> >> From: Thomas Gleixner <tglx@...utronix.de>
> >> Sent: Saturday, November 22, 2025 5:56 PM
> 
> Can you please fix your mail-client not to copy the whole header into
> the reply?
> 

Outlook, it's unfixable. I can remove it manually each time if it's
too much noise.

> >> On Fri, Nov 21 2025 at 13:14, Cosmin Tanislav wrote:
> >> > +static inline int rzt2h_icu_irq_to_offset(struct irq_data *d, void __iomem **base,
> >> > +                                     unsigned int *offset)
> >> > +{
> >> > +   struct rzt2h_icu_priv *priv = irq_data_to_priv(d);
> >> > +   unsigned int hwirq = irqd_to_hwirq(d);
> >> > +
> >> > +   if (RZT2H_ICU_IRQ_IN_RANGE(hwirq, IRQ_NS)) {
> >> > +           *offset = hwirq - RZT2H_ICU_IRQ_NS_START;
> >> > +           *base = priv->base_ns;
> >> > +   } else if (RZT2H_ICU_IRQ_IN_RANGE(hwirq, IRQ_S) ||
> >> > +              /* SEI follows safety IRQs in registers and in IRQ numbers. */
> >> > +              RZT2H_ICU_IRQ_IN_RANGE(hwirq, SEI)) {
> >>
> >> This nested commend in the condition is really unreadable.
> >>
> >
> > Would this read better in your opinion?
> >
> >         /*
> >          * Safety IRQs and SEI use a separate register space from the non-safety IRQs.
> >          * SEI interrupt number follows immediately after the safety IRQs.
> >          */
> >         if (RZT2H_ICU_IRQ_IN_RANGE(hwirq, IRQ_NS)) {
> >                 *offset = hwirq - RZT2H_ICU_IRQ_NS_START;
> >                 *base = priv->base_ns;
> >         } else if (RZT2H_ICU_IRQ_IN_RANGE(hwirq, IRQ_S) ||
> >                    RZT2H_ICU_IRQ_IN_RANGE(hwirq, SEI)) {
> >                 *offset = hwirq - RZT2H_ICU_IRQ_S_START;
> >                 *base = priv->base_s;
> >         } else {
> >                 return -EINVAL;
> >         }
> 
> Yes. Way better.
> 

Ack.

> > One more thing, for the above cases where the same macro is used twice
> > in a condition, is it okay to keep it split across two lines to align
> > them with each other, or do you want them on a single line up to 100
> > columns?
> 
> Usually single line, but in this case it might be more readable. Up to you.
> 

Ack.

> >> > +   if (!irq_domain) {
> >> > +           pm_runtime_put(dev);
> >> > +           return -ENOMEM;
> >> > +   }
> >>
> >> The mix of 'return $ERR' and 'return dev_err_probe()' is confusing at best.
> >>
> >
> > For ENOMEM, dev_err_probe() doesn't really print anything. ENOMEM is
> > what other drivers seem to use for a NULL irq_domain_create_hierarchy()
> > result.
> 
> That's what I was missing. Now it makes sense.
> 

In conclusion, should I keep the bare `return -ENOMEM` in both instances?
Just to make sure the next version is proper.

> Thanks,
> 
>         tglx