Patchwork [2/3] Clean up upcast from PCIDevice to I6300State

login
register
mail settings
Submitter Markus Armbruster
Date Aug. 21, 2009, 8:31 a.m.
Message ID <1250843494-28326-3-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/31809/
State Superseded
Headers show

Comments

Markus Armbruster - Aug. 21, 2009, 8:31 a.m.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/wdt_i6300esb.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)
Markus Armbruster - Aug. 24, 2009, 8:49 a.m.
We got container_of() in osdep.h.

We also got DO_UPCAST() in qdev.h.  Odd place, as it's not really
specific to qdev (except for the naming of the last parameter).  It
takes the same parameters as container_of(), but in a different order,
which I find needlessly confusing.  The last parameter is insufficiently
parenthesized in the macro expansion.  Finally, I don't really like the
name --- I find container_of() much clearer --- but that's just me.

Why do we have two macros to do essentially the same thing?

When should container_of() be used, and when DO_UPCAST()?
Paul Brook - Aug. 24, 2009, 11:55 a.m.
On Monday 24 August 2009, Markus Armbruster wrote:
> We got container_of() in osdep.h.
>
> We also got DO_UPCAST() in qdev.h.  Odd place, as it's not really
> specific to qdev (except for the naming of the last parameter).  It
> takes the same parameters as container_of(), but in a different order,
> which I find needlessly confusing.  The last parameter is insufficiently
> parenthesized in the macro expansion.  Finally, I don't really like the
> name --- I find container_of() much clearer --- but that's just me.
>
> Why do we have two macros to do essentially the same thing?
>
> When should container_of() be used, and when DO_UPCAST()?

DO_UPCAST requires that the child object be at offset zero.

Paul
Paul Brook - Aug. 24, 2009, 11:59 a.m.
>  /* Device state. */
>  struct I6300State {
> -    PCIDevice dev;             /* PCI device state, must be first field. */
> +    PCIDevice dev;
>...
> -    I6300State *d = (I6300State *) dev;
> +    I6300State *d = container_of(dev, I6300State, dev);

I'm pretty sure this is wrong, and code elsewhere still requires the PCIDevice 
be the first field in I6300State. e.g. i6300esb_pc_init. Did you actually try 
putting dev at a nonzero offset?

Paul
Markus Armbruster - Aug. 24, 2009, 1:44 p.m.
Paul Brook <paul@codesourcery.com> writes:

>>  /* Device state. */
>>  struct I6300State {
>> -    PCIDevice dev;             /* PCI device state, must be first field. */
>> +    PCIDevice dev;
>>...
>> -    I6300State *d = (I6300State *) dev;
>> +    I6300State *d = container_of(dev, I6300State, dev);
>
> I'm pretty sure this is wrong, and code elsewhere still requires the PCIDevice 
> be the first field in I6300State. e.g. i6300esb_pc_init. Did you actually try 
> putting dev at a nonzero offset?

I believe this commit is correct, because no code remains in
wdt_i6300esb.c that relies on the zero offset, and qdev is not yet in
play.  However, the *next* commit (conversion to qdev) is wrong if qdev
requires the PCIDevice at offset zero (not obvious to me, document it?).

I'll use DO_UPCAST() when I respin the patch series, to match usage
elsewhere.

Thanks!
Gerd Hoffmann - Aug. 24, 2009, 3:07 p.m.
Hi,

> play.  However, the *next* commit (conversion to qdev) is wrong if qdev
> requires the PCIDevice at offset zero (not obvious to me, document it?).

qdev_create() allocates DeviceInfo->size bytes for you and assumes 
DeviceState starts at offset zero of your FooDeviceState.  I think that 
is the only place though.

Could probably be relaxed by adding a offset field to DeviceInfo, but I 
don't think that buys us much, so unless there is some real need I'd 
just go with the "DeviceState must be first" convention.

It also could be that there is some old code which does casts instead of 
using DO_UPCAST, especially for PCI because PCIDevice predates qdev. 
That I'd consider a bug though.

cheers,
   Gerd

Patch

diff --git a/hw/wdt_i6300esb.c b/hw/wdt_i6300esb.c
index 5e9fd7c..2227303 100644
--- a/hw/wdt_i6300esb.c
+++ b/hw/wdt_i6300esb.c
@@ -25,7 +25,6 @@ 
 #include "qemu-timer.h"
 #include "watchdog.h"
 #include "hw.h"
-#include "isa.h"
 #include "pc.h"
 #include "pci.h"
 
@@ -71,7 +70,7 @@ 
 
 /* Device state. */
 struct I6300State {
-    PCIDevice dev;              /* PCI device state, must be first field. */
+    PCIDevice dev;
 
     int reboot_enabled;         /* "Reboot" on timer expiry.  The real action
                                  * performed depends on the -watchdog-action
@@ -199,7 +198,7 @@  static void i6300esb_timer_expired(void *vp)
 static void i6300esb_config_write(PCIDevice *dev, uint32_t addr,
                                   uint32_t data, int len)
 {
-    I6300State *d = (I6300State *) dev;
+    I6300State *d = container_of(dev, I6300State, dev);
     int old;
 
     i6300esb_debug("addr = %x, data = %x, len = %d\n", addr, data, len);
@@ -227,7 +226,7 @@  static void i6300esb_config_write(PCIDevice *dev, uint32_t addr,
 
 static uint32_t i6300esb_config_read(PCIDevice *dev, uint32_t addr, int len)
 {
-    I6300State *d = (I6300State *) dev;
+    I6300State *d = container_of(dev, I6300State, dev);
     uint32_t data;
 
     i6300esb_debug ("addr = %x, len = %d\n", addr, len);
@@ -361,7 +360,7 @@  static void i6300esb_map(PCIDevice *dev, int region_num,
         i6300esb_mem_writew,
         i6300esb_mem_writel,
     };
-    I6300State *d = (I6300State *) dev;
+    I6300State *d = container_of(dev, I6300State, dev);
     int io_mem;
 
     i6300esb_debug("addr = %x, size = %x, type = %d\n", addr, size, type);