| 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:
<TY2PPF5CB9A1BE6CC5EEC90EBF13B083D1EF267A@TY2PPF5CB9A1BE6.apcprd06.prod.outlook.com>
Date: Sat, 7 Feb 2026 03:50:45 +0000
From: Ryan Chen <ryan_chen@...eedtech.com>
To: Thomas Gleixner <tglx@...utronix.de>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Joel Stanley <joel@....id.au>, Andrew Jeffery <andrew@...econstruct.com.au>,
Paul Walmsley <pjw@...nel.org>, Palmer Dabbelt <palmer@...belt.com>, Albert
Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-aspeed@...ts.ozlabs.org"
<linux-aspeed@...ts.ozlabs.org>, "linux-riscv@...ts.infradead.org"
<linux-riscv@...ts.infradead.org>
Subject: RE: [PATCH 2/4] irqchip/ast2700-intcx: Add AST2700 INTC0/INTC1
support
Hello Thomas,
Thanks your feedback.
> -----Original Message-----
> From: Thomas Gleixner <tglx@...utronix.de>
> Sent: Friday, February 6, 2026 7:34 PM
> To: Ryan Chen <ryan_chen@...eedtech.com>; Rob Herring <robh@...nel.org>;
> Krzysztof Kozlowski <krzk+dt@...nel.org>; Conor Dooley
> <conor+dt@...nel.org>; Joel Stanley <joel@....id.au>; Andrew Jeffery
> <andrew@...econstruct.com.au>; Paul Walmsley <pjw@...nel.org>; Palmer
> Dabbelt <palmer@...belt.com>; Albert Ou <aou@...s.berkeley.edu>;
> Alexandre Ghiti <alex@...ti.fr>
> Cc: linux-kernel@...r.kernel.org; devicetree@...r.kernel.org;
> linux-arm-kernel@...ts.infradead.org; linux-aspeed@...ts.ozlabs.org;
> linux-riscv@...ts.infradead.org; Ryan Chen <ryan_chen@...eedtech.com>
> Subject: Re: [PATCH 2/4] irqchip/ast2700-intcx: Add AST2700 INTC0/INTC1
> support
>
> On Thu, Feb 05 2026 at 14:07, Ryan Chen wrote:
> > @@ -88,6 +88,7 @@ obj-$(CONFIG_MVEBU_PIC) +=
> irq-mvebu-pic.o
> > obj-$(CONFIG_MVEBU_SEI) += irq-mvebu-sei.o
> > obj-$(CONFIG_LS_EXTIRQ) += irq-ls-extirq.o
> > obj-$(CONFIG_LS_SCFG_MSI) += irq-ls-scfg-msi.o
> > +obj-$(CONFIG_ASPEED_AST2700_INTC) += irq-ast2700.o
> irq-ast2700-intc0.o irq-ast2700-intc1.o
> > obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o
> irq-aspeed-scu-ic.o
> > obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-intc.o
> > obj-$(CONFIG_STM32MP_EXTI) += irq-stm32mp-exti.o
> > diff --git a/drivers/irqchip/irq-ast2700-intc0-test.c
> > b/drivers/irqchip/irq-ast2700-intc0-test.c
>
> How is this kunit test supposed to be built?
Sorry, do you mean split it with Makefile?
obj-$(CONFIG_ASPEED_AST2700_INTC_TEST) += irq-ast2700-intc0-test.o
>
> Also split this kunit thing out into a separate patch. It is not relevant for the
> functional part.
Will update to separate patch
>
> > new file mode 100644
> > index 000000000000..d6bc19676b2e
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-ast2700-intc0-test.c
> > @@ -0,0 +1,474 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2026 Code Construct */ #include <kunit/test.h>
> > +#include "irq-ast2700.h"
> > +
> > +static void aspeed_intc0_resolve_route_bad_args(struct kunit *test) {
> > + static const struct aspeed_intc_interrupt_range c1ranges[] = { 0 };
> > + static const aspeed_intc_output_t c1outs[] = { 0 };
> > + struct aspeed_intc_interrupt_range resolved;
> > + const struct irq_domain c0domain = { 0 };
> > +
>
> Pointless newline
Will update.
>
> > + int rc;
> > +
> > + rc = aspeed_intc0_resolve_route(NULL, 0, c1outs, 0, c1ranges, NULL);
> > + KUNIT_EXPECT_EQ(test, rc, -EINVAL);
> > +
> > + rc = aspeed_intc0_resolve_route(&c0domain, 0, c1outs,
> > + ARRAY_SIZE(c1ranges), c1ranges,
> > + &resolved);
> > + KUNIT_EXPECT_EQ(test, rc, -ENODEV);
> > +
> > + rc = aspeed_intc0_resolve_route(&c0domain, ARRAY_SIZE(c1outs), c1outs,
> > + 0, c1ranges, &resolved);
> > + KUNIT_EXPECT_EQ(test, rc, -ENODEV);
> > +}
> > +
> > +static int
> > +arm_gicv3_fwnode_read_string_array(const struct fwnode_handle
> *fwnode_handle,
> > + const char *propname, const char **val,
> > + size_t nval)
>
> Please use the full 100 character you have available and avoid extra line breaks.
> It fits nicely into two lines:
>
> static int arm_gicv3_fwnode_read_string_array(const struct fwnode_handle
> *fwnode_handle,
> const char *propname, const char
> **val, size_t nval)
>
> There is also no real point to have these overly long function and argument
> names.
Will update
static int gicv3_fwnode_read_string_array(const struct fwnode_handle *fwnode,
const char *propname, const char **val, size_t nval)
>
> > +#include <asm-generic/errno.h>
>
> That's wrong. Include <linux/errno.h>
Will update.
>
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +#include "irq-ast2700.h"
>
> Missing newline
Will update
>
> > +#define INT_NUM 480
> > +#define INTM_NUM 50
> > +#define SWINT_NUM 16
> > +
> > +#define INTM_BASE (INT_NUM)
> > +#define SWINT_BASE (INT_NUM + INTM_NUM) #define INT0_NUM
> (INT_NUM +
> > +INTM_NUM + SWINT_NUM)
> > +
> > +#define GIC_P2P_SPI_END 128
> > +
> > +#define INTC0_SWINT_IER 0x10
> > +#define INTC0_SWINT_ISR 0x14
> > +#define INTC0_INTBANKX_IER 0x1000
> > +#define INTC0_INTBANK_GROUPS 11
> > +#define INTC0_INTBANKS_PER_GRP 3
> > +#define INTC0_INTMX_IER 0x1b00
> > +#define INTC0_INTMX_ISR 0x1b04
> > +#define INTC0_INTM_BANK_NUM 3
> > +#define INTM_IRQS_PER_BANK 10
>
> If you make these defines tabular and they become readable:
Will update
>
> #define INT_NUM 480
> #define INTM_NUM 50
> ...
> #define INTM_BASE (INT_NUM)
> ...
> #define GIC_P2P_SPI_END 128
> ...
> #define INTC0_SWINT_IER 0x10
>
> See?
Sorry, do you mean tab?
If yes, I will update all with tab
#define INT_NUM 480
#define INTM_NUM 50
#define SWINT_NUM 16
#define INTM_BASE (INT_NUM)
#define SWINT_BASE (INT_NUM + INTM_NUM)
#define INT0_NUM (INT_NUM + INTM_NUM + SWINT_NUM)
#define GIC_P2P_SPI_END 128
#define INTC0_SWINT_IER 0x10
#define INTC0_SWINT_ISR 0x14
#define INTC0_INTBANKX_IER 0x1000
#define INTC0_INTBANK_GROUPS 11
#define INTC0_INTBANKS_PER_GRP 3
#define INTC0_INTMX_IER 0x1b00
#define INTC0_INTMX_ISR 0x1b04
#define INTC0_INTM_BANK_NUM 3
#define INTM_IRQS_PER_BANK 10
>
> > +struct aspeed_intc0 {
> > + struct device *dev;
> > + void __iomem *base;
> > + raw_spinlock_t intc_lock;
> > + struct irq_domain *local;
> > + struct device_node *parent;
> > + struct aspeed_intc_interrupt_ranges ranges; };
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-de
> clarations-and-initializers
>
> I pointed you to that documentation before. Do I really have to remind you
> every couple of week?
Sorry, my fault, will update.
>
> > +
> > +static void aspeed_swint_irq_mask(struct irq_data *data) {
> > + struct aspeed_intc0 *intc0 = irq_data_get_irq_chip_data(data);
> > + int bit = data->hwirq - SWINT_BASE;
> > + unsigned int mask;
> > +
> > + guard(raw_spinlock_irqsave)(&intc0->intc_lock);
>
> s/_irqsave// Interrupts are disabled when this is invoked.
Sorry, do you mean when this function been call.
Ther have desc->lock to do the raw_spin_lock_irqsave.
Am I right?
>
> > + mask = readl(intc0->base + INTC0_SWINT_IER) & ~BIT(bit);
> > + writel(mask, intc0->base + INTC0_SWINT_IER);
> > + irq_chip_mask_parent(data);
> > +}
> > +
> > +static void aspeed_swint_irq_unmask(struct irq_data *data) {
> > + struct aspeed_intc0 *intc0 = irq_data_get_irq_chip_data(data);
> > + int bit = data->hwirq - SWINT_BASE;
> > + unsigned int unmask;
> > +
> > + guard(raw_spinlock_irqsave)(&intc0->intc_lock);
> > + unmask = readl(intc0->base + INTC0_SWINT_IER) | BIT(bit);
>
> These unmask/mask variable are simply not helpful. What's wrong with
> naming them 'ier' because that's what this is about. And while at it the data
> type for hardware related variables is u32 not unsigned int to make it clear.
>
Thanks, Will use
u32 ier;
> > +static struct irq_chip aspeed_swint_chip = {
> > + .name = "ast2700-swint",
> > + .irq_eoi = aspeed_swint_irq_eoi,
> > + .irq_mask = aspeed_swint_irq_mask,
> > + .irq_unmask = aspeed_swint_irq_unmask,
> > + .irq_set_affinity = irq_chip_set_affinity_parent,
> > + .flags = IRQCHIP_SET_TYPE_MASKED,
>
> See above
All those will review it again. Thanks.
>
> > +
> > +#define INTC0_IN_NUM 480
> > +#define INTC0_ROUTE_NUM 5
>
> Those should be at the top of the file next to the other constants.
Will update.
>
> > +static const aspeed_intc_output_t aspeed_intc0_routes[INTC0_IN_NUM /
> 32][INTC0_ROUTE_NUM] = {
> > + [0] = {
> > + [0b000] = 0,
> > + [0b001] = 256,
> > + [0b010] = 426,
> > + [0b011] = AST2700_INTC_INVALID_ROUTE,
> > + [0b100] = AST2700_INTC_INVALID_ROUTE,
>
> Seriously? What's the point of this binary notation and the insane amount of
> space this table occupies?
>
> [0] = { 0, 256, 426, AST2700_INTC_INVALID_ROUTE,
> AST2700_INTC_INVALID_ROUTE },
> [1] = { 32, 288, 458, AST2700_INTC_INVALID_ROUTE,
> AST2700_INTC_INVALID_ROUTE },
> [4] = { 128, 384, 554, 160, 176 },
> ...
>
Thanks, will update.
>
> > +
> > +#define INTC0_INTM_NUM 50
> > +
> > +static const aspeed_intc_output_t
> > + aspeed_intc0_intm_routes[INTC0_INTM_NUM / 10] = {
>
> pointless line break
>
> > + [0] = 192, /* INTM00 ~ INTM09 */
> > + [1] = 416, /* INTM10 ~ INTM19 */
> > + [2] = 586, /* INTM20 ~ INTM29 */
> > + [3] = 208, /* INTM30 ~ INTM39 */
> > + [4] = 224, /* INTM40 ~ INTM49 */
> > + };
> > +
> > +static bool range_contains_element(u32 start, u32 count, u32 value)
>
> in_range32() provides that already
Will update.
>
> > +{
> > + if (WARN_ON_ONCE((U32_MAX - count) < start))
> > + return false;
> > +
> > + return value >= start && value < start + count; }
> > +
> > +static int
> > +resolve_input_from_child_ranges(const struct aspeed_intc0 *intc0,
> > + const struct aspeed_intc_interrupt_range *range,
> > + u32 outpin, u32 *input)
> > +{
> > + u32 offset;
> > + u32 base;
>
> One line
Will update
>
> > +
> > + if (!range_contains_element(range->start, range->count, outpin))
> > + return -ENOENT;
> > +
> > + if (range->upstream.param_count == 0)
> > + return -EINVAL;
> > +
> > + base = range->upstream.param[0];
> > + offset = outpin - range->start;
> > + if ((U32_MAX - offset) < base) {
>
> if (!in_range32(...)
Will update
if (offset && !in_range32(base, 0, U32_MAX - offset + 1)) {
>
>
> > + dev_warn(intc0->dev,
> > + "%s: Arithmetic overflow for input derivation: %u + %u\n",
>
> Pointless line break. Please fix them all over the place.
Will update
dev_warn(intc0->dev, "%s: Arithmetic overflow for input derivation: %u + %u\n",
__func__, base, offset);
>
> > + __func__, base, offset);
> > + return -EINVAL;
> > + }
> > +
> > + *input = base + offset;
> > + return 0;
> > +static int resolve_parent_route_for_input(const struct aspeed_intc0 *intc0,
> > + const struct fwnode_handle *parent, u32 input,
> > + struct aspeed_intc_interrupt_range *resolved) {
> > + aspeed_intc_output_t c0o;
> > + int rc = -ENOENT;
> > +
> > + if (input < INT_NUM) {
> > + bool found;
> > +
> > + dev_dbg(intc0->dev, "%s: Resolving parent route for linear input
> %u\n",
> > + __func__, input);
>
> Do you really still need all those debug prints or are you trusting your code by
> now?
>
> If they still are considered valuable then shorten them in a sensible way so they
> nicely hide in the code instead of cluttering it to the point of making it
> unreadable.
Will try to remove dev_dbg.
>
> > +{
> > + struct aspeed_intc0 *intc0;
> > + struct fwnode_handle *parent_fwnode;
> > + int ret;
>
> See Documentation about variable declarations...
Will update
struct fwnode_handle *parent_fwnode;
struct aspeed_intc0 *intc0;
int ret;
>
> > + for (size_t i = 0; i < nc1outs; i++) {
> > + aspeed_intc_output_t c1o = c1outs[i];
> > +
> > + if (c1o == AST2700_INTC_INVALID_ROUTE) {
> > + dev_dbg(intc0->dev, "%s: Invalid output at route index %zu\n",
> > + __func__, i);
> > + continue;
> > + }
> > +
> > + dev_dbg(intc0->dev, "%s: Have output %u for route index %zu\n",
> > + __func__, c1o, i);
> > +
> > + for (size_t j = 0; j < nc1ranges; j++) {
> > + struct aspeed_intc_interrupt_range c1r = c1ranges[j];
> > + u32 input;
> > +
> > + dev_dbg(intc0->dev,
> > + "%s: Inspecting candidate range %zu starting at %u for
> %u\n",
> > + __func__, j, c1r.start, c1r.count);
> > +
> > + /*
> > + * Range match for intc1 output pin
> > + *
> > + * Assume a failed match is still a match for the purpose of
> testing,
> > + * saves a bunch of mess in the test fixtures
> > + */
> > + if (!(c0domain == irq_find_matching_fwspec(&c1r.upstream,
> > + c0domain->bus_token) ||
> > + IS_ENABLED(CONFIG_ASPEED_AST2700_INTC_TEST))) {
> > + dev_dbg(intc0->dev, "%s: Parent mismatch for candidate
> range %zu\n",
> > + __func__, j);
> > + continue;
> > + }
> > +
> > + ret = resolve_input_from_child_ranges(intc0, &c1r, c1o, &input);
> > + if (ret) {
> > + if (ret == -ENOENT)
> > + dev_dbg(intc0->dev,
> > + "%s: Output %u not in candidate range %zu
> starting at %u for %u\n",
> > + __func__, c1o, j, c1r.start, c1r.count);
> > + continue;
>
> All of this is unreadable and I told you about the bracket rules before, no?
Will remove dev_dbg with following
ret = resolve_input_from_child_ranges(intc0, &c1r, c1o, &input);
if (ret)
continue;
>
> > + }
> > + dev_dbg(intc0->dev,
> > + "%s: Resolved INTC0 input to %u using candidate range %zu:
> [%u, %u)\n",
> > + __func__, input, j, c1r.start, c1r.start + c1r.count);
> > +
> > + /*
> > + * INTC1 should never request routes for peripheral interrupt
> sources
> > + * directly attached to INTC0.
> > + */
> > + if (input < GIC_P2P_SPI_END) {
> > + dev_dbg(intc0->dev,
> > + "%s: Invalid range specification at index %zu routed
> INTC1 output to unreachable INTC0 input\n",
> > + __func__, j);
> > + continue;
> > + }
> > +
> > + ret = resolve_parent_route_for_input(intc0, parent_fwnode,
> input, NULL);
> > + if (ret < 0)
> > + continue;
> > +
> > + /* Route resolution succeeded */
> > + resolved->start = c1o;
> > + resolved->count = 1;
> > + resolved->upstream = c1r.upstream;
> > + resolved->upstream.param[0] = input;
> > + dev_dbg(intc0->dev,
> > + "%s: Route resolution selected INTC1 output %u via index
> %zu\n",
> > + __func__, c1o, i);
> > + /* Cast protected by prior test against nc1outs */
> > + return (int)i;
> > + }
> > + }
> > +
> > + ret = -EHOSTUNREACH;
> > + return ret;
>
> Impressive.
Will try to improve this function.
>
> > +}
> > +EXPORT_SYMBOL_GPL(aspeed_intc0_resolve_route);
>
> What is this export for? All usage sites are built in, no?
Will remove this.
>
> > +static int aspeed_intc0_irq_domain_map(struct irq_domain *domain,
> > + unsigned int irq, irq_hw_number_t hwirq) {
> > + if (hwirq < GIC_P2P_SPI_END)
> > + irq_set_chip_and_handler(irq, &linear_intr_irq_chip,
> > + handle_level_irq);
>
> Make this one line. Otherwise you need brackets.
Will update all
if (hwirq < GIC_P2P_SPI_END)
irq_set_chip_and_handler(irq, &linear_intr_irq_chip, handle_level_irq);
else if (hwirq < INTM_BASE)
return -EINVAL;
else if (hwirq < SWINT_BASE)
irq_set_chip_and_handler(irq, &aspeed_intm_chip, handle_level_irq);
else if (hwirq < INT0_NUM)
irq_set_chip_and_handler(irq, &aspeed_swint_chip, handle_level_irq);
else
return -EINVAL;
>
> > + else if (hwirq < INTM_BASE)
> > + return -EINVAL;
> > + else if (hwirq < SWINT_BASE)
> > + irq_set_chip_and_handler(irq, &aspeed_intm_chip,
> > + handle_level_irq);
> > + else if (hwirq < INT0_NUM)
> > + irq_set_chip_and_handler(irq, &aspeed_swint_chip,
> > + handle_level_irq);
> > + else
> > + return -EINVAL;
> > +
> > + irq_set_chip_data(irq, domain->host_data);
> > + return 0;
> > +}
>
> > +static int aspeed_intc0_irq_domain_activate(struct irq_domain *domain,
> > + struct irq_data *data, bool reserve) {
> > + struct aspeed_intc0 *intc0 = irq_data_get_irq_chip_data(data);
> > +
> > + if (data->hwirq < INT_NUM) {
> > + int bank = data->hwirq / 32;
> > + int bit = data->hwirq % 32;
> > + u32 mask = BIT(bit);
> > + int route;
> > +
> > + route = resolve_parent_route_for_input(intc0,
> > + intc0->local->parent->fwnode,
> > + data->hwirq, NULL);
> > + if (route < 0)
> > + return route;
> > +
> > + guard(raw_spinlock_irqsave)(&intc0->intc_lock);
> > + for (int i = 0; i < 3; i++) {
> > + void __iomem *sel = intc0->base + 0x200 + bank * 4 + 0x100 *
> i;
>
> Magic constants 0x200 4 and 0x100. Use proper defines and a macro/inline to
> calculate the register address and not this incomprehensible garbage.
Will update with #define
>
> > + u32 reg = readl(sel);
> > +
> > + if (route & BIT(i))
> > + reg |= mask;
> > + else
> > + reg &= ~mask;
> > +
> > + writel(reg, sel);
> > + if (readl(sel) != reg)
> > + return -EACCES;
> > + }
> > + } else if (data->hwirq < INT0_NUM) {
> > + return 0;
> > + } else {
> > + return -EINVAL;
> > + }
>
> If you rearrange those conditions you can avoid the indentation all together
>
> if (in_range32(data->hwirq, INTM_BASE, INTM_NUM +
> SWINT_NUM))
> return 0;
> if (data->hwirq >= INT_NUM)
> return -EINVAL;
>
> No?
Got it will update
>
> > +
> > + return 0;
> > +}
> > +
> > +static void aspeed_intc0_disable_intbank(struct aspeed_intc0 *intc0)
> > +{
> > + int i, j;
> > +
> > + for (i = 0; i < INTC0_INTBANK_GROUPS; i++) {
> > + for (j = 0; j < INTC0_INTBANKS_PER_GRP; j++) {
>
> Both i and j should be declared in the for () statement. Your coding style is so
> inconsistent it's not even funny anymore.
Will update.
>
> > + u32 base = INTC0_INTBANKX_IER + (0x100 * i) + (0x10 * j);
> > +
> > + writel(0, intc0->base + base);
> > + }
> > + }
> > +}
> > +IRQCHIP_PLATFORM_DRIVER_BEGIN(ast2700_intc0)
> > +IRQCHIP_MATCH("aspeed,ast2700-intc0-ic", aspeed_intc0_ic_probe)
> > +IRQCHIP_PLATFORM_DRIVER_END(ast2700_intc0)
> > +
> > +#ifdef CONFIG_ASPEED_AST2700_INTC_TEST #include
> > +"irq-ast2700-intc0-test.c"
> > +#endif
>
> Yikes. What's wrong with Makefile?
Will remove it.
And add it in Makefile
obj-$(CONFIG_ASPEED_AST2700_INTC_TEST) += irq-ast2700-intc0-test.o
>
> > diff --git a/drivers/irqchip/irq-ast2700-intc1.c
> > b/drivers/irqchip/irq-ast2700-intc1.c
> > +static void aspeed_intc1_ic_irq_handler(struct irq_desc *desc) {
> > + struct aspeed_intc1 *intc1 = irq_desc_get_handler_data(desc);
> > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > + unsigned long bit, status;
> > +
> > + chained_irq_enter(chip, desc);
> > +
> > + for (int bank = 0; bank < INTC1_BANK_NUM; bank++) {
> > + status = readl(intc1->base + INTC1_ISR + (0x10 * bank));
> > + if (!status)
> > + continue;
> > +
> > + for_each_set_bit(bit, &status, INTC1_IRQS_PER_BANK) {
> > + generic_handle_domain_irq(intc1->local,
> > + (bank * INTC1_IRQS_PER_BANK) +
> > + bit);
> > + writel(BIT(bit),
> > + intc1->base + INTC1_ISR + (0x10 * bank));
>
> My eyes bleed by now.
I will modify the some input in intc1.c
Thanks your review.
>
> Thanks,
>
> tglx
Powered by blists - more mailing lists