Re: [PATCH v9 4/4] irqchip/irq-loongarch-ir:Add Redirect irqchip support

[Tianyang Zhang]

Hi, Thomas

在 2026/1/30 下午4:54, Thomas Gleixner 写道:
> On Fri, Jan 30 2026 at 10:59, Tianyang Zhang wrote:
> > +// SPDX-License-Identifier: GPL-2.0
> GPL-2.0-only please
Ok, I got it
> > +/*
> > + * Copyright (C) 2020 Loongson Technologies, Inc.
> This was written 6 years ago already?
This was copied from other files, and I will rework it, thanks
> > + */
> > +
> > +#include <linux/cpuhotplug.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqchip.h>
> > +#include <linux/irqchip/irq-msi-lib.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/msi.h>
> Includes should be alphabetically ordered.
Ok, I got it
> > +#define REDIRECT_REG(reg, node) \
> > +	((void __iomem *)(IO_BASE | redirect_reg_base | (u64)(node) << NODE_ADDRSPACE_SHIFT | (reg)))
> > +
> > +static inline void redirect_write_reg64(u32 node, u64 val, u32 reg)
> > +{
> > +	void __iomem *reg_addr = REDIRECT_REG(reg, node);
> > +
> > +	return writeq(val, reg_addr);
> Bogus return and you can simplify this to
>
>        writeq(val, REDIRECT_REG(reg, node));
>
> No?
Ok, this is more reasonable.
> > +}
> > +
> > +static inline u32 redirect_read_reg32(u32 node, u32 reg)
> > +{
> > +	void __iomem *reg_addr = REDIRECT_REG(reg, node);
> > +
> > +	return readl(reg_addr);
> Condense to single line as well
Ok, I got it
> > +static void irde_invalid_entry(struct redirect_item *item)
> This should be named irde_invalidate_entry() as that's what the function
> is about. irq_invalid_entry() reads more like a function which check for
> an invalid entry.
Ok, I got it
> > +{
> > +	struct irde_inv_cmd cmd;
> > +	u64 raddr = 0;
> > +
> > +	cmd.cmd_info = 0;
> > +	cmd.index.type = INVALID_INDEX;
> > +	cmd.index.need_notice = 1;
> > +	cmd.index.index = item->index;
> > +	cmd.notice_addr = (u64)(__pa(&raddr));
> > +
> > +	invalid_enqueue(item, &cmd);
> > +
> > +	/*
> > +	 * CPU needs to wait here for cmd to complete, and it determines this
> The CPU
Ok, I got it, thanks
> > +	 * by checking whether invalid queue has already written a valid value
> whether the invalidation queue
Ok, I got it, thanks
> > +static int redirect_table_alloc(int node, u32 nr_irqs)
> > +{
> > +	struct redirect_table *ird_table = &irde_descs[node].ird_table;
> > +	unsigned int index, order;
> > +
> > +	if (nr_irqs > 1) {
> > +		nr_irqs = __roundup_pow_of_two(nr_irqs);
> > +		order = ilog2(nr_irqs);
> > +	}
> > +
> > +	guard(raw_spinlock_irqsave)(&ird_table->lock);
> > +
> > +	index = bitmap_find_free_region(ird_table->bitmap,
> > +					IRD_ENTRIES, order);
> Get rid of this pointless line break. You have 100 characters and the
> above even fits into 80
Ok, I got it
> > +	if (index < 0) {
> > +		pr_err("No redirect entry to use\n");
> > +		return -ENOMEM;
> > +	}
> > +static int redirect_domain_alloc(struct irq_domain *domain, unsigned int virq,
> > +				 unsigned int nr_irqs, void *arg)
> > +{
> > +	msi_alloc_info_t *info = arg;
> > +	int ret, i, node, index;
> > +
> > +	node = dev_to_node(info->desc->dev);
> > +
> > +	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, arg);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	index = redirect_table_alloc(node, nr_irqs);
> > +	if (index < 0) {
> > +		pr_err("Alloc redirect table entry failed\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	for (i = 0; i < nr_irqs; i++) {
> > +		struct irq_data *irq_data = irq_domain_get_irq_data(domain, virq + i);
> > +		struct redirect_item *item;
> > +
> > +		item = kzalloc(sizeof(*item), GFP_KERNEL);
> > +		if (!item) {
> > +			pr_err("Alloc redirect descriptor failed\n");
> > +			goto out_free_resources;
> > +		}
> > +		item->irde = &irde_descs[node];
> > +
> > +		/*
> > +		 * Only bits 47:6 of the GPID are passed to the controller,
> > +		 * 64-byte alignment must be guarantee and make kzalloc can
> > +		 * align to the respective size.
> -ENOPARSE
Ok ,I got it
> > +		 */
> > +static const struct irq_domain_ops redirect_domain_ops = {
> > +	.alloc		= redirect_domain_alloc,
> > +	.free		= redirect_domain_free,
> > +	.select		= msi_lib_irq_domain_select,
> > +};
> > +static void __redirect_irde_fini(struct irde_desc *irde)
> This schoolbook _fini() naming is just lame and nondescriptive. Please
> use descriptive function names which make it clear what this is about,
> e.g. redirect_free_irde() or something like that.
>
> Also this should be __init, no?
Yes , This code style is indeed not very appropriate, I will adjust the 
function name
> > +{
> > +	struct redirect_table *ird_table = &irde_descs->ird_table;
> > +	struct redirect_queue *inv_queue = &irde_descs->inv_queue;
> > +static inline void redirect_irde_fini(int node)
> > +{
> > +	__redirect_irde_fini(&irde_descs[node]);
> This indirection is really pointless. Just move the '&irde_descs[node]' to
> the only caller.
Ok, I got it
> > +int __init redirect_acpi_init(struct irq_domain *parent)
> > +{
> > +	struct fwnode_handle *fwnode;
> > +	int ret = -EINVAL, node;
> > +
> > +	fwnode = irq_domain_alloc_named_fwnode("redirect");
> > +	if (!fwnode) {
> > +		pr_err("Unable to alloc redirect domain handle\n");
> > +		goto fail;
> > +	}
> > +
> > +	redirect_domain = irq_domain_create_hierarchy(parent, 0, IRD_ENTRIES, fwnode,
> > +						      &redirect_domain_ops, irde_descs);
> > +	if (!redirect_domain) {
> > +		pr_err("Unable to alloc redirect domain\n");
> > +		goto out_free_fwnode;
> > +	}
> > +
> > +
> stray newline
Ok, I got it
> > +	for_each_node_mask(node, node_possible_map) {
> > +		ret = redirect_irde_init(node);
> > +		if (ret)
> > +			goto out_clear_irde;
> > +	}
> > +
> > +	ret = acpi_cascade_irqdomain_init();
> > +	if (ret < 0) {
> > +		pr_err("Failed to cascade IRQ domain, ret=%d\n", ret);
> > +		goto out_clear_irde;
> > +	}
> > +
> > +	pr_info("loongarch irq redirect modules init succeeded\n");
> You really want to have:
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> at the top of the file, so that all printk()s in this file are properly
> prefixed.
Ok, I got it, thanks
> Thanks,
>
>          tglx

Tiyang