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

[Yann Sionneau <ysionneau@kalrayinc.com>]

Hello Krzysztof,

On 22/07/2024 14:39, Krzysztof Kozlowski wrote:
> On 22/07/2024 11:41, ysionneau@kalrayinc.com wrote:
>> From: Yann Sionneau <ysionneau@kalrayinc.com>
>>
>> [...]
>> +
>> +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);
> Fix your code instead.

I am not sure I understand your comment here, I don't have the control over what the kernel passes to my driver, do I?

On the other hand, "node" being the node that matches the compatible, maybe it can never be NULL, is that what you're saying?

After doing some archeology in our old code base it seems indeed this line is an artefact of this previous code snippet:

```

np = of_find_compatible_node(NULL, NULL, "kalray,coolidge-ipi-ctrl");
BUG_ON(!np);

```

Now that this is a real driver declared via IRQCHIP_DECLARE(), I guess that this check isn't needed anymore.

>
>> +
>> +	ipi_base = of_iomap(node, 0);
>> +	BUG_ON(!ipi_base);
> No, handle it by returning.
Ack
>
>> +
>> +	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");
>> +		return ret;
>> +	}
>> +
>> +	set_smp_cross_call(kvx_ipi_send);
>> +	pr_info("controller probed\n");
> Drop this simple probe successes. This is not the way to trace system
> bootup. Keep only reasonable amount, not every driver printing that its
> initcall started.
Ack.

Thanks for the review!

Regards,

-- 

Yann