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

login
register
mail settings
Submitter Grant Likely
Date June 8, 2010, 2:26 p.m.
Message ID <20100608142643.26088.61366.stgit@angua>
Download mbox | patch
Permalink /patch/54990/
State Not Applicable
Headers show

Comments

Grant Likely - June 8, 2010, 2:26 p.m.
When allocating a platform_device to represent an OF node, also allocate
space for the resource table and populate it with IRQ and reg property
information.  This change is in preparation for merging the
of_platform_bus_type with the platform_bus_type so that existing
platform_driver code can retrieve base addresses and IRQs data.

Background: a previous commit removed struct of_device and made it a
#define alias for platform_device.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
CC: Michal Simek <monstr@monstr.eu>
CC: Grant Likely <grant.likely@secretlab.ca>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Stephen Rothwell <sfr@canb.auug.org.au>
CC: microblaze-uclinux@itee.uq.edu.au
CC: linuxppc-dev@ozlabs.org
CC: devicetree-discuss@lists.ozlabs.org
---
 drivers/of/platform.c |   31 ++++++++++++++++++++++++++++---
 1 files changed, 28 insertions(+), 3 deletions(-)
Anton Vorontsov - June 8, 2010, 3:57 p.m.
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.

p.s.

[Two Minutes Hate for Grant.]

Just wonder what happened to of_gpio stuff? You blocked it
in 2.6.34 for no reason saying "I'll pick it into my OF
tree before the 2.6.35 merge window" and it's 2.6.36 merge
window quite soon.

It's still in origin/test-devicetree, which is obviously
not -next. What is your plan now?

Thanks,

> +		dev->num_resources = num_reg + num_irq;
> +		res = dev->resource;
> +		for (i = 0; i < num_reg; i++, res++) {
> +			rc = of_address_to_resource(np, i, res);
> +			WARN_ON(rc);
> +		}
> +		for (i = 0; i < num_irq; i++, res++) {
> +			rc = of_irq_to_resource(np, i, res);
> +			WARN_ON(rc == NO_IRQ);
> +		}
> +	}
> +
>  	if (bus_id)
>  		dev_set_name(&dev->dev, "%s", bus_id);
>  	else
Grant Likely - June 8, 2010, 4:02 p.m.
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.  This is a common pattern.

> p.s.
>
> Just wonder what happened to of_gpio stuff? You blocked it
> in 2.6.34 for no reason saying "I'll pick it into my OF
> tree before the 2.6.35 merge window" and it's 2.6.36 merge
> window quite soon.

It's in my test-devicetree branch.  I'll be posting for 2nd review in
the next few days and then adding to my devicetree-next branch
probably before the end of the week.

g.
Anton Vorontsov - June 8, 2010, 4:46 p.m.
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.
Grant Likely - June 8, 2010, 6:41 p.m.
On Tue, Jun 8, 2010 at 10:46 AM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> 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.

It's not obscure.  It's smaller and simpler code with fewer error paths.

>> 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.

Fragile?  How?  &var[1] *always* gives you a pointer to the first
address after a structure.  If the structure changes, then so does the
offset.  Heck, if the type of 'var' changes, then the offset changes
in kind too.  If anything, I should have also used sizeof(*res) in the
kzalloc call so that the allocated size is protected against a type
change to 'res' too.

If you prefer, I can move the dev->resource assignment to immediately
after the kzalloc to keep everything contained within 4 lines of each
other.

> 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.

tx & rx buffers are different because they must be DMAable.  That
imposes alignment requirements with kzalloc() guarantees.

> 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.

git grep '\*).*&[a-z1-9_]*\[1\]'

g.
Anton Vorontsov - June 8, 2010, 7:48 p.m.
On Tue, Jun 08, 2010 at 12:41:47PM -0600, Grant Likely wrote:
[...]
> >> 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.
> 
> Fragile?  How?  &var[1] *always* gives you a pointer to the first
> address after a structure.  If the structure changes, then so does the
> offset.  Heck, if the type of 'var' changes, then the offset changes
> in kind too.  If anything, I should have also used sizeof(*res) in the
> kzalloc call so that the allocated size is protected against a type
> change to 'res' too.

You just introduced an unnamed structure of device + resources,
it isn't declared anywhere but in the code itself (either via
&foo[1] or buf + sizeof(*foo)).

You're not the only one who hacks (or at least have to
understand) the OF stuff, so let's try keep this stuff
readable?

I told you several ways of how to improve the code (based on
the ideas from drivers/base/, so the ideas aren't even mine,
fwiw).

> If you prefer, I can move the dev->resource assignment to immediately
> after the kzalloc to keep everything contained within 4 lines of each
> other.

Sure, that would be better, but I already said what would I
really prefer, i.e.

- A dedicated allocation;
- Or, at least, the same thing as drivers/base/platform.c does:
  platform_object { platform_device; name[1]; };

> > 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.
> 
> git grep '\*).*&[a-z1-9_]*\[1\]'

Ugh? This produces a lot of false positives. But OK, let's run it.

~/linux-2.6$ git grep '\*).*&[a-z1-9_]*\[1\]' | wc -l
164

^^^ Now let's presume that half of it (and not just a few hits)
    is the ugly hacks that we're talking about. Then compare:

~/linux-2.6$ git grep 'struct.*_priv' | wc -l
22448

^^^ this is a common pattern.
Benjamin Herrenschmidt - June 10, 2010, 6:17 a.m.
> You just introduced an unnamed structure of device + resources,
> it isn't declared anywhere but in the code itself (either via
> &foo[1] or buf + sizeof(*foo)).
> 
> You're not the only one who hacks (or at least have to
> understand) the OF stuff, so let's try keep this stuff
> readable?
> 
> I told you several ways of how to improve the code (based on
> the ideas from drivers/base/, so the ideas aren't even mine,
> fwiw).

I tend to agree with Anton here.

BTW. Why not make of_device a wrapper (or even alias of)
platform_device ? :-) That way you get the resource array etc.. for free
and it will make the whole of_device vs. platform_device issue moot.

Cheers,
Ben.
Grant Likely - June 10, 2010, 2:18 p.m.
On Thu, Jun 10, 2010 at 12:17 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
>> You just introduced an unnamed structure of device + resources,
>> it isn't declared anywhere but in the code itself (either via
>> &foo[1] or buf + sizeof(*foo)).
>>
>> You're not the only one who hacks (or at least have to
>> understand) the OF stuff, so let's try keep this stuff
>> readable?
>>
>> I told you several ways of how to improve the code (based on
>> the ideas from drivers/base/, so the ideas aren't even mine,
>> fwiw).
>
> I tend to agree with Anton here.

The reason I'm confident doing it that way is that it is *not* a
structure.  There is no structure relationship between the resource
table and the platform_device other than they are allocated with the
same kzalloc() call.  All the code that cares about that is contained
within 4 lines of code.  I'm resistant to using a structure because it
is adds an additional 5-6 lines of code to add a structure that won't
be used anywhere else, and is only 4 lines to begin with.

> BTW. Why not make of_device a wrapper (or even alias of)
> platform_device ? :-) That way you get the resource array etc.. for free
> and it will make the whole of_device vs. platform_device issue moot.

of_device is an alias of platform_device now.  The resource array in
platform devices is not statically defined.  It is allocated
separately.  I can't currently use the platform_device_alloc code
which does separate deallocation because the OF code needs its own
release hook to put the node.  OTOH, I can probably change the guts of
of_release_dev() to be called by platform_device_release().

okay, I'll try changing this an see how it looks.

g.
M. Warner Losh - June 10, 2010, 3:13 p.m.
In message: <AANLkTilpUi1cazljWSFbzliY78RKyHUlvBshUD3NPHPv@mail.gmail.com>
            Grant Likely <grant.likely@secretlab.ca> writes:
: On Thu, Jun 10, 2010 at 12:17 AM, Benjamin Herrenschmidt
: <benh@kernel.crashing.org> wrote:
: >
: >> You just introduced an unnamed structure of device + resources,
: >> it isn't declared anywhere but in the code itself (either via
: >> &foo[1] or buf + sizeof(*foo)).
: >>
: >> You're not the only one who hacks (or at least have to
: >> understand) the OF stuff, so let's try keep this stuff
: >> readable?
: >>
: >> I told you several ways of how to improve the code (based on
: >> the ideas from drivers/base/, so the ideas aren't even mine,
: >> fwiw).
: >
: > I tend to agree with Anton here.
: 
: The reason I'm confident doing it that way is that it is *not* a
: structure.  There is no structure relationship between the resource
: table and the platform_device other than they are allocated with the
: same kzalloc() call.  All the code that cares about that is contained
: within 4 lines of code.  I'm resistant to using a structure because it
: is adds an additional 5-6 lines of code to add a structure that won't
: be used anywhere else, and is only 4 lines to begin with.

I tend to agree with Grant here.  The idiom he's using is very wide
spread in the industry and works extremely well.  It keeps the
ugliness confined to a couple of lines and is less ugly than the
alternatives for this design pattern.  It is a little surprising when
you see the code the first time, granted, but I think its expressive
power trumps that small surprise.

Warner
Anton Vorontsov - June 10, 2010, 3:47 p.m.
On Thu, Jun 10, 2010 at 09:13:57AM -0600, M. Warner Losh wrote:
[...]
> : >> I told you several ways of how to improve the code (based on
> : >> the ideas from drivers/base/, so the ideas aren't even mine,
> : >> fwiw).
> : >
> : > I tend to agree with Anton here.
> : 
> : The reason I'm confident doing it that way is that it is *not* a
> : structure.  There is no structure relationship between the resource
> : table and the platform_device other than they are allocated with the
> : same kzalloc() call.  All the code that cares about that is contained
> : within 4 lines of code.  I'm resistant to using a structure because it
> : is adds an additional 5-6 lines of code to add a structure that won't
> : be used anywhere else, and is only 4 lines to begin with.
> 
> I tend to agree with Grant here.  The idiom he's using is very wide
> spread in the industry and works extremely well.  It keeps the
> ugliness confined to a couple of lines and is less ugly than the
> alternatives for this design pattern.  It is a little surprising when
> you see the code the first time, granted, but I think its expressive
> power trumps that small surprise.

Oh, come on. Both constructions are binary equivalent.

So how can people seriously be with *that* code:

	dev->resource = (void *)&dev[1];

which, semantically, is a nonsense and asks for a fix.

While
	dev_obj->dev.resource = dev_obj->resource;

simply makes sense.
M. Warner Losh - June 10, 2010, 4:01 p.m.
In message: <20100610154741.GA7484@oksana.dev.rtsoft.ru>
            Anton Vorontsov <cbouatmailru@gmail.com> writes:
: On Thu, Jun 10, 2010 at 09:13:57AM -0600, M. Warner Losh wrote:
: [...]
: > : >> I told you several ways of how to improve the code (based on
: > : >> the ideas from drivers/base/, so the ideas aren't even mine,
: > : >> fwiw).
: > : >
: > : > I tend to agree with Anton here.
: > : 
: > : The reason I'm confident doing it that way is that it is *not* a
: > : structure.  There is no structure relationship between the resource
: > : table and the platform_device other than they are allocated with the
: > : same kzalloc() call.  All the code that cares about that is contained
: > : within 4 lines of code.  I'm resistant to using a structure because it
: > : is adds an additional 5-6 lines of code to add a structure that won't
: > : be used anywhere else, and is only 4 lines to begin with.
: > 
: > I tend to agree with Grant here.  The idiom he's using is very wide
: > spread in the industry and works extremely well.  It keeps the
: > ugliness confined to a couple of lines and is less ugly than the
: > alternatives for this design pattern.  It is a little surprising when
: > you see the code the first time, granted, but I think its expressive
: > power trumps that small surprise.
: 
: Oh, come on. Both constructions are binary equivalent.
: 
: So how can people seriously be with *that* code:
: 
: 	dev->resource = (void *)&dev[1];
: 
: which, semantically, is a nonsense and asks for a fix.

It isn't nonsense.  That's just your opinion of it, nothing more.

: While
: 	dev_obj->dev.resource = dev_obj->resource;
: 
: simply makes sense.

But this requires extra, bogus fields in the structure and creates a
bogus sizeof issue.

There are problems both ways.  Yelling about it isn't going to make
you any more right, or convince me that I'm wrong.  It is an argument
that is at least two decades old...

Warner
Grant Likely - June 10, 2010, 4:30 p.m.
On Thu, Jun 10, 2010 at 9:47 AM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> On Thu, Jun 10, 2010 at 09:13:57AM -0600, M. Warner Losh wrote:
> [...]
>> : >> I told you several ways of how to improve the code (based on
>> : >> the ideas from drivers/base/, so the ideas aren't even mine,
>> : >> fwiw).
>> : >
>> : > I tend to agree with Anton here.
>> :
>> : The reason I'm confident doing it that way is that it is *not* a
>> : structure.  There is no structure relationship between the resource
>> : table and the platform_device other than they are allocated with the
>> : same kzalloc() call.  All the code that cares about that is contained
>> : within 4 lines of code.  I'm resistant to using a structure because it
>> : is adds an additional 5-6 lines of code to add a structure that won't
>> : be used anywhere else, and is only 4 lines to begin with.
>>
>> I tend to agree with Grant here.  The idiom he's using is very wide
>> spread in the industry and works extremely well.  It keeps the
>> ugliness confined to a couple of lines and is less ugly than the
>> alternatives for this design pattern.  It is a little surprising when
>> you see the code the first time, granted, but I think its expressive
>> power trumps that small surprise.
>
> Oh, come on. Both constructions are binary equivalent.
>
> So how can people seriously be with *that* code:
>
>        dev->resource = (void *)&dev[1];
>
> which, semantically, is a nonsense and asks for a fix.
>
> While
>        dev_obj->dev.resource = dev_obj->resource;
>
> simply makes sense.

Well, my choices are (without whitespace so as not to bias line count):

A)
struct of_device *alloc_function(int num_res)
{
	struct device *ofdev;
	struct resource *res;
	ofdev = kzalloc(sizeof(*ofdev) + (sizeof(*res) * num_res), GFP_KERNEL);
	if (!ofdev)
		return NULL;
	res = (struct resource *)&ofdev[1];
	...
	return ofdev;
}
10 lines of code

B)
struct of_device_object {
	struct of_device ofdev;
	struct resource resource[0];
};
struct of_device *alloc_function(int num_res)
{
	struct of_device_object *ofobj;
	struct of_device *ofdev;
	struct resource *res;
	ofobj = kzalloc(sizeof(*ofobj) + (sizeof(*res) * num_res), GFP_KERNEL);
	if (!ofobj)
		return NULL;
	res = ofobj->resource;
	...
	return &ofobj->ofdev;
}
15 lines of code

C)
struct of_device *alloc_function(int num_res)
{
	struct device *ofdev;
	struct resource *res;
	ofdev = kzalloc(sizeof(*ofdev), GFP_KERNEL)
	if (!ofdev)
		return NULL;
	res = kzalloc((sizeof(*res) * num_res), GFP_KERNEL);
	if (!res) {
		kfree(ofdev);  /* or goto an error unwind label */
		return NULL;
	}
	res = (struct resource *)&ofdev[1];
	...
	return ofdev;
}
15 lines of code, plus an extra few lines at kfree(ofdev) time.

When I look at the three, option A is more concise and clearer in it's
intent to me.

That being said, I'm looking at refactoring to use
platform_device_alloc() instead, which is effectively option C. (which
I'd normally avoid, but it removes otherwise duplicate code from
drivers/of).

g.
Anton Vorontsov - June 10, 2010, 4:52 p.m.
On Thu, Jun 10, 2010 at 10:01:40AM -0600, M. Warner Losh wrote:
[...]
> But this requires extra, bogus fields in the structure

False. The [0] field isn't bogus, it has a defined meaning.
It says: here is some amount of resouces may be allocated.

> and creates a bogus sizeof issue.

Creates? False. The same sizeof problem exists in Grant's
approach. sizeof(*dev) != what_we_have_allocated. Which is
isn't great, I agree. And that's exactly why I proposed a
dedicated allocation in the first place.
Mitch Bradley - June 10, 2010, 5:09 p.m.
Wow, there is some serious bikeshedding going on with this argument 
about structures and arrays .
M. Warner Losh - June 10, 2010, 5:09 p.m.
In message: <20100610165243.GA18043@oksana.dev.rtsoft.ru>
            Anton Vorontsov <cbouatmailru@gmail.com> writes:
: On Thu, Jun 10, 2010 at 10:01:40AM -0600, M. Warner Losh wrote:
: [...]
: > But this requires extra, bogus fields in the structure
: 
: False. The [0] field isn't bogus, it has a defined meaning.
: It says: here is some amount of resouces may be allocated.

Not false: It is extra fields and extra lines of code.  The fact that
it is well defined doesn't make it any less bogus.  But again, that's
just opinion, much like your objection to the equally well defined
structure it replaces is bogus.

: > and creates a bogus sizeof issue.
: 
: Creates? False. The same sizeof problem exists in Grant's
: approach. sizeof(*dev) != what_we_have_allocated. Which is
: isn't great, I agree. And that's exactly why I proposed a
: dedicated allocation in the first place.

Your solution doesn't solve the sizeof issue.  Don't pretend it does.
Both ways allocate a random amount of memory larger than sizeof the
structure.

Warner
Anton Vorontsov - June 10, 2010, 5:10 p.m.
On Thu, Jun 10, 2010 at 10:30:26AM -0600, Grant Likely wrote:
[...]
> C)
> struct of_device *alloc_function(int num_res)
> {
> 	struct device *ofdev;
> 	struct resource *res;
> 	ofdev = kzalloc(sizeof(*ofdev), GFP_KERNEL)
> 	if (!ofdev)
> 		return NULL;
> 	res = kzalloc((sizeof(*res) * num_res), GFP_KERNEL);
> 	if (!res) {
> 		kfree(ofdev);  /* or goto an error unwind label */
> 		return NULL;
> 	}
> 	res = (struct resource *)&ofdev[1];

You mean ofdev->resource = res; ?

> That being said, I'm looking at refactoring to use
> platform_device_alloc() instead, which is effectively option C. (which
> I'd normally avoid, but it removes otherwise duplicate code from
> drivers/of).

Sounds great!

Thanks,
Grant Likely - June 10, 2010, 5:20 p.m.
On Thu, Jun 10, 2010 at 11:09 AM, Mitch Bradley <wmb@firmworks.com> wrote:
> Wow, there is some serious bikeshedding going on with this argument about
> structures and arrays .

I know! Isn't it fun?

g.
Grant Likely - June 10, 2010, 5:21 p.m.
On Thu, Jun 10, 2010 at 11:10 AM, Anton Vorontsov
<cbouatmailru@gmail.com> wrote:
> On Thu, Jun 10, 2010 at 10:30:26AM -0600, Grant Likely wrote:
> [...]
>>       res = kzalloc((sizeof(*res) * num_res), GFP_KERNEL);
>>       if (!res) {
>>               kfree(ofdev);  /* or goto an error unwind label */
>>               return NULL;
>>       }
>>       res = (struct resource *)&ofdev[1];
>
> You mean ofdev->resource = res; ?

Yeah, cut-and-paste error.

g.
Benjamin Herrenschmidt - June 11, 2010, 1:14 a.m.
On Thu, 2010-06-10 at 10:01 -0600, M. Warner Losh wrote:
> : Oh, come on. Both constructions are binary equivalent.
> : 
> : So how can people seriously be with *that* code:
> : 
> :       dev->resource = (void *)&dev[1];
> : 
> : which, semantically, is a nonsense and asks for a fix.
> 
> It isn't nonsense.  That's just your opinion of it, nothing more.

No, it's dangerously fragile abuse of the C language which loses type
safety etc... It might be correct, but if somebody comes back in 2 year
to change something in that code, the chances of breaking it are higher
than having the type safe:

> :       dev_obj->dev.resource = dev_obj->resource;
> : 

Variant.

It's also less ugly.

> : simply makes sense.
> 
> But this requires extra, bogus fields in the structure and creates a
> bogus sizeof issue.
> 
> There are problems both ways.  Yelling about it isn't going to make
> you any more right, or convince me that I'm wrong.  It is an argument
> that is at least two decades old... 

Cheers,
Ben.

Patch

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 4bb898d..10ad0b6 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -441,16 +441,41 @@  struct of_device *of_device_alloc(struct device_node *np,
 				  struct device *parent)
 {
 	struct of_device *dev;
-
-	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	int rc, i, num_reg = 0, num_irq = 0;
+	struct resource *res, temp_res;
+
+	/* First count how many resources are needed */
+	while (of_address_to_resource(np, num_reg, &temp_res) == 0)
+		num_reg++;
+	while (of_irq_to_resource(np, num_irq, &temp_res) != NO_IRQ)
+		num_irq++;
+
+	/* Allocate the device including space for the resource table, and
+	 * initialize it. */
+	i = num_reg + num_irq;
+	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];
+		dev->num_resources = num_reg + num_irq;
+		res = dev->resource;
+		for (i = 0; i < num_reg; i++, res++) {
+			rc = of_address_to_resource(np, i, res);
+			WARN_ON(rc);
+		}
+		for (i = 0; i < num_irq; i++, res++) {
+			rc = of_irq_to_resource(np, i, res);
+			WARN_ON(rc == NO_IRQ);
+		}
+	}
+
 	if (bus_id)
 		dev_set_name(&dev->dev, "%s", bus_id);
 	else