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

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]

Message-ID: <42e7d388-a4c8-42f9-bf2f-001871a7d948@kalrayinc.com>
Date: Fri, 23 Aug 2024 16:46:21 +0200
From: Yann Sionneau <ysionneau@...rayinc.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, linux-kernel@...r.kernel.org,
 Thomas Gleixner <tglx@...utronix.de>
Cc: Jonathan Borne <jborne@...rayinc.com>, Julian Vetter
 <jvetter@...rayinc.com>, Clement Leger <clement@...ment-leger.fr>, Guillaume
 Thouvenin <thouveng@...il.com>, Luc Michel <luc@...chel.fr>, Jules Maselbas
 <jmaselbas@...v.net>, bpf@...r.kernel.org
Subject: Re: [RFC PATCH v3 35/37] kvx: Add IPI driver

Hello Krzysztof,

On 22/07/2024 14:39, Krzysztof Kozlowski wrote:
> On 22/07/2024 11:41, ysionneau@...rayinc.com wrote:
>> From: Yann Sionneau <ysionneau@...rayinc.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