Hi , Thomas
在 2025/5/25 下午5:06, Thomas Gleixner 写道:
On Fri, May 23 2025 at 18:18, Tianyang Zhang wrote:
-static void avecintc_sync(struct avecintc_data *adata)
+void avecintc_sync(struct avecintc_data *adata)
{
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;
This change really wants to be seperate with a proper explanation and
not burried inside of this pile of changes.
Ok, I got it , I will add some annotation info
+static inline bool invalid_queue_is_full(int node, u32 *tail)
+{
+ u32 head;
+
+ head = read_queue_head(node);
Please move the initialization into the declaration line:
u32 head = read_queue...();
All over the place, where it's the first operation in the code. That
makes the code more dense and easier to follow.
OK I got it , thanks
+ *tail = read_queue_tail(node);
+
+ return !!(head == ((*tail + 1) % INVALID_QUEUE_SIZE));
What's the !! for? A == B is a boolean expression already.
Emmm....This is actually a rookie mistake, thanks
+}
+
+static void invalid_enqueue(struct redirect_queue *rqueue, struct irde_inv_cmd *cmd)
+{
+ struct irde_inv_cmd *inv_addr;
+ u32 tail;
+
+ guard(raw_spinlock_irqsave)(&rqueue->lock);
+
+ while (invalid_queue_is_full(rqueue->node, &tail))
+ cpu_relax();
+
+ inv_addr = (struct irde_inv_cmd *)(rqueue->base + tail * sizeof(struct irde_inv_cmd));
+ memcpy(inv_addr, cmd, sizeof(struct irde_inv_cmd));
+ tail = (tail + 1) % INVALID_QUEUE_SIZE;
+
+ /*
+ * The uncache-memory access may have an out of order problem cache-memory access,
+ * so a barrier is needed to ensure tail is valid
+ */
This comment does not make sense at all.
What's the actual uncached vs. cached access problem here? AFAICT it's
all about the ordering of the writes:
You need to ensure that the memcpy() data is visible _before_ the
tail is updated, no?
Yes, the fundamental purpose is to ensure that all data is valid when
updating registers.
I will modify the annotation information here. Thank you
+ wmb();
+
+ write_queue_tail(rqueue->node, tail);
+}
+static int redirect_table_free(struct redirect_item *item)
That return value is there to be ignored by the only caller, right?
Let's re evaluate the significance of the return value here, thanks
+{
+ struct redirect_table *ird_table;
+ struct redirect_entry *entry;
+
+ ird_table = item->table;
+
+ entry = item->entry;
+ memset(entry, 0, sizeof(struct redirect_entry));
+
+ scoped_guard(raw_spinlock_irqsave, &ird_table->lock)
+ bitmap_release_region(ird_table->bitmap, item->index, 0);
+
+ kfree(item->gpid);
+
+ irde_invlid_entry_node(item);
+
+ return 0;
+}
+
+static inline void redirect_domain_prepare_entry(struct redirect_item *item,
+ struct avecintc_data *adata)
Please align the argument in the second line properly:
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#line-breaks
Ok, I got it , thanks
+
+static inline void redirect_ack_irq(struct irq_data *d)
+{
+}
+
+static inline void redirect_unmask_irq(struct irq_data *d)
+{
+}
+
+static inline void redirect_mask_irq(struct irq_data *d)
+{
+}
These want some explanation why they are empty.
Ok, I got it , thanks
+
+static struct irq_chip loongarch_redirect_chip = {
+ .name = "REDIRECT",
+ .irq_ack = redirect_ack_irq,
+ .irq_mask = redirect_mask_irq,
+ .irq_unmask = redirect_unmask_irq,
+ .irq_set_affinity = redirect_set_affinity,
+ .irq_compose_msi_msg = redirect_compose_msi_msg,
+};
+out_free_resources:
+ redirect_free_resources(domain, virq, nr_irqs);
+ irq_domain_free_irqs_common(domain, virq, nr_irqs);
+
+ return -EINVAL;
-ENOMEM?
Ok, I got it , thanks
+}
+
+ bitmap = bitmap_zalloc(IRD_ENTRIES, GFP_KERNEL);
+ if (!bitmap) {
+ pr_err("Node [%d] redirect table bitmap alloc pages failed!\n", node);
+ return -ENOMEM;
Leaks pages.
Ok, I got it , thanks
+ }
+
+ ird_table->bitmap = bitmap;
+ ird_table->nr_ird = IRD_ENTRIES;
+ ird_table->node = node;
+
+ raw_spin_lock_init(&ird_table->lock);
+
+ if (redirect_queue_init(node))
+ return -EINVAL;
Leaks pages and bitmap.
Ok, I got it , thanks
+
+ iocsr_write64(CFG_DISABLE_IDLE, LOONGARCH_IOCSR_REDIRECT_CFG);
+ iocsr_write64(__pa(ird_table->table), LOONGARCH_IOCSR_REDIRECT_TBR);
+
+ return 0;
+}
+#if defined(CONFIG_ACPI)
#ifdef CONFIG_ACPI
Ok, I got it , thanks
+static int __init redirect_reg_base_init(void)
+{
+ acpi_status status;
+ uint64_t addr = 0;
What's this initialization for?
The initial purpose here was to confirm the validity of the data
returned by acpi_evaluate_integer,
but perhaps this is not necessary.
I will confirm again here, thanks
+int __init redirect_acpi_init(struct irq_domain *parent)
+{
+ struct fwnode_handle *fwnode;
+ struct irq_domain *domain;
+ int ret;
+
+ fwnode = irq_domain_alloc_named_fwnode("redirect");
+ if (!fwnode) {
+ pr_err("Unable to alloc redirect domain handle\n");
+ goto fail;
+ }
+
+ domain = irq_domain_create_hierarchy(parent, 0, IRD_ENTRIES, fwnode,
+ &redirect_domain_ops, irde_descs);
Please align the arguments in the second line properly.
Ok, I got it , thanks
+static int redirect_cpu_online(unsigned int cpu)
+{
+ int ret, node = cpu_to_node(cpu);
+
+ if (cpu != cpumask_first(cpumask_of_node(node)))
+ return 0;
+
+ ret = redirect_table_init(node);
+ if (ret) {
+ redirect_table_fini(node);
+ return -EINVAL;
+ }
+
+ return 0;
+}
So if you unplug all CPUs of a node and then replug the first CPU in the
node, then this invokes redirect_table_init() unconditionally, which
will unconditionally allocate pages and bitmap again ....
We need to reconsider here, thank you
Thanks,
tglx
Thanks again
Tianyang
