Re: [PATCH v5 13/17] irqchip/riscv-intc: Add ACPI support for AIA

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, May 01 2024 at 17:47, Sunil V L wrote:
> diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> index 9e71c4428814..af7a2f78f0ee 100644
> --- a/drivers/irqchip/irq-riscv-intc.c
> +++ b/drivers/irqchip/irq-riscv-intc.c
> @@ -249,14 +249,105 @@ IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
>  IRQCHIP_DECLARE(andes, "andestech,cpu-intc", riscv_intc_init);
>  
>  #ifdef CONFIG_ACPI
> +struct rintc_data {
> +	u32 ext_intc_id;
> +	unsigned long hart_id;
> +	u64 imsic_addr;
> +	u32 imsic_size;

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

> +};
> +
> +static u32 nr_rintc;
> +static struct rintc_data *rintc_acpi_data[NR_CPUS];
> +
> +int acpi_get_intc_index_hartid(u32 index, unsigned long *hartid)

Why int? All of these functions have strictly boolean return values:
success = true, fail = false, no?

Either bool or get rid of the pointer and let the function return
either the real hart id or an invalid one.

> +{
> +	if (index >= nr_rintc)
> +		return -1;
> +
> +	*hartid = rintc_acpi_data[index]->hart_id;
> +	return 0;

I.e.

	return index >= nr_rintc ? rintc_acpi_data[index]->hart_id : INVALID_HART_ID;

> +int acpi_get_ext_intc_parent_hartid(u8 id, u32 idx, unsigned long *hartid)
> +{
> +	int i, j = 0;
> +
> +	for (i = 0; i < nr_rintc; i++) {
> +		if (APLIC_PLIC_ID(rintc_acpi_data[i]->ext_intc_id) == id) {
> +			if (idx == j) {
> +				*hartid = rintc_acpi_data[i]->hart_id;
> +				return 0;
> +			}
> +			j++;
> +		}
> +	}
> +
> +	return -1;
> +}
> +
> +void acpi_get_plic_nr_contexts(u8 id, int *nr_contexts)
> +{
> +	int i, j = 0;
> +
> +	for (i = 0; i < nr_rintc; i++) {
> +		if (APLIC_PLIC_ID(rintc_acpi_data[i]->ext_intc_id) == id)
> +			j++;
> +	}
> +
> +	*nr_contexts = j;
> +}
> +
> +int acpi_get_plic_context(u8 id, u32 idx, int *context_id)
> +{
> +	int i, j = 0;
> +
> +	for (i = 0; i < nr_rintc; i++) {
> +		if (APLIC_PLIC_ID(rintc_acpi_data[i]->ext_intc_id) == id) {
> +			if (idx == j) {
> +				*context_id = IDC_CONTEXT_ID(rintc_acpi_data[i]->ext_intc_id);
> +				return 0;
> +			}
> +
> +			j++;
> +		}
> +	}

So that's the third incarnation of the same loop with the truly self
explaining variable and argument names.

    j is actually the index of the context which is associated to a
    given PLIC ID.

    idx is the context index to search for

Right? So why can't these things be named in a way which makes the
intent of the code clear?

Also why are all the arguments u8/u32? There is no hardware
involved. Simple 'unsigned int' is just fine and the u8/u32 is not bying
you anything here.

Aside of that these ugly macros can be completely avoided and the code
can be written without a copy & pasta orgy.

struct rintc_data {
	union {
		u32		ext_intc_id;
                struct {
                	u32	context_id	: 16,
                        			:  8,
                        	aplic_plic_id	:  8;
                }
        };
	unsigned long		hart_id;
	u64			imsic_addr;
	u32			imsic_size;
};

#define for_each_matching_plic(_plic, _plic_id)				\
	for (_plic = 0; _plic < nr_rintc; _plict++)			\
        	if (rintc_acpi_data[_plic]->aplic_plic_id != _plic_id)	\
                	continue;					\
                else

unsigned int acpi_get_plic_nr_contexts(unsigned int plic_id)
{
	unsigned int nctx = 0;

	for_each_matching_plic(plic, plic_id)
		nctx++;

	return nctx;
}

static struct rintc_data *get_plic_context(unsigned int plic_id, unsigned int ctxt_idx)
{
	unsigned int ctxt = 0;

	for_each_matching_plic(plic, plic_id) {
        	if (ctxt == ctxt_idx)
                	return rintc_acpi_data + plic;
        }
        return NULL;
}

unsigned long acpi_get_ext_intc_parent_hartid(unsigned int plic_id, unsigned int ctxt_idx)
{
        struct rintc_data *data = get_plic_context(plic_id, ctxt_idx);

        return data ? data->hart_id : INVALID_HART_ID;
}

unsigned int acpi_get_plic_context(unsigned int plic_id, unsigned int ctxt_idx)
{
        struct rintc_data *data = get_plic_context(plic_id, ctxt_idx);

        return data ? data->context_id : INVALID_CONTEXT;
}

Or something like that. Hmm?

> +int acpi_get_imsic_mmio_info(u32 index, struct resource *res)
> +{
> +	if (index >= nr_rintc)
> +		return -1;
> +
> +	res->start = rintc_acpi_data[index]->imsic_addr;
> +	res->end = res->start + rintc_acpi_data[index]->imsic_size - 1;
> +	res->flags = IORESOURCE_MEM;
> +	return 0;
> +}
> +
> +static struct fwnode_handle *ext_entc_get_gsi_domain_id(u32 gsi)
> +{
> +	return riscv_acpi_get_gsi_domain_id(gsi);
> +}

This wrapper is required because using riscv_acpi_get_gsi_domain_id()
directly is too obvious, right?

>  static int __init riscv_intc_acpi_init(union acpi_subtable_headers *header,
>  				       const unsigned long end)
>  {
> -	struct fwnode_handle *fn;
>  	struct acpi_madt_rintc *rintc;
> +	struct fwnode_handle *fn;
> +	int rc;
>  
>  	rintc = (struct acpi_madt_rintc *)header;
> +	rintc_acpi_data[nr_rintc] = kzalloc(sizeof(*rintc_acpi_data[0]), GFP_KERNEL);
> +	if (!rintc_acpi_data[nr_rintc])
> +		return -ENOMEM;
> +
> +	rintc_acpi_data[nr_rintc]->ext_intc_id = rintc->ext_intc_id;
> +	rintc_acpi_data[nr_rintc]->hart_id = rintc->hart_id;
> +	rintc_acpi_data[nr_rintc]->imsic_addr = rintc->imsic_addr;
> +	rintc_acpi_data[nr_rintc]->imsic_size = rintc->imsic_size;
> +	nr_rintc++;
>  
>  	/*
>  	 * The ACPI MADT will have one INTC for each CPU (or HART)
> @@ -273,7 +364,14 @@ static int __init riscv_intc_acpi_init(union acpi_subtable_headers *header,
>  		return -ENOMEM;
>  	}
>  
> -	return riscv_intc_init_common(fn, &riscv_intc_chip);
> +	rc = riscv_intc_init_common(fn, &riscv_intc_chip);
> +	if (rc) {
> +		irq_domain_free_fwnode(fn);
> +		return rc;
> +	}

This looks like a completely unrelated bug fix. Please don't mix functional
changes and fixes.

Thanks,

        tglx