Hi Thomas,
On Thu, May 23, 2024 at 11:47:06PM +0200, Thomas Gleixner wrote:
> 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
>
Sure, thanks!
> > +};
> > +
> > +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.
>
Sure. I just tried to keep it similar to the parent function. But let me
go with your suggestion in the next revision.
> > +{
> > + 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;
>
Sure.
> > +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?
>
Nice!. Yes, this is better. Thanks a lot for the suggestion. Let me
update in the next revision.
> > +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?
>
:-). Let me remove it.
> > 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.
>
Makes sense. Let me create separate patch.
Thanks a lot for the review!
Sunil
