Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

[Agustin Vega-Frias]

On 2017-01-19 07:36, Lorenzo Pieralisi wrote:
> On Wed, Jan 18, 2017 at 09:45:56PM +0200, Andy Shevchenko wrote:
>
> [...]
>
> > > +/**
> > > + * acpi_irq_get - Look for the ACPI IRQ resource with the given index and
> > > + *                use it to initialize the given Linux IRQ resource.
> > > + * @handle ACPI device handle
> > > + * @index  ACPI IRQ resource index to lookup
> > > + * @res    Linux IRQ resource to initialize
> >
> > Ah, you missed colons after field names:
> > * @field1:
> >
> > > + *
> > > + * Return:
> >
> > Next line:  0 on success
> >
> > > + *         -EINVAL if an error occurs
> > > + *         -EPROBE_DEFER if the IRQ lookup/conversion failed
> > > + */
> > > +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res)
> > > +{
> >
> > > +       int rc;
> >
> > Put this last in the definition block.
> >
> > > +       struct irq_fwspec fwspec;
> > > +       struct irq_domain *domain;
> > > +       unsigned long flags;
> > > +
> > > +       rc = acpi_irq_parse_one(handle, index, &fwspec, &flags);
> > > +       if (rc)
> > > +               return rc;
> > > +
> > > +       domain = irq_find_matching_fwnode(fwspec.fwnode, DOMAIN_BUS_ANY);
> > > +       if (!domain)
> > > +               return -EPROBE_DEFER;
> >
> > Hmm... Could it be other issues here?
>
> That's a good point. Here probing should be deferred only if we know a
> driver capable of handling fwspec.fwnode will have a chance to be
> probed/initialized eventually right (which basically means it is
> compiled in the kernel and we hope it will probed successfully) ? I am
> not sure there is an easy way to detect that in ACPI at the moment, I
> suspect it is time we added a linker section (or augment the existing
> one used for irqchip early MADT parsing - IRQCHIP_ACPI_DECLARE) for 
> this
> purpose I do not see any other option (apart from leaving devices in 
> the
> deferred probe list if a driver for the irqchip represented by
> fwspec.fwnode is not present in the kernel, which is a bit sloppy, or
> resorting to checking Kconfig entries to detect compile time enabled
> irqchip drivers).

I'll add the linker section suggested by Lorenzo for the extra check.
We can do that check in acpi_get_irq_source_fwhandle and make the lookup
fail if the device is not listed in the table.

That way we can keep this check as-is.

Thoughts?

> > > +
> > > +       rc = irq_create_fwspec_mapping(&fwspec);
> > > +       if (rc <= 0)
> > > +               return -EINVAL;
> > > +
> >
> > > +       res->start = rc;
> > > +       res->end = rc;
> > > +       res->flags = flags;
> >
> > Perhaps struct resource *r should be a parameter to 
> > acpi_irq_parse_one().
>
> Yeah but then you would end up with flags initialized in
> acpi_irq_parse_one() and the other resource params (ie start, end) 
> here,
> I do not think it is nicer.

The reason this took this form is that we are trying to get some 
symmetry
with what of_irq_get does.

Thanks,
Agustin

> Thanks,
> Lorenzo
>
> [...]
>
> > With Best Regards,
> > Andy Shevchenko

--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.