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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 19 Jan 2017 11:14:34 -0500
From:   Agustin Vega-Frias <agustinv@...eaurora.org>
To:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Marc Zyngier <marc.zyngier@....com>,
        Timur Tabi <timur@...eaurora.org>,
        Christopher Covington <cov@...eaurora.org>,
        Andy Gross <agross@...eaurora.org>, harba@...eaurora.org,
        Jon Masters <jcm@...hat.com>, msalter@...hat.com,
        mlangsdo@...hat.com, Al Stone <ahs3@...hat.com>, astone@...hat.com,
        Graeme Gregory <graeme.gregory@...aro.org>,
        guohanjun@...wei.com, charles.garcia-tobin@....com
Subject: Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain
 mapping

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.