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
