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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170119123608.GA3431@red-moon>
Date:   Thu, 19 Jan 2017 12:36:08 +0000
From:   Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Agustin Vega-Frias <agustinv@...eaurora.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-acpi@...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 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).

> > +
> > +       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.

Thanks,
Lorenzo

> 
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_irq_get);
> > +
> > +/**
> >   * acpi_set_irq_model - Setup the GSI irqdomain information
> >   * @model: the value assigned to acpi_irq_model
> >   * @fwnode: the irq_domain identifier for mapping and looking up
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index c4af003..61423d2 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -102,6 +102,14 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
> >         }
> >
> >         r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> > +       if (r && r->flags & IORESOURCE_DISABLED && ACPI_COMPANION(&dev->dev)) {
> 
> has_acpi_companion() ?
> 
> > +               int ret;
> > +
> > +               ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> > +               if (ret)
> > +                       return ret;
> > +       }
> 
> > @@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void)
> >                 memset(&pd->dev.devres_head, 0, sizeof(pd->dev.devres_head));
> >         }
> >  }
> > -
> 
> It doesn't belong here.
> 
> -- 
> With Best Regards,
> Andy Shevchenko