| 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
| ||
|
Message-ID:
<TYYPR01MB139556A313B1377F9306A7F6485D0A@TYYPR01MB13955.jpnprd01.prod.outlook.com>
Date: Mon, 24 Nov 2025 12:50:46 +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
> -----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
>
> 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;
}
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?
> > + *offset = hwirq - RZT2H_ICU_IRQ_S_START;
> > + *base = priv->base_s;
> > + } else {
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int rzt2h_icu_irq_set_type(struct irq_data *d, unsigned int type)
> > +{
> > + struct rzt2h_icu_priv *priv = irq_data_to_priv(d);
> > + unsigned int parent_type;
> > + unsigned int offset;
>
> Combine same data types into one line please.
>
Ack.
> > + void __iomem *base;
> > + u32 val, md;
> > + int ret;
>
>
> > + guard(raw_spinlock)(&priv->lock);
> > + val = readl_relaxed(base + RZT2H_ICU_PORTNF_MD);
> > + val &= ~RZT2H_ICU_PORTNF_MDi_MASK(offset);
> > + val |= RZT2H_ICU_PORTNF_MDi_PREP(offset, md);
> > + writel_relaxed(val, base + RZT2H_ICU_PORTNF_MD);
> > +
>
> This looks wrong. guard() holds the lock across the set_parent()
> call. If you really need that then this needs a comment explaining the
> why. Otherwise use scoped_guard().
>
Ack.
> > + return irq_chip_set_type_parent(d, parent_type);
> > +}
> > +static const struct irq_chip rzt2h_icu_chip = {
> > + .name = "rzt2h-icu",
> > + .irq_mask = irq_chip_mask_parent,
> > + .irq_unmask = irq_chip_unmask_parent,
> > + .irq_eoi = irq_chip_eoi_parent,
> > + .irq_set_type = rzt2h_icu_set_type,
> > + .irq_set_wake = irq_chip_set_wake_parent,
> > + .irq_set_affinity = irq_chip_set_affinity_parent,
> > + .irq_retrigger = irq_chip_retrigger_hierarchy,
> > + .irq_get_irqchip_state = irq_chip_get_parent_state,
> > + .irq_set_irqchip_state = irq_chip_set_parent_state,
> > + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SET_TYPE_MASKED |
> > + IRQCHIP_SKIP_SET_WAKE,
>
> https://www.kernel.org/doc/html/latest%25
> 2Fprocess%2Fmaintainer-tip.html%23struct-declarations-and-initializers&data=05%7C02%7Ccosmin-
> gabriel.tanislav.xa%40renesas.com%7C377460d2de2a4899c85f08de29df9604%7C53d82571da1947e49cb4625a166a4a2a
> %7C0%7C0%7C638994237471295714%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiO
> iJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=K%2FjQlj7d2xbeyIrYLSmDoI90CNwyndz%2B0nkU
> j%2Bu6fn0%3D&reserved=0
>
> And please read and follow the rest of the documentation too.
>
Ack.
> > +};
> > +
> > +static int rzt2h_icu_alloc(struct irq_domain *domain, unsigned int virq,
> > + unsigned int nr_irqs, void *arg)
> > +{
> > + struct rzt2h_icu_priv *priv = domain->host_data;
> > + irq_hw_number_t hwirq;
> > + unsigned int type;
> > + int ret;
> > +
> > + ret = irq_domain_translate_twocell(domain, arg, &hwirq, &type);
> > + if (ret)
> > + return ret;
> > +
> > + ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &rzt2h_icu_chip,
> > + NULL);
>
> Get rid of the line breaks all over the place. You have 100 characters.
>
Ack.
> > + if (ret)
> > + return ret;
> > +
> > + return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
> > + &priv->fwspec[hwirq]);
> > +}
>
>
> > +static int rzt2h_icu_init(struct platform_device *pdev,
> > + struct device_node *parent)
> > +{
> > + struct irq_domain *irq_domain, *parent_domain;
> > + struct device_node *node = pdev->dev.of_node;
> > + struct device *dev = &pdev->dev;
> > + struct rzt2h_icu_priv *priv;
> > + int ret;
> > +
> > + parent_domain = irq_find_host(parent);
> > + if (!parent_domain)
> > + return dev_err_probe(dev, -ENODEV, "cannot find parent domain\n");
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, priv);
> > +
> > + priv->base_ns = devm_of_iomap(dev, dev->of_node, 0, NULL);
> > + if (IS_ERR(priv->base_ns))
> > + return PTR_ERR(priv->base_ns);
> > +
> > + priv->base_s = devm_of_iomap(dev, dev->of_node, 1, NULL);
> > + if (IS_ERR(priv->base_s))
> > + return PTR_ERR(priv->base_s);
> > +
> > + ret = rzt2h_icu_parse_interrupts(priv, node);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "cannot parse interrupts: %d\n", ret);
> > +
> > + ret = devm_pm_runtime_enable(dev);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "devm_pm_runtime_enable failed: %d\n", ret);
> > +
> > + ret = pm_runtime_resume_and_get(dev);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "pm_runtime_resume_and_get failed: %d\n", ret);
> > +
> > + raw_spin_lock_init(&priv->lock);
> > +
> > + irq_domain = irq_domain_create_hierarchy(parent_domain, 0, RZT2H_ICU_NUM_IRQ,
> > + dev_fwnode(dev),
> > + &rzt2h_icu_domain_ops, priv);
> > + 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.
Do you want me to use a different error code and switch to
dev_err_probe(), or keep ENOMEM and switch to dev_err_probe() for
uniformity anyway?
What should be done for the devm_kzalloc() failure case at the top of
rzt2h_icu_init()?
> Thanks,
>
> tglx
Powered by blists - more mailing lists