Message-ID: <2da824f7-f2bb-3cf8-7012-963f034837f3@loongson.cn>
Date: Mon, 17 Nov 2025 11:15:52 +0800
From: Tianyang Zhang <zhangtianyang@...ngson.cn>
To: Thomas Gleixner <tglx@...utronix.de>, chenhuacai@...nel.org,
 kernel@...0n.name, akpm@...ux-foundation.org, willy@...radead.org,
 david@...hat.com, linmag7@...il.com, thuth@...hat.com, maobibo@...ngson.cn,
 apopple@...dia.com
Cc: loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org,
 Liupu Wang <wangliupu@...ngson.cn>
Subject: Re: [PATCH v6 2/2] irqchip/irq-loongarch-ir:Add Redirect irqchip
 support

Hi, Thomas

在 2025/11/13 下午9:46, Thomas Gleixner 写道:
> On Thu, Nov 13 2025 at 11:49, Tianyang Zhang wrote:
>>   arch/loongarch/include/asm/cpu-features.h |   1 +
>>   arch/loongarch/include/asm/cpu.h          |   2 +
>>   arch/loongarch/include/asm/loongarch.h    |   6 +
>>   arch/loongarch/kernel/cpu-probe.c         |   2 +
> Can you please split the architecture parts out into a separate patch?
Ok, I will split the patch
>
>>   drivers/irqchip/Makefile                  |   2 +-
>>   drivers/irqchip/irq-loongarch-avec.c      |  20 +-
>>   drivers/irqchip/irq-loongarch-ir.c        | 527 ++++++++++++++++++++++
>>   drivers/irqchip/irq-loongson.h            |  19 +
>>   8 files changed, 565 insertions(+), 14 deletions(-)
>>   create mode 100644 drivers/irqchip/irq-loongarch-ir.c
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index 93e3ced023bb..a0be18891890 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -118,7 +118,7 @@ obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
>>   obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
>>   obj-$(CONFIG_TI_SCI_INTA_IRQCHIP)	+= irq-ti-sci-inta.o
>>   obj-$(CONFIG_TI_PRUSS_INTC)		+= irq-pruss-intc.o
>> -obj-$(CONFIG_IRQ_LOONGARCH_CPU)		+= irq-loongarch-cpu.o irq-loongarch-avec.o
>> +obj-$(CONFIG_IRQ_LOONGARCH_CPU)		+= irq-loongarch-cpu.o irq-loongarch-avec.o irq-loongarch-ir.o
>>   obj-$(CONFIG_LOONGSON_LIOINTC)		+= irq-loongson-liointc.o
>>   obj-$(CONFIG_LOONGSON_EIOINTC)		+= irq-loongson-eiointc.o
>>   obj-$(CONFIG_LOONGSON_HTPIC)		+= irq-loongson-htpic.o
>> diff --git a/drivers/irqchip/irq-loongarch-avec.c b/drivers/irqchip/irq-loongarch-avec.c
>> index bf52dc8345f5..de7b6059c1b6 100644
>> --- a/drivers/irqchip/irq-loongarch-avec.c
>> +++ b/drivers/irqchip/irq-loongarch-avec.c
>> @@ -24,7 +24,6 @@
>>   #define VECTORS_PER_REG		64
>>   #define IRR_VECTOR_MASK		0xffUL
>>   #define IRR_INVALID_MASK	0x80000000UL
>> -#define AVEC_MSG_OFFSET		0x100000
>>   
>>   #ifdef CONFIG_SMP
>>   struct pending_list {
>> @@ -47,15 +46,6 @@ struct avecintc_chip {
>>   
>>   static struct avecintc_chip loongarch_avec;
>>   
>> -struct avecintc_data {
>> -	struct list_head	entry;
>> -	unsigned int		cpu;
>> -	unsigned int		vec;
>> -	unsigned int		prev_cpu;
>> -	unsigned int		prev_vec;
>> -	unsigned int		moving;
>> -};
>> -
> ...
>> -static void avecintc_sync(struct avecintc_data *adata)
>> +void avecintc_sync(struct avecintc_data *adata)
> Moving the struct to the header and exposing this function should be a
> separate patch as well.
Ok, I got it
>
>
>>   {
>>   	struct pending_list *plist;
>>   
>> @@ -109,7 +99,7 @@ static int avecintc_set_affinity(struct irq_data *data, const struct cpumask *de
>>   			return -EBUSY;
>>   
>>   		if (cpu_online(adata->cpu) && cpumask_test_cpu(adata->cpu, dest))
>> -			return 0;
>> +			return IRQ_SET_MASK_OK_DONE;
> Changing that return value wants to be a separate patch with an
> explanation why this is done and why it's functionaly equivalent.
Ok, I got it
>
>> +#define IRD_ENTRY_SIZE			16
> Shouldn't that be sizeof(struct redirect_entry) or if you want to keep
> it this way then you want a compile time assert that ensures that
> sizeof(struct redirect_entry) == IRD_ENTRY_SIZE.
Ok, I got it
>> +#define REDIRECT_REG(reg, node) \
>> +	((void __iomem *)(IO_BASE | redirect_reg_base | (u64)(node) << NODE_ADDRSPACE_SHIFT | (reg)))
>> +
> This only works when @node is guaranteed to be a real node number and
> not NUMA_NO_NODE.
Indeed, the node's value is sourced from irde->node in the following 
process.

static void redirect_irde_cfg(struct irde_desc *irde)
{
         redirect_write_reg64(irde->node, CFG_DISABLE_IDLE, 
LOONGARCH_IOCSR_REDIRECT_CFG);
         redirect_write_reg64(irde->node, __pa(irde->ird_table.table), 
LOONGARCH_IOCSR_REDIRECT_TBR);
         redirect_write_reg32(irde->node, 0, LOONGARCH_IOCSR_REDIRECT_CQH);
         redirect_write_reg32(irde->node, 0, LOONGARCH_IOCSR_REDIRECT_CQT);
         redirect_write_reg64(irde->node, ((u64)irde->inv_queue.cmd_base 
& CQB_ADDR_MASK) |
                              CQB_SIZE_MASK, LOONGARCH_IOCSR_REDIRECT_CQB);
}
Once the irde->node is initialized, it remains unchanged permanently, 
which should ensure its correctness.

int __init redirect_acpi_init(struct irq_domain *parent)

{
...
         for_each_node_mask(node, node_possible_map)
                 ret = redirect_irde_init(node);

}

>
>> +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);
>> +}
>> +
>> +static inline void redirect_write_reg32(int node, u32 val, u32 reg)
> Can you make your mind up and use a consistent data type for @node?
Ok, I got it
>
>> +static void irde_invalid_entry_node(struct redirect_item *item)
>> +{
>> +	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);
>> +
>> +	while (!raddr)
> Please add a comment what this @raddr magic does.
Ok, I will do that
>
>> +		cpu_relax();
>> +
>> +}
>> +
>> +static inline struct avecintc_data *irq_data_get_avec_data(struct irq_data *data)
>> +{
>> +	return data->parent_data->chip_data;
>> +}
>> +
>> +static int redirect_table_alloc(struct redirect_item *item)
>> +{
>> +	struct redirect_table *ird_table = &item->irde->ird_table;
>> +	int index;
>> +
>> +	guard(raw_spinlock_irqsave)(&ird_table->lock);
>> +
>> +	index = find_first_zero_bit(ird_table->bitmap, IRD_ENTRIES);
>> +	if (index > IRD_ENTRIES) {
> Off by one. Valid index is 0 ... (IRD_ENTRIES - 1)
Ok , I got it ,  it is an illegal critical condition. Thank you.
>
>> +static void redirect_table_free(struct redirect_item *item)
>> +{
>> +	struct redirect_table *ird_table = &item->irde->ird_table;
>> +	struct redirect_entry *entry = item_get_entry(item);
>> +
>> +	memset(entry, 0, sizeof(*entry));
>> +
>> +	scoped_guard(raw_spinlock_irq, &ird_table->lock)
>> +		bitmap_release_region(ird_table->bitmap, item->index, 0);
> That's silly. bitmap_release_region(..., order = 0) is equivalent to
> clear_bit(...) no?

Am I correct in understanding that there is a semantic conflict between 
bitmap_release_region

and behavior order-0, making  bitmap_release_region unsuitable for use 
in this context? Or should

we simply pursue the most concise style?

>
>> +	kfree(item->gpid);
> Is it correct to free this _before_ invalidating the entry?

Currently, gpid only utilizes the following components, and all of them 
are handled correctly.

Therefore, the use of kfree should be appropriate.

static inline void redirect_domain_prepare_entry(struct redirect_item *item,
                                                  struct avecintc_data 
*adata)
{
....
         item->gpid->en = 1; //doorbell
         item->gpid->irqnum = adata->vec; //parent domain data
         item->gpid->dst = adata->cpu; // parent domain data
...
}


>
>> +	irde_invalid_entry_node(item);
>> +}
>> +
>> +static inline void redirect_domain_prepare_entry(struct redirect_item *item,
>> +						 struct avecintc_data *adata)
>> +{
>> +	struct redirect_entry *entry = item_get_entry(item);
>> +
>> +	item->gpid->en = 1;
>> +	item->gpid->irqnum = adata->vec;
>> +	item->gpid->dst = adata->cpu;
>> +
>> +	entry->lo.valid = 1;
>> +	entry->lo.gpid = ((u64)item->gpid & GPID_ADDR_MASK) >> GPID_ADDR_SHIFT;
>> +	entry->lo.vector = 0xff;
>> +}
>> +
>> +static int redirect_set_affinity(struct irq_data *data, const struct cpumask *dest, bool force)
>> +{
>> +	struct redirect_item *item = data->chip_data;
>> +	int ret;
>> +
>> +	ret = irq_chip_set_affinity_parent(data, dest, force);
>> +	if (ret == IRQ_SET_MASK_OK_DONE) {
>> +		return ret;
>> +	} else if (ret) {
>> +		pr_err("IRDE:set_affinity error %d\n", ret);
>> +		return ret;
>> +	} else {
>> +		struct avecintc_data *adata = irq_data_get_avec_data(data);
>> +
>> +		redirect_domain_prepare_entry(item, adata);
>> +		irde_invalid_entry_node(item);
> Can you name that function irde_invalidate_entry() please? That's
> actually telling what it does. irde_invalid_entry_node() is confusing at
> best.
OK, I got it
>> +		avecintc_sync(adata);
>> +	}
>> +
>> +	return IRQ_SET_MASK_OK;
>> +}
>> +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;
>> +
>> +	node = dev_to_node(info->desc->dev);
>> +
>> +	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, arg);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	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];
>> +
>> +		ret = redirect_table_alloc(item);
>> +		if (ret) {
>> +			pr_err("Alloc redirect table entry failed\n");
>> +			goto out_free_resources;
>> +		}
>> +		/*
>> +		 * Only bits 47:6 of the GPID are passed to the controller,
>> +		 * 64-byte alignment must be guaranteed.
>> +		 */
>> +		BUILD_BUG_ON(sizeof(*item->gpid) != 64);
> s/BUILD_BUG_ON()/static_assert()/
>
> Also sizeof() does help to guarantee alignment. It only tells you the size.
Ok , I got it
>
>> +		item->gpid = kzalloc_node(sizeof(*item->gpid), GFP_KERNEL, node);
>> +		if (!item->gpid) {
>> +			pr_err("Alloc redirect GPID failed\n");
>> +			goto out_free_resources;
>> +		}
>> +
>> +		irq_data->chip_data = item;
>> +		irq_data->chip = &loongarch_redirect_chip;
>> +
>> +		redirect_domain_prepare_entry(item, irq_data_get_avec_data(irq_data));
>> +	}
>> +	return 0;
>> +
>> +out_free_resources:
>> +	redirect_free_resources(domain, virq, nr_irqs);
>> +	irq_domain_free_irqs_common(domain, virq, nr_irqs);
>> +
>> +	return -ENOMEM;
>> +}
>> +static int redirect_table_init(struct irde_desc *irde)
>> +{
>> +	struct redirect_table *ird_table = &irde->ird_table;
>> +	struct folio *folio;
>> +	unsigned long *bitmap;
>> +	int node = irde->node;
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations
Ok , I got it
>
>> +
>> +	folio = __folio_alloc_node(GFP_KERNEL | __GFP_ZERO, IRD_TABLE_PAGE_ORDER, node);
>> +	if (!folio) {
>> +		pr_err("Node [%d] redirect table alloc pages failed!\n", node);
>> +		goto fail;
> No point in making this a goto. Just return -ENOMEM.
Ok , I got it
>> +	ret = redirect_queue_init(irde);
>> +	if (ret)
>> +		goto fail_clear;
> Just get rid of the goto and handle the failure here.
Ok, I got it
>
>> +	redirect_irde_cfg(irde);
>> +
>> +	return 0;
>> +
>> +fail_clear:
>> +	__redirect_irde_fini(irde);
>> +fail:
>> +	return ret;
>> +}
>> +#ifdef CONFIG_ACPI
>> static int __init pch_msi_parse_madt(union acpi_subtable_headers *header, const unsigned long end)
> So if ACPI is disabled, the code in this file is dead code. Why do you
> build it at all if ACPI is off?
OK, I will consider adding the correct dependency relationships. Thanks
>
>> +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;
>> +	}
>> +
>> +
>> +	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);
>> +
>> +	pr_info("loongarch irq redirect modules init succeeded\n");
> If acpi_cascade_irqdomain_init() fails, then this prints first an error
> and then claims that it succeeded and returns success. That doesn't make
> any sense.

Sorry,it is an obvious mistake.

Thank you for pointing it out—I will review and ensure the 
implementation is accurate.

> Thanks,
>
>          tglx

Thank you

Tianyang

Powered by blists - more mailing lists