Re: [PATCH 6/6] of/device: populate platform_device (of_device) resource table on allocation

[Anton Vorontsov <cbouatmailru@gmail.com>]

On Tue, Jun 08, 2010 at 10:02:49AM -0600, Grant Likely wrote:
> On Tue, Jun 8, 2010 at 9:57 AM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> > On Tue, Jun 08, 2010 at 08:26:43AM -0600, Grant Likely wrote:
> > [...]
> >> +     dev = kzalloc(sizeof(*dev) + (sizeof(struct resource) * i), GFP_KERNEL);
> >>       if (!dev)
> >>               return NULL;
> >> -
> >>       dev->dev.of_node = of_node_get(np);
> >>       dev->dev.dma_mask = &dev->archdata.dma_mask;
> >>       dev->dev.parent = parent;
> >>       dev->dev.release = of_release_dev;
> >>
> >> +     /* Populate the resource table */
> >> +     if (num_irq || num_reg) {
> >> +             dev->resource = (void*)&dev[1];
> >
> > This is ugly. Why not allocate the memory specifically for
> > dev->resource? Is this because you plan to get rid of
> > of_release_dev(), and the generic release_dev() won't
> > know if it should free the dev->resource? There must
> > be a better way to handle this.
> 
> Allocating in one big block means less potential memory fragmentation,
> and the kernel can free it all at once.

Are there any numbers of saved amount of memory so that we
could compare?

The "less memory fragmentation" is indeed potential, but
introduction of obscure code is going on now at this precise
moment.

> This is a common pattern.

This can't be true because it produces ugly casts and fragile
code all over the place -- which is exactly what everybody
tries to avoid in the kernel.

Such a pattern is common when a driver allocates e.g. tx and rx
buffers (of the same type) together, and then split the buffer
into two pointers.

But I heard of no such pattern for 'struct device + struct
resources' allocation without even some kind of _priv struct,
which is surely something new, and ugly.

If you really want to avoid (an unproven) memory fragmentation,
you could do:

struct of_device_with_resources {
	struct device dev;
	struct resource resourses[0];
};

This at least will get rid of the casts and incomprehensible
"dev->resource = (void*)&dev[1];" construction.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2