[U-Boot] [RFC PATCH 06/12] devres: introduce Devres (Managed Device Resource) framework

[Masahiro Yamada]

Hi Simon,


2015-07-09 9:22 GMT+09:00 Simon Glass <sjg at chromium.org>:
> Hi Masahiro,
>
> On 7 July 2015 at 22:29, Masahiro Yamada <yamada.masahiro at socionext.com> wrote:
>> In U-Boot's driver model, memory is basically allocated and freed
>> in the core framework.  So, low level drivers generally only have
>> to specify the size of needed memory with .priv_auto_alloc_size,
>> .platdata_auto_alloc_size, etc.  Nevertheless, some drivers still
>> need to allocate memory on their own in case they cannot statically
>> know how much memory is needed.  Moreover, I am afraid the failure
>> paths of driver model core parts are getting messier as more and
>> more memory size members are supported, .per_child_auto_alloc_size,
>> .per_child_platdata_auto_alloc_size...  So, I believe it is
>> reasonable enough to port Devres into U-boot.
>>
>> As you know, Devres, which originates in Linux, manages device
>> resources for each device and automatically releases them on driver
>> detach.  With devres, device resources are guaranteed to be freed
>> whether initialization fails half-way or the device gets detached.
>>
>> The basic idea is totally the same to that of Linux, but I tweaked
>> it a bit so that it fits in U-Boot's driver model.
>>
>> In U-Boot, drivers are activated in two steps: binding and probing.
>> Binding puts a driver and a device together.  It is just data
>> manipulation on the system memory, so nothing has happened on the
>> hardware device at this moment.  When the device is really used, it
>> is probed.  Probing initializes the real hardware device to make it
>> really ready for use.
>>
>> So, the resources acquired during the probing process must be freed
>> when the device is removed.  Likewise, what has been allocated in
>> binding should be released when the device is unbound.  The struct
>> devres has a member "probe" to remember when the resource was
>> allocated.
>>
>> CONFIG_DEBUG_DEVRES is also supported for easier debugging.
>> If enabled, debug messages are printed each time a resource is
>> allocated/freed.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro at socionext.com>
>
> A few points to make:
>
> 1. I don't think we will be adding a lot more types of memory attached
> to devices. We have:
>
> - device private data
> - uclass' private data for the device
> - parent's private data for the device
>
> That is all set up in device_probe() and friends. Then we have
> platform data for the above which is set up in device_bind(). Within
> the driver model architecture it's hard to see another type of data
> coming along although of course I would not rule it out.
>
> 2. The auto-allocation feature of driver model has actually been very
> successful at removing the need for drivers to do their own memory
> allocation.
>
>  git grep -l U_BOOT_DRIVER |wc
> 105
>
> grep '[mc]alloc(' `git grep -l U_BOOT_DRIVER ` |wc
> 20
>
> and a quick check suggests that half of those 10 are bogus (could be
> redone to avoid a special malloc()).
>
> So I don't think the devm functions will be used much.


Right.


> 3. How do we handle things like gpio_exynos_bind() which allocs some
> data and passes it to a device it creates, as platform data? At
> present we don't free it.

So, currently this driver is leaking the memory, isn't it?

If we use devm_kzalloc() here, the platdata for GPIOs
will be released when the parent pinctrl is unbound.


> 4. There is a data size overhead to this which is not insignificant.
> As I read it we add 16 bytes to each memory allocation, which for most
> devices will amount to 32 or 48 bytes. Currently struct udevice is 84
> bytes so increasing the overhead by 50% for no real improvement in
> functionality. This does matter in SPL in some cases.
>
> With all that said I think overall this is a good and useful change. I
> can see it saving hassle later.
>
> So, can we reduce the overhead / turn it off for SPL? Perhaps it could
> dissolve to nothing if CONFIG_DM_DEVICE_REMOVE is not defined?

I think I can do this.

devres.c can be built (and makes sense) only when CONFIG_DM_DEVICE_REMOVE is
enabled.


> As it happens I started yesterday on a change to check driver model
> pointers. I've been talking about it for a while but there are enough
> drivers out there that I think it is worth doing now. I hope to have
> something next week. However it doesn't look like it will interfere
> with your change much.
>
> BTW can we please have exported functions documented in the header file?

IIRC, when we discussed this before, we could not reach agreement
which should be documented, headers or sources.

I know you prefer comments in headers, while I prefer in sources (like Linux).

I can move them to dm/device.h if you think it is important to be
consistent in the DM core portion.



-- 
Best Regards
Masahiro Yamada