linux-kernel - Re: [PATCH 7/9] of/platform: Resolve interrupt references at probe time

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: <20130918104335.GA8256@ulmo>
Date:	Wed, 18 Sep 2013 12:43:36 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	"Strashko, Grygorii" <grygorii.strashko@...com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Stephen Warren <swarren@...dotorg.org>,
	Wolfram Sang <wsa@...-dreams.de>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <rob.herring@...xeda.com>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH 7/9] of/platform: Resolve interrupt references at probe
 time

On Tue, Sep 17, 2013 at 01:04:06PM +0000, Strashko, Grygorii wrote:
> Hi Thierry,
> 
> On 09/16/2013 11:32 AM, Thierry Reding wrote:> Interrupt references are currently resolved very early (when a device is
> > created). This has the disadvantage that it will fail in cases where the
> > interrupt parent hasn't been probed and no IRQ domain for it has been
> > registered yet. To work around that various drivers use explicit
> > initcall ordering to force interrupt parents to be probed before devices
> > that need them are created. That's error prone and doesn't always work.
> > If a platform device uses an interrupt line connected to a different
> > platform device (such as a GPIO controller), both will be created in the
> > same batch, and the GPIO controller won't have been probed by its driver
> > when the depending platform device is created. Interrupt resolution will
> > fail in that case.
> > 
> > Another common workaround is for drivers to explicitly resolve interrupt
> > references at probe time. This is suboptimal, however, because it will
> > require every driver to duplicate the code.
> > 
> > This patch adds support for late interrupt resolution to the platform
> > driver core, by resolving the references right before a device driver's
> > .probe() function will be called. This not only delays the resolution
> > until a much later time (giving interrupt parents a better chance of
> > being probed in the meantime), but it also allows the platform driver
> > core to queue the device for deferred probing if the interrupt parent
> > hasn't registered its IRQ domain yet.
> > 
> > Signed-off-by: Thierry Reding <treding@...dia.com>
> > ---
> >   drivers/base/platform.c     |  4 ++++
> >   drivers/of/platform.c       | 43 +++++++++++++++++++++++++++++++++++++------
> >   include/linux/of_platform.h |  7 +++++++
> >   3 files changed, 48 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 4f8bef3..8dcf835 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -481,6 +481,10 @@ static int platform_drv_probe(struct device *_dev)
> 
> Should it be the part of really_probe()? Isn't it?

really_probe() takes a struct device and is in fact called by all types
of devices. This code, however, is highly platform_device specific, so I
don't think we can do it in really_probe().

Unfortunately every device type has its own way of storing interrupts.
Platform devices store them as resources, I2C clients store them as a
separate field in struct i2c_client, etc.

> > +int of_platform_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	int num_irq, ret = 0;
> > +
> > +	if (!pdev->dev.of_node)
> > +		return 0;
> > +
> > +	num_irq = of_irq_count(pdev->dev.of_node);
> > +	if (num_irq > 0) {
> > +		struct resource *res = pdev->resource;
> > +		int num_reg = pdev->num_resources;
> > +		int num = num_reg + num_irq;
> > +
> > +		res = krealloc(res, num * sizeof(*res), GFP_KERNEL);
> > +		if (!res)
> > +			return -ENOMEM;
> > +
> > +		pdev->num_resources = num;
> > +		pdev->resource = res;
> > +		res += num_reg;
> 
> What will happen if Driver probe is failed or deferred?
> Seems resource table size will grow each time the Driver probe is
> deferred or failed.

That's a very good point. I think what we can do is check whether the
total number of resources that the device has (pdev->num_resources)
corresponds to num_reg + num_irq and skip in that case. That and...

> > +		ret = of_irq_to_resource_table(np, res, num_irq);
> > +		if (ret < 0)
> > +			return ret;

... updating pdev->num_resources after this point should cover all
cases. Do you see any other potential problems?

Thierry

Content of type "application/pgp-signature" skipped