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

[Simon Glass]

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.

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.

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?

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?

Regards,
Simon

[snip]