OF device code merges and improvements

These patches merge more common of_device code between microblaze and powerpc and some other miscellaneous improvement in preparation for the merge of of_platform_bus_type with platform_bus_type.

These patches are based on top of the IRQ and address code merge which I posted earlier. If you want to test them, I've pushed the commits out to the here:

git://git.secretlab.ca/git/linux-2.6 devicetree-next

Cheers,
g.

---

Grant Likely (6):
of: Use full node name in resource structures
of/device: merge of_device_uevent
of: Modify of_device_get_modalias to be passed struct device of/device: Merge of_platform_bus_probe()
of: Merge of_device_alloc
of/device: populate platform_device (of_device) resource table on allocation

 arch/microblaze/include/asm/device.h      |    3 
 arch/microblaze/include/asm/of_device.h   |   21 ---
 arch/microblaze/include/asm/of_platform.h |   33 ----
 arch/microblaze/kernel/Makefile           |    2 
 arch/microblaze/kernel/of_device.c        |  112 ---------------
 arch/microblaze/kernel/of_platform.c      |  141 ++-----------------
 arch/powerpc/include/asm/device.h         |    3 
 arch/powerpc/include/asm/of_device.h      |   13 --
 arch/powerpc/include/asm/of_platform.h    |   11 -
 arch/powerpc/kernel/of_device.c           |  110 +-------------
 arch/powerpc/kernel/of_platform.c         |  131 -----------------
 drivers/macintosh/macio_sysfs.c           |    5 -
 drivers/of/address.c                      |    2 
 drivers/of/device.c                       |   62 +++++++-
 drivers/of/irq.c                          |    1 
 drivers/of/platform.c                     |  221 +++++++++++++++++++++++++++++
 include/linux/of_device.h                 |    6 +
 include/linux/of_platform.h               |   21 +++
 18 files changed, 337 insertions(+), 561 deletions(-)
 delete mode 100644 arch/microblaze/kernel/of_device.c

Resource names appear in human readable output, so when extracting IRQ and address resources from a device tree node, use the full node name to give proper context in places like /proc/iomem.

---
 drivers/of/address.c |    2 +-
 drivers/of/irq.c     |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

index 5c220c3..fcadb72 100644
@@ -549,7 +549,7 @@ static int __of_address_to_resource(struct device_node *dev, const u32 *addrp,
 		r->end = taddr + size - 1;
 	}
 	r->flags = flags;
-	r->name = dev->name;
+	r->name = dev->full_name;
 	return 0;
 }
 

index 5c097be..8e8cdce 100644
@@ -330,6 +330,7 @@ unsigned int of_irq_to_resource(struct device_node *dev, int index,
 	if (r && irq != NO_IRQ) {
 		r->start = r->end = irq;
 		r->flags = IORESOURCE_IRQ;
+		r->name = dev->full_name;
 	}
 
 	return irq;

Merge common code between powerpc and microblaze

---
 arch/microblaze/include/asm/of_device.h |    3 --
 arch/microblaze/kernel/of_device.c      |   48 ------------------------------
 arch/powerpc/include/asm/of_device.h    |    3 --
 arch/powerpc/kernel/of_device.c         |   49 -------------------------------
 drivers/of/device.c                     |   48 ++++++++++++++++++++++++++++++
 include/linux/of_device.h               |    4 +++
 6 files changed, 52 insertions(+), 103 deletions(-)

index 0a5f3f9..58e627d 100644
@@ -22,9 +22,6 @@ extern struct of_device *of_device_alloc(struct device_node *np,
 					 const char *bus_id,
 					 struct device *parent);
 
-extern int of_device_uevent(struct device *dev,
-			    struct kobj_uevent_env *env);
-
 extern void of_device_make_bus_id(struct of_device *dev);
 
 /* This is just here during the transition */

index cb36632..5d5103c 100644
@@ -9,8 +9,5 @@ extern struct of_device *of_device_alloc(struct device_node *np,
 					 const char *bus_id,
 					 struct device *parent);
 
-extern int of_device_uevent(struct device *dev,
-			    struct kobj_uevent_env *env);
-
 #endif /* __KERNEL__ */
 #endif /* _ASM_POWERPC_OF_DEVICE_H */

index a3ae590..da83e73 100644
@@ -44,6 +44,10 @@ static inline void of_device_free(struct of_device *dev)
 
 extern ssize_t of_device_get_modalias(struct of_device *ofdev,
 					char *str, ssize_t len);
+
+extern int of_device_uevent(struct device *dev, struct kobj_uevent_env *env);
+
+
 #endif /* CONFIG_OF_DEVICE */
 
 #endif /* _LINUX_OF_DEVICE_H */

Now that the of_node pointer is part of struct device, of_device_get_modalias could be used on any struct device that has the device node pointer set. This patch changes of_device_get_modalias to accept a struct device instead of a struct of_device.

---
 arch/microblaze/include/asm/of_device.h |    3 ---
 drivers/macintosh/macio_sysfs.c         |    5 +----
 drivers/of/device.c                     |   16 ++++++----------
 include/linux/of_device.h               |    2 +-
 4 files changed, 8 insertions(+), 18 deletions(-)

index 58e627d..c9be534 100644
@@ -15,9 +15,6 @@
 #include <linux/device.h>
 #include <linux/of.h>
 
-extern ssize_t of_device_get_modalias(struct of_device *ofdev,
-					char *str, ssize_t len);
-
 extern struct of_device *of_device_alloc(struct device_node *np,
 					 const char *bus_id,
 					 struct device *parent);

index 6999ce5..6024038 100644
@@ -41,10 +41,7 @@ compatible_show (struct device *dev, struct device_attribute *attr, char *buf)
 static ssize_t modalias_show (struct device *dev, struct device_attribute *attr,
 			      char *buf)
 {
-	struct of_device *ofdev = to_of_device(dev);
-	int len;
-
-	len = of_device_get_modalias(ofdev, buf, PAGE_SIZE - 2);
+	int len = of_device_get_modalias(dev, buf, PAGE_SIZE - 2);
 
 	buf[len] = '\n';
 	buf[len+1] = 0;

index 275cc9c..c2a98f5 100644
@@ -68,10 +68,7 @@ static ssize_t name_show(struct device *dev,
 static ssize_t modalias_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	struct of_device *ofdev = to_of_device(dev);
-	ssize_t len = 0;
-
-	len = of_device_get_modalias(ofdev, buf, PAGE_SIZE - 2);
+	ssize_t len = of_device_get_modalias(dev, buf, PAGE_SIZE - 2);
 	buf[len] = '\n';
 	buf[len+1] = 0;
 	return len+1;

@@ -123,19 +120,18 @@ void of_device_unregister(struct of_device *ofdev)
 }
 EXPORT_SYMBOL(of_device_unregister);
 
-ssize_t of_device_get_modalias(struct of_device *ofdev,
-				char *str, ssize_t len)
+ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len)
 {
 	const char *compat;
 	int cplen, i;
 	ssize_t tsize, csize, repend;
 
 	/* Name & Type */
-	csize = snprintf(str, len, "of:N%sT%s", ofdev->dev.of_node->name,
-			 ofdev->dev.of_node->type);
+	csize = snprintf(str, len, "of:N%sT%s", dev->of_node->name,
+			 dev->of_node->type);
 
 	/* Get compatible property if any */
-	compat = of_get_property(ofdev->dev.of_node, "compatible", &cplen);
+	compat = of_get_property(dev->of_node, "compatible", &cplen);
 	if (!compat)
 		return csize;
 

@@ -210,7 +206,7 @@ int of_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 	if (add_uevent_var(env, "MODALIAS="))
 		return -ENOMEM;
 
-	sl = of_device_get_modalias(to_of_device(dev), &env->buf[env->buflen-1],
+	sl = of_device_get_modalias(dev, &env->buf[env->buflen-1],
 				    sizeof(env->buf) - env->buflen);
 	if (sl >= (sizeof(env->buf) - env->buflen))
 		return -ENOMEM;

index da83e73..238e92e 100644
@@ -42,7 +42,7 @@ static inline void of_device_free(struct of_device *dev)
 	of_release_dev(&dev->dev);
 }
 
-extern ssize_t of_device_get_modalias(struct of_device *ofdev,
+extern ssize_t of_device_get_modalias(struct device *dev,
 					char *str, ssize_t len);
 
 extern int of_device_uevent(struct device *dev, struct kobj_uevent_env *env);

Merge common code between PowerPC and microblaze. This patch merges the code that scans the tree and registers devices. The functions merged are of_platform_bus_probe(), of_platform_bus_create(), and of_platform_device_create().

This patch also move the of_default_bus_ids[] table out of a Microblaze header file and makes it non-static. The device ids table isn't merged because powerpc and microblaze use different default data.

---
 arch/microblaze/include/asm/of_platform.h |   33 -------
 arch/microblaze/kernel/of_platform.c      |  141 ++++-------------------------
 arch/powerpc/include/asm/of_platform.h    |   11 --
 arch/powerpc/kernel/of_platform.c         |  131 ---------------------------
 drivers/of/platform.c                     |  143 +++++++++++++++++++++++++++++
 include/linux/of_platform.h               |   17 +++
 6 files changed, 179 insertions(+), 297 deletions(-)

index 3749127..625003f 100644
@@ -14,39 +14,6 @@
 /* This is just here during the transition */
 #include <linux/of_platform.h>
 
-/*
- * The list of OF IDs below is used for matching bus types in the
- * system whose devices are to be exposed as of_platform_devices.
- *
- * This is the default list valid for most platforms. This file provides
- * functions who can take an explicit list if necessary though
- *
- * The search is always performed recursively looking for children of
- * the provided device_node and recursively if such a children matches
- * a bus type in the list
- */
-
-static const struct of_device_id of_default_bus_ids[] = {
-	{ .type = "soc", },
-	{ .compatible = "soc", },
-	{ .type = "plb5", },
-	{ .type = "plb4", },
-	{ .type = "opb", },
-	{ .type = "simple", },
-	{},
-};
-
-/* Platform devices and busses creation */
-extern struct of_device *of_platform_device_create(struct device_node *np,
-						const char *bus_id,
-						struct device *parent);
-/* pseudo "matches" value to not do deep probe */
-#define OF_NO_DEEP_PROBE ((struct of_device_id *)-1)
-
-extern int of_platform_bus_probe(struct device_node *root,
-				const struct of_device_id *matches,
-				struct device *parent);
-
 extern struct of_device *of_find_device_by_phandle(phandle ph);
 
 extern void of_instantiate_rtc(void);

index d4aaa34..b37d2dc 100644
@@ -11,17 +11,6 @@
  *
  */
 
-/* Platform devices and busses creation */
-extern struct of_device *of_platform_device_create(struct device_node *np,
-						   const char *bus_id,
-						   struct device *parent);
-/* pseudo "matches" value to not do deep probe */
-#define OF_NO_DEEP_PROBE ((struct of_device_id *)-1)
-
-extern int of_platform_bus_probe(struct device_node *root,
-				 const struct of_device_id *matches,
-				 struct device *parent);
-
 extern struct of_device *of_find_device_by_phandle(phandle ph);
 
 extern void of_instantiate_rtc(void);

index 1643d37..4bbba41 100644
@@ -25,6 +25,8 @@
  */
 extern struct bus_type of_platform_bus_type;
 
+extern const struct of_device_id of_default_bus_ids[];
+
 /*
  * An of_platform_driver driver is attached to a basic of_device on
  * the "platform bus" (of_platform_bus_type).

@@ -63,6 +65,21 @@ static inline void of_unregister_platform_driver(struct of_platform_driver *drv)
 extern struct of_device *of_find_device_by_node(struct device_node *np);
 
 extern int of_bus_type_init(struct bus_type *bus, const char *name);
+
+#if !defined(CONFIG_SPARC) /* SPARC has its own device registration method */
+/* Platform devices and busses creation */
+extern struct of_device *of_platform_device_create(struct device_node *np,
+						   const char *bus_id,
+						   struct device *parent);
+
+/* pseudo "matches" value to not do deep probe */
+#define OF_NO_DEEP_PROBE ((struct of_device_id *)-1)
+
+extern int of_platform_bus_probe(struct device_node *root,
+				 const struct of_device_id *matches,
+				 struct device *parent);
+#endif /* !CONFIG_SPARC */
+
 #endif /* CONFIG_OF_DEVICE */
 
 #endif	/* _LINUX_OF_PLATFORM_H */

Merge common code between PowerPC and Microblaze

---
 arch/microblaze/include/asm/device.h    |    3 +
 arch/microblaze/include/asm/of_device.h |   15 -------
 arch/microblaze/kernel/Makefile         |    2 -
 arch/microblaze/kernel/of_device.c      |   64 -------------------------------
 arch/powerpc/include/asm/device.h       |    3 +
 arch/powerpc/include/asm/of_device.h    |   10 -----
 arch/powerpc/kernel/of_device.c         |   61 ++++--------------------------
 drivers/of/platform.c                   |   53 ++++++++++++++++++++++++++
 include/linux/of_platform.h             |    4 ++
 9 files changed, 72 insertions(+), 143 deletions(-)
 delete mode 100644 arch/microblaze/kernel/of_device.c

index 123b2fe..5e63552 100644
@@ -21,6 +21,9 @@ struct pdev_archdata {
 	u64 dma_mask;
 };
 
+/* Don't override the default bus id behaviour */
+#define of_device_make_bus_id __of_device_make_bus_id
+
 #endif /* _ASM_MICROBLAZE_DEVICE_H */
 
 

index c9be534..47e8d42 100644
@@ -10,19 +10,4 @@
 
 #ifndef _ASM_MICROBLAZE_OF_DEVICE_H
 #define _ASM_MICROBLAZE_OF_DEVICE_H
-#ifdef __KERNEL__
-
-#include <linux/device.h>
-#include <linux/of.h>
-
-extern struct of_device *of_device_alloc(struct device_node *np,
-					 const char *bus_id,
-					 struct device *parent);
-
-extern void of_device_make_bus_id(struct of_device *dev);
-
-/* This is just here during the transition */
-#include <linux/of_device.h>
-
-#endif /* __KERNEL__ */
 #endif /* _ASM_MICROBLAZE_OF_DEVICE_H */

index e51bc15..727e2cb 100644
@@ -15,7 +15,7 @@ endif
 extra-y := head.o vmlinux.lds
 
 obj-y += dma.o exceptions.o \
-	hw_exception_handler.o init_task.o intc.o irq.o of_device.o \
+	hw_exception_handler.o init_task.o intc.o irq.o \
 	of_platform.o process.o prom.o prom_parse.o ptrace.o \
 	setup.o signal.o sys_microblaze.o timer.o traps.o reset.o
 

index a3954e4..3c897a8 100644
@@ -31,4 +31,7 @@ struct pdev_archdata {
 	u64 dma_mask;
 };
 
+/* Override the default bus_id behaviour for OF stuff */
+extern void of_device_make_bus_id(struct device *dev);
+
 #endif /* _ASM_POWERPC_DEVICE_H */

index 5d5103c..04f7671 100644
@@ -1,13 +1,3 @@
 #ifndef _ASM_POWERPC_OF_DEVICE_H
 #define _ASM_POWERPC_OF_DEVICE_H
-#ifdef __KERNEL__
-
-#include <linux/device.h>
-#include <linux/of.h>
-
-extern struct of_device *of_device_alloc(struct device_node *np,
-					 const char *bus_id,
-					 struct device *parent);
-
-#endif /* __KERNEL__ */
 #endif /* _ASM_POWERPC_OF_DEVICE_H */

index 4bbba41..8c82a2f 100644
@@ -60,6 +60,10 @@ static inline void of_unregister_platform_driver(struct of_platform_driver *drv)
 	of_unregister_driver(drv);
 }
 
+extern void __of_device_make_bus_id(struct device *dev);
+extern struct of_device *of_device_alloc(struct device_node *np,
+					 const char *bus_id,
+					 struct device *parent);
 #include <asm/of_platform.h>
 
 extern struct of_device *of_find_device_by_node(struct device_node *np);

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.

---
 drivers/of/platform.c |   31 ++++++++++++++++++++++++++++---
 1 files changed, 28 insertions(+), 3 deletions(-)

index 4bb898d..10ad0b6 100644
@@ -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

On Tue, Jun 08, 2010 at 08:26:43AM -0600, Grant Likely wrote:
[...]

Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

Allocating in one big block means less potential memory fragmentation, and the kernel can free it all at once. This is a common pattern.

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.

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 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: [email protected]
irc://irc.freenode.net/bd2
_______________________________________________
devicetree-discuss mailing list
[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss

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

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.

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

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

g.

Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

On Tue, Jun 08, 2010 at 12:41:47PM -0600, Grant Likely 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).

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]; };

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.

Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

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.

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.

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.

Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

In message: <AANLkTilpUi1cazljWSFbzliY78RKyHUlvBshUD3NPHPv-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Grant Likely <grant.likely-s3s/[email protected]> writes:
: On Thu, Jun 10, 2010 at 12:17 AM, Benjamin Herrenschmidt
: <[email protected]> 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

On Thu, Jun 10, 2010 at 09:13:57AM -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.

While

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

simply makes sense.

Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

On Thu, Jun 10, 2010 at 09:13:57AM -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.

While

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

simply makes sense.

Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

In message: <20100610154741.GA7484-wnGakbxT3iijyJ0x5qLZdcN33GVbZNy3@public.gmane.org>
Anton Vorontsov <[email protected]> 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

On Thu, Jun 10, 2010 at 10:01:40AM -0600, M. Warner Losh wrote:
[...]

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

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.

Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

Wow, there is some serious bikeshedding going on with this argument about structures and arrays .

I know! Isn't it fun?

g.

In message: <20100610165243.GA18043-wnGakbxT3iijyJ0x5qLZdcN33GVbZNy3@public.gmane.org>
Anton Vorontsov <[email protected]> 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

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:

Variant.

It's also less ugly.

Cheers,
Ben.

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.

Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

On Thu, Jun 10, 2010 at 10:30:26AM -0600, Grant Likely wrote:
[...]

You mean ofdev->resource = res; ?

Sounds great!

Thanks,

Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

Yeah, cut-and-paste error.

g.

On Thu, Jun 10, 2010 at 09:13:57AM -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.

While

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

simply makes sense.

Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

On Thu, Jun 10, 2010 at 09:13:57AM -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.

While

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

simply makes sense.

Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2