Re: [RFC v2 PATCH 07/14] irq: add a new generic IPI reservation code to irq core

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, 13 Oct 2015, Qais Yousef wrote:
> +/**
> + * irq_reserve_ipi() - setup an IPI to destination cpumask
> + * @domain: IPI domain
> + * @dest: cpumask of cpus to receive the IPI
> + *
> + * Allocate a virq that can be used to send IPI to any CPU in dest mask.
> + *
> + * On success it'll return linux irq number and 0 on failure
> + */
> +unsigned int irq_reserve_ipi(struct irq_domain *domain,
> +			     const struct ipi_mask *dest)
> +{
> +	struct irq_data *data;
> +	int virq;
> +	unsigned int nr_irqs;

Please order them so:

+	struct irq_data *data;
+	unsigned int nr_irqs;
+	int virq;

Much simpler to read.

> +	if (domain == NULL)
> +		domain = irq_default_domain; /* need a separate ipi_default_domain? */

No tail comments please.

We should neither use irq_default_domain nor have an
ipi_default_domain.

> +
> +	if (domain == NULL) {
> +		pr_warn("Must provide a valid IPI domain!\n");
> +		return 0;
> +	}
> +
> +	if (!irq_domain_is_ipi(domain)) {
> +		pr_warn("Not an IPI domain!\n");
> +		return 0;
> +	}
> +
> +	/* always allocate a virq per cpu */
> +	nr_irqs = bitmap_weight(dest->cpumask, dest->nbits);;

Double semicolon

> +
> +	virq = irq_domain_alloc_descs(-1, nr_irqs, 0, NUMA_NO_NODE);
> +	if (virq <= 0) {
> +		pr_warn("Can't reserve IPI, failed to alloc descs\n");
> +		return 0;
> +	}
> +
> +	/* we are reusing hierarchy alloc function, should we create another one? */
> +	virq = __irq_domain_alloc_irqs(domain, virq, nr_irqs, NUMA_NO_NODE,
> +					(void *) dest, true);
> +	if (virq <= 0) {
> +		pr_warn("Can't reserve IPI, failed to alloc irqs\n");
> +		goto free_descs;
> +	}
> +
> +	data = irq_get_irq_data(virq);
> +	bitmap_copy(data->ipi_mask.cpumask, dest->cpumask, dest->nbits);
> +	data->ipi_mask.nbits = dest->nbits;

This does only initialize the first virq data. What about the others?

> +	return virq;
> +
> +free_descs:
> +	irq_free_descs(virq, nr_irqs);
> +	return 0;
> +}
> +
> +/**
> + * irq_destroy_ipi() - unreserve an IPI that was previously allocated
> + * @irq: linux irq number to be destroyed
> + *
> + * Return an IPI allocated with irq_reserve_ipi() to the system.

That wants to explain that it actually destroys a number of virqs not
just the primary one.

Thanks,

	tglx