Re: [PATCHv2] driver core/platform: don't leak memory allocated for dma_mask

[Yann Droneaud <ydroneaud@opteya.com>]

Hi Uwe,

Le mardi 14 janvier 2014 à 09:19 +0100, Uwe Kleine-König a écrit :
> On Tue, Jan 14, 2014 at 08:18:29AM +0100, Yann Droneaud wrote:
> > Since commit 01dcc60a7cb8, platform_device_register_full() is
> > available to allocate and register a platform device.
> > 
> > If a dma_mask is provided as part of platform_device_info,
> > platform_device_register_full() allocate memory for a u64
> > using kmalloc().
> > 
> > A comment in the code state that "[t]his memory isn't freed
> > when the device is put".
> >

[...]

> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 3a94b799f166..6e3e639fb886 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -157,7 +157,7 @@ EXPORT_SYMBOL_GPL(platform_add_devices);
> >  
> >  struct platform_object {
> >  	struct platform_device pdev;
> > -	char name[1];
> > +	char payload[0];
> I don't know the recent minimal versions needed to compile the kernel
> and since when gcc supports c99 flexible array members, but I would
> expect that they just work. Having said that I'd prefer using that one,
> i.e. use
> 	char payload[];
> >  };

I'm not confident with flexible array when using sizeof(), offsetof(),
etc. I will try to use the c99 feature.

> > +static struct platform_device *platform_device_dmamask_alloc(const char *name,
> > +							     int id)
> > +{
> > +	struct platform_object *pa;
> > +	const size_t padding = (((offsetof(struct platform_object, payload) +
> > +				  (__alignof__(u64) - 1)) &
> > +				 ~(__alignof__(u64) - 1)) -
> > +				offsetof(struct platform_object, payload));
> > +
> > +	pa = platform_object_alloc(padding + sizeof(u64) + strlen(name) + 1);
> > +	if (pa) {
> > +		char *payload = pa->payload + padding;
> > +		/*
> > +		 * Conceptually dma_mask in struct device should not be a pointer.
> > +		 * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
> > +		 */
> > +		pa->pdev.dev.dma_mask = (void *)payload;
> > +		payload += sizeof(u64);
> > +		strcpy(payload, name);
> > +		platform_object_init(pa, payload, id);
> > +	}
> > +
> > +	return pa ? &pa->pdev : NULL;
> > +}
> This looks all complicated. Did you think about spending the extra
> memory and add a dma_mask to platform_object? That should simplify the
> code quite a bit which probably is worth the extra memory being used.
> 

You could have did it in the first place. But you choose to allocate a
chunk of memory for the u64. I believe there's a reason ;)

I will try to get some figure on the number of platform_device
registered with a dmamask versus without a dmamask: adding the u64 to
all platform_object might cost more memory than the extra code (1 branch
and a function).

Regards.

-- 
Yann Droneaud
OPTEYA