Re: [RFC PATCH v3 35/37] kvx: Add IPI driver

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, Jul 22 2024 at 11:41, ysionneau@kalrayinc.com wrote:
> +/*
> + * IPI controller can signal RM and PE0 -> 15
> + * In order to restrict that to the PE, write the corresponding mask

This comment is undecodable

> + */
> +#define KVX_IPI_CPU_MASK	(~0xFFFF)
> +
> +/* A collection of single bit ipi messages.  */
> +static DEFINE_PER_CPU_ALIGNED(unsigned long, ipi_data);
> +
> +struct kvx_ipi_ctrl {
> +	void __iomem *regs;
> +	unsigned int ipi_irq;
> +};
> +
> +static struct kvx_ipi_ctrl kvx_ipi_controller;
> +
> +void kvx_ipi_send(const struct cpumask *mask, unsigned int operation)

Why is this global? It's only used in this file, no?

> +{
> +	const unsigned long *maskb = cpumask_bits(mask);
> +	unsigned long flags;
> +	int cpu;
> +
> +	/* Set operation that must be done by receiver */
> +	for_each_cpu(cpu, mask)
> +		set_bit(operation, &per_cpu(ipi_data, cpu));
> +
> +	/* Commit the write before sending IPI */
> +	smp_wmb();
> +
> +	local_irq_save(flags);
> +
> +	WARN_ON(*maskb & KVX_IPI_CPU_MASK);

> +#define KVX_IPI_CPU_MASK	(~0xFFFF)

This means the system is limited to 16 CPUs, right?

How should a bit >= NR_CPUs be set in a valid cpu mask?  Also above you
happily iterate the full cpumask. This does not make sense.

> +	writel(*maskb, kvx_ipi_controller.regs + IPI_INTERRUPT_OFFSET);
> +
> +	local_irq_restore(flags);
> +}
> +
> +static int kvx_ipi_starting_cpu(unsigned int cpu)
> +{
> +	enable_percpu_irq(kvx_ipi_controller.ipi_irq, IRQ_TYPE_NONE);
> +
> +	return 0;
> +}
> +
> +static int kvx_ipi_dying_cpu(unsigned int cpu)
> +{
> +	disable_percpu_irq(kvx_ipi_controller.ipi_irq);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t ipi_irq_handler(int irq, void *dev_id)
> +{
> +	unsigned long *pending_ipis = &per_cpu(ipi_data, smp_processor_id());

  this_cpu_ptr() ?

> +	while (true) {
> +		unsigned long ops = xchg(pending_ipis, 0);
> +
> +		if (!ops)
> +			return IRQ_HANDLED;
> +
> +		handle_IPI(ops);
> +	}

        for (ops = xchg(pending_ipis, 0); ops; ops = xchg(pending_ipis, 0))
        	handle_IPI(ops);

Hmm?

> +	return IRQ_HANDLED;
> +}
> +
> +int __init kvx_ipi_ctrl_init(struct device_node *node,
> +			     struct device_node *parent)
> +{
> +	int ret;
> +	unsigned int ipi_irq;
> +	void __iomem *ipi_base;
> +
> +	BUG_ON(!node);
> +
> +	ipi_base = of_iomap(node, 0);

What's the point of this ipi_base indirection? Just use controller.regs
directly.

> +	BUG_ON(!ipi_base);
> +
> +	kvx_ipi_controller.regs = ipi_base;
> +
> +	/* Init mask for interrupts to PE0 -> PE15 */
> +	writel(KVX_IPI_CPU_MASK, kvx_ipi_controller.regs + IPI_MASK_OFFSET);
> +
> +	ipi_irq = irq_of_parse_and_map(node, 0);
> +	if (!ipi_irq) {
> +		pr_err("Failed to parse irq: %d\n", ipi_irq);
> +		return -EINVAL;
> +	}
> +
> +	ret = request_percpu_irq(ipi_irq, ipi_irq_handler,
> +						"kvx_ipi", &kvx_ipi_controller);
> +	if (ret) {
> +		pr_err("can't register interrupt %d (%d)\n",
> +						ipi_irq, ret);
> +		return ret;
> +	}
> +	kvx_ipi_controller.ipi_irq = ipi_irq;
> +
> +	ret = cpuhp_setup_state(CPUHP_AP_IRQ_KVX_STARTING,
> +				"kvx/ipi:online",
> +				kvx_ipi_starting_cpu,
> +				kvx_ipi_dying_cpu);
> +	if (ret < 0) {
> +		pr_err("Failed to setup hotplug state");

That leaves the half initialized IPI handler around.

> +		return ret;

Thanks,

        tglx