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] [day] [month] [year] [list]

Message-ID: <87ecpnilqw.ffs@tglx>
Date: Mon, 24 Nov 2025 14:49:27 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Cosmin-Gabriel Tanislav <cosmin-gabriel.tanislav.xa@...esas.com>, 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

On Mon, Nov 24 2025 at 12:50, Cosmin-Gabriel Tanislav wrote:
>> -----Original Message-----
>> From: Thomas Gleixner <tglx@...utronix.de>
>> Sent: Saturday, November 22, 2025 5:56 PM
>> To: Cosmin-Gabriel Tanislav <cosmin-gabriel.tanislav.xa@...esas.com>; 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>; Cosmin-Gabriel Tanislav <cosmin-
>> gabriel.tanislav.xa@...esas.com>
>> Cc: linux-kernel@...r.kernel.org; devicetree@...r.kernel.org; linux-renesas-soc@...r.kernel.org
>> Subject: Re: [PATCH 2/4] irqchip: add RZ/{T2H,N2H} Interrupt Controller (ICU) driver

Can you please fix your mail-client not to copy the whole header into
the reply?

>> 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.

> 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.

>> > +   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.

Thanks,

        tglx