Re: [PATCH 20/24] irqchip/gic-v5: Add GICv5 LPI/IPI support

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, Apr 08 2025 at 12:50, Lorenzo Pieralisi wrote:
> +/* Wait for completion of an IST change */
> +static int gicv5_irs_ist_wait_for_idle(struct gicv5_irs_chip_data *irs_data)
> +{
> +	int ret;
> +	u32 val;
> +
> +	ret = readl_relaxed_poll_timeout_atomic(
> +			irs_data->irs_base + GICV5_IRS_IST_STATUSR, val,
> +			FIELD_GET(GICV5_IRS_IST_STATUSR_IDLE, val), 1,
> +			USEC_PER_SEC);
> +
> +	if (ret == -ETIMEDOUT)
> +		pr_err_ratelimited("IST_STATUSR.IDLE timeout...\n");
> +
> +	return ret;

I'm sure I've seen that code before and without looking I'm sure the
diff between the two functions is ~2 lines.

> +
> +	mtree_lock(&lpi_mt);
> +	ret = mas_empty_area(&mas, 0, num_lpis - 1, lpis);
> +	if (ret) {
> +		pr_err("Failed to perform a dynamic alloc in the LPI MT!\n");
> +		return ret;
> +	}
> +
> +	lpi_base = mas.index;
> +
> +	/*
> +	 * We don't really care about the entry itself, only about
> +	 * allocation of a maple tree ranges describing in use LPIs.
> +	 * That's why, upon allocation, we try to merge slots adjacent
> +	 * with the empty one we are allocating to minimize the number
> +	 * of slots we take from maple tree nodes for nothing, all
> +	 * we need to keep track of is in use ranges.
> +	 */

I'm really not convinced that you need a maple tree and the code
complexity for this. What's wrong with a simple bitmap other than that
it might need 1MB memory?

> +static int gicv5_irq_lpi_domain_alloc(struct irq_domain *domain,
> +				      unsigned int virq, unsigned int nr_irqs,
> +				      void *arg)
> +{
> +	irq_hw_number_t hwirq;
> +	struct irq_data *irqd;
> +	u32 *base_lpi = arg;
> +	int i, ret;
> +
> +	hwirq = *base_lpi;
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		irqd = irq_desc_get_irq_data(irq_to_desc(virq + i));

irq_get_irq_data() and irq_domain_get_irq_data() exist for a reason.

> +
> +		irq_domain_set_info(domain, virq + i, hwirq + i,
> +				    &gicv5_lpi_irq_chip, NULL,
> +				    handle_fasteoi_irq, NULL, NULL);
> +		irqd_set_single_target(irqd);
> +static int gicv5_irq_ipi_domain_alloc(struct irq_domain *domain,
> +				      unsigned int virq, unsigned int nr_irqs,
> +				      void *arg)
> +{
> +	int ret, i;
> +	u32 lpi;
> +	struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(virq));

Again. Zero reason to fiddle with irq_desc.

Thanks,

        tglx