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

Yann Sionneau
2024-08-23T12:40:04Z

Hello Krzysztof,

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