Re: [RFC PATCH v3 19/37] irqchip: Add irq-kvx-apic-gic driver

[Yann Sionneau]

Hello Krzysztof,

On 22/07/2024 14:28, Krzysztof Kozlowski wrote:
> On 22/07/2024 11:41, ysionneau@xxxxxxxxxxxxx wrote:
>> From: Yann Sionneau <ysionneau@xxxxxxxxxxxxx>
>>
> ...
>
>> +
>> +static int __init kvx_init_apic_gic(struct device_node *node,
>> +				    struct device_node *parent)
>> +{
>> +	struct kvx_apic_gic *gic;
>> +	int ret;
>> +	unsigned int irq;
>> +
>> +	if (!parent) {
>> +		pr_err("kvx apic gic does not have parent\n");
> How is this possible? Aren't you controlling the code being executed?

I think this is called from generic code with values from the DT.

If this node does not have any interrupt-parent I guess parent is NULL.

DT is user controlled, not controlled by the driver code.

Also, I can see such tests in several irqchip drivers: irq-imx-gpcv2, irq-sni-exiu, irq-crossbar, irq-meson-gpio, irq-al-fic, irq-tegra, irq-mmp, etc

Isn't it ok?

>> +		return -EINVAL;
>> +	}
>> +
>> +	gic = kzalloc(sizeof(*gic), GFP_KERNEL);
>> +	if (!gic)
>> +		return -ENOMEM;
>> +
>> +	if (of_property_read_u32(node, "kalray,intc-nr-irqs",
>> +						&gic->input_nr_irqs))
> There is no such property. Also, there shouldn't be anyway...
Ok I'll remove this property from the code and just use KVX_GIC_INPUT_IT_COUNT renamed as KVX_GIC_MAX_INPUT_IT_COUNT everywhere with the maximum possible value.
>
>> +		gic->input_nr_irqs = KVX_GIC_INPUT_IT_COUNT;
>> +
>> +	if (WARN_ON(gic->input_nr_irqs > KVX_GIC_INPUT_IT_COUNT)) {
> Why? Please, drop all these WARN_ON from here and other patches. WARN_ON
> is for cases which cannot happen, as it might panic entire system.
Ok, I'll replace with pr_warn().
> Instead, handle the case properly.
>
>> +		ret = -EINVAL;
>> +		goto err_kfree;
>> +	}
>> +
>> +	gic->base = of_io_request_and_map(node, 0, node->name);
>> +	if (!gic->base) {
>> +		ret = -EINVAL;
>> +		goto err_kfree;
>> +	}
>> +
>> +	raw_spin_lock_init(&gic->lock);
>> +	apic_gic_init(gic);