Message-ID: <20140422030528.GB26554@atomide.com>
Date:	Mon, 21 Apr 2014 20:05:28 -0700
From:	Tony Lindgren <tony@...mide.com>
To:	Rob Herring <robherring2@...il.com>
Cc:	Russell King - ARM Linux <linux@....linux.org.uk>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Jean-Jacques Hiblot <jjhiblot@...phandler.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	Rob Herring <robh+dt@...nel.org>,
	Gregory Clement <gregory.clement@...e-electrons.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v3 2/2] dt: platform driver: Fill the resources before
 probe and defer if needed

* Tony Lindgren <tony@...mide.com> [140421 13:26]:
> * Rob Herring <robherring2@...il.com> [140421 12:01]:
> > On Mon, Apr 21, 2014 at 10:54 AM, Tony Lindgren <tony@...mide.com> wrote:
> > > * Rob Herring <robherring2@...il.com> [140421 06:47]:
> > >> On Fri, Apr 18, 2014 at 6:24 PM, Tony Lindgren <tony@...mide.com> wrote:
> > >> > * Russell King - ARM Linux <linux@....linux.org.uk> [140418 16:04]:
> > >> >> On Fri, Apr 18, 2014 at 02:58:48PM -0700, Tony Lindgren wrote:
> > >> >> > Oh come on, let's stop pretending it's not broken. And it's way worse with
> > >> >> > device tree as there's nothing making sure the resources for a driver
> > >> >> > are set up before the driver probes. And we've been unable to fix just
> > >> >> > this issue alone for about six months now. It's also broken beyond that.
> > >> >> > It's called of_platform_bus yet it won't even pass the platform_data
> > >> >> > as auxdata to the devices on a sub-bus instantatiated like I2C.
> > >> >>
> > >> >> Isn't there a much simpler solution to the platform device IRQ problem?
> > >> >>
> > >> >> Rather than trying to fix it at the point where the resources are
> > >> >> created, why not just *not* have DT create the IRQ resources in the
> > >> >> first place, and instead have platform_get_irq() (which is the function
> > >> >> which should be used to get an IRQ) be the actor to do whatever is
> > >> >> necessary to return the IRQ(s) ?
> > >> >
> > >> > Yeah why not. I don't see why we would need to do all this of_* special
> > >> > trickery for much anything beyond parsing the binding.
> > >>
> > >> That can work, but it will still need something like
> > >> of_find_irq_domain() to determine whether to return -EPROBE_DEFER or
> > >> not.
> > >
> > > Right. Naturally let's do whatever it takes to first fix this issue
> > > in a minimal way first for the -rc cycle so we can do the longer term
> > > changes needed.
> > 
> > I'm not really convinced there is a simple and safe enough solution for
> > 3.15. We should also be willing to tag a solution for stable if we take
> > it for -rc (although that decision could be deferred).
> 
> Yes the fix needs to also go to stable. And this is a regression
> between legacy booting and DT based booting so we do have good
> reasons to fix this for the -rc cycle. Who knows how many people are
> hitting this and are tinkering with the driver initcall levels to
> work around it. Ideally the fix, even if intrusive, would already
> get us a little bit into the right direction.
> 
> > >> You could also go in the other direction and don't create the device
> > >> until the resources can be resolved. Unlike any of the other
> > >> solutions, that would work for amba bus as well although we may never
> > >> have a case where we need this with the amba bus. This would require
> > >> making of_platform_populate be callable multiple times, but there are
> > >> already some other reasons for wanting to do that. Specifically, I
> > >> would like the core code to call of_platform_populate with default
> > >> options and then only platforms with non-default options need a call
> > >> to of_platform_populate.
> > >
> > > I like this idea as this would also probably remove the the numerous
> > > dmesg errors we are currently getting for drivers reprobing with
> > > -EPROBE_DEFER.
> > 
> > One issue with my proposal is with supporting modules. IIUC, deferred
> > probe will continue trying forever and loading modules can cause probe
> > to succeed. If devices are not created and on the deferred probe list,
> > then they will not get probed when a module load fixes the dependency.
> > 
> > > In the long term we should have platform bus just call a set of
> > > standardized functions implemented by whatever the data source might
> > > be. That way we can limit the use of of_* functions in device drivers
> > > to just parsing of custom bindings in the drivers and use bus specific
> > > functions for everything else.
> > >
> > >> >> Yes, I know we have some drivers which use platform_get_resources() with
> > >> >> IORESOURCE_IRQ, but they should really use the right accessor.  And those
> > >> >> who just dereference the resource array directly... get what's coming
> > >> >> (though of course they have to be fixed.)
> > >> >
> > >> > $ git grep IORESOURCE_IRQ drivers/ | grep platform_get_resource | wc -l
> > >> > 179
> > >>
> > >> Certainly, this is worthwhile clean-up no matter what the solution.
> > >
> > > Yeah agreed. But let's also consider the IORESOURCE_IRQ as just another
> > > source for for the bus or driver data in addition to the DT parsed data.
> > > Both sources of data should work just fine with platform_bus even
> > > without cleaning up the drivers
> > 
> > Ah, right. Except for those drivers you need to work with deferred probe
> > would have to use platform_get_irq. That fact makes this solution quite
> > a bit easier.
> 
> Yes fixing up the known broken drivers is also doable for the -rc
> cycle.
>  
> > Something like this is what you had in mind?
> > 
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index e714709..5b47210 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/string.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/of_device.h>
> > +#include <linux/of_irq.h>
> >  #include <linux/module.h>
> >  #include <linux/init.h>
> >  #include <linux/dma-mapping.h>
> > @@ -87,7 +88,11 @@ int platform_get_irq(struct platform_device *dev,
> > unsigned int num)
> >  		return -ENXIO;
> >  	return dev->archdata.irqs[num];
> >  #else
> > -	struct resource *r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> > +	struct resource *r;
> > +	if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node)
> > +		return of_irq_get(dev->dev.of_node, num);
> > +
> > +	r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> > 
> >  	return r ? r->start : -ENXIO;
> >  #endif
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index 7d3184f..30449ad 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -400,6 +400,26 @@ int of_irq_to_resource(struct device_node *dev, int
> > index, struct resource *r)
> >  EXPORT_SYMBOL_GPL(of_irq_to_resource);
> > 
> >  /**
> > + * of_irq_get - Decode a node's IRQ and return it as a Linux irq number
> > + * @dev: pointer to device tree node
> > + * @index: zero-based index of the irq
> > + *
> > + * Returns Linux irq number on success, or -EPROBE_DEFER if the irq domain
> > + * is not yet created.
> > + *
> > + */
> > +int of_irq_get(struct device_node *dev, int index)
> > +{
> > +	int irq = irq_of_parse_and_map(dev, index);
> > +
> > +	if (!irq && of_find_irq_domain(dev, index) == NULL)
> > +		return -EPROBE_DEFER;
> > +
> > +	return irq;
> > +}
> 
> Yeah something like that. That might also work as a pretty
> minimal fix as long as we fix the known broken drivers to use
> platform_get_irq().

Actually, the above code for of_irq_get() won't help as we're still
calling irq_of_parse_and_map() before we should. So the nasty warnings
are still there if the irqdomain is not yet found.
 
> But for the long run, how about we define some kind of Linux
> generic bus ops:
> 
> struct bus_ops {
> 	struct resource *	(*get_resource)(struct device *dev, unsigned int type, unsigned int num);
> 	int			(get_irq)(struct device *dev, unsigned int num);
> 	...
> };
> 
> And then DT code would register static of_irq_get() as a get_irq()
> for the platform_bus. And naturally we'd implement these functions
> for legacy resources too. And that way platform_bus code stays clear
> of implementation details and just goes through the list of
> registered data sources. It may even make sense to do it for the
> fix to keep the of_* functions private to drivers/of/*.c and the
> bus code generic.
> 
> > +EXPORT_SYMBOL_GPL(of_irq_get);
> 
> Probably no need to export this? I don't think we want other
> code to use this directly..
> 
> Regards,
> 
> Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists