Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx> writes:
> On Sat, Jul 30, 2011 at 08:58:07PM -0600, Grant Likely wrote:
>> On Sat, Jul 30, 2011 at 01:03:32PM +0100, Russell King - ARM Linux wrote:
>> > On Thu, Jul 21, 2011 at 04:52:18PM -0700, Kevin Hilman wrote:
>> > > Rather than embedding a struct platform_device inside a struct
>> > > omap_device, decouple them, leaving only a pointer to the
>> > > platform_device inside the omap_device.
>> > >
>> > > This patch uses devres to allocate and attach the omap_device to the
>> > > struct device, so finding an omap_device from a struct device means
>> > > using devres_find(), and the to_omap_device() helper function was
>> > > modified accordingly.
>> > >
>> > > RFC/Hack alert:
>> > >
>> > > Currently the driver core (drivers/base/dd.c) doesn't expect any
>> > > devres resources to exist before the driver's ->probe() is called. In
>> > > this patch, I just comment out the warning, but we'll need to
>> > > understand why that limitation exists, and if it's a real limitation.
>> > > A first glance suggests that it's not really needed. If this is a
>> > > true limitation, we'll need to find some way other than devres to
>> > > attach an omap_device to a struct device.
>> > >
>> > > On OMAP, we will need an omap_device attached to a struct device
>> > > before probe because device HW may be disabled in probe and drivers
>> > > are expected to use runtime PM in ->probe() to activate the hardware
>> > > before access. Because the runtime PM API calls use omap_device (via
>> > > our PM domain layer), we need omap_device attached to a
>> > > platform_device before probe.
>> >
>> > This feels really wrong to overload devres with this. devres purpose is
>> > to provide the device's _drivers_ with a way to allocate and free resources
>> > in such a way to avoid leaks on .remove or probe failure. So I think that
>> > overloading it with something that has a different lifetime is completely
>> > wrong.
>>
>> I disagree; extending devres to also handle device lifetime scoped
>> resources makes perfect sense. It is a clean extension, and struct device
>> is really getting rather huge. I certainly prefer re-scoping devres
>> to adding more elements to struct device.
>
> The point is you're asking something which is designed to have a well
> defined lifetime of driver-bind...driver-unbind to do a lot more than
> that - extending its lifetime conditional on some flag to the entire
> device lifetime instead.
>
> Such extensions tend to be a disaster - and a recipe for confusion as
> people will no longer have a clear idea of the lifetime issues. We
> already know that people absolutely struggle to understand lifed
> objects - we see it time and time again where people directly kfree
> stuff like platform devices without thinking about whether they're
> still in use.
>
> So no, extending devres is a _big_ mistake and is far from clean.
>
> Not only that, but if most of the platform devices are omap devices,
> (as it would be on OMAP), you'll consume more memory through having to
> have the additional management structs for the omap_device stuff than
> a simple pointer.
>
> As for struct device getting rather huge, last time I looked it contained
> a load of stuff which was there whether or not a platform used it. Eg,
> you get 4 bytes wasted for an of_node whether you have DT support or not.
> If sizeof(struct device) really is a problem, then it needs the unused
> stuff ifdef'd away. So I don't buy the "its getting rather huge" argument.
>
>> > We could add a new member to struct dev_archdata or pdev_archdata to carry
>> > a pointer to this data, which I think would be a far cleaner (and saner)
>> > way to deal with this. In much the same way as we already have an of_node
>> > member in struct device.
>>
>> Since this is just for omap_device, which by definition is arm-only, I
>> don't have any strong objection to using pdev_archdata. If it was
>> cross-architecture, then I'd argue strongly for the devres approach.
>
> Indeed, which is why I think putting it in the platform device archdata
> makes total sense, more sense than buggering up the clean devres lifetime
> rules that we have today.
OK, so I'll continue this approach using pdev_archdata, which would work
fine for me. It's currently empty, so I'll just add a 'void *' if it's
OK with you Russell:
diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h
index 9f390ce..bb777cd 100644
--- a/arch/arm/include/asm/device.h
+++ b/arch/arm/include/asm/device.h
@@ -13,6 +13,7 @@ struct dev_archdata {
};
struct pdev_archdata {
+ void *p;
};
#endif
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
