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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 13 Oct 2015 15:37:30 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Qais Yousef <qais.yousef@...tec.com>
cc:	linux-kernel@...r.kernel.org, jason@...edaemon.net,
	marc.zyngier@....com, jiang.liu@...ux.intel.com,
	ralf@...ux-mips.org, linux-mips@...ux-mips.org
Subject: Re: [RFC v2 PATCH 07/14] irq: add a new generic IPI reservation code
 to irq core

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/