| Message ID | 1386348867-25038-4-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
Paolo Bonzini <pbonzini@redhat.com> writes: > Resetting should be done in post-order, not pre-order. However, > qdev_walk_children and qbus_walk_children do not allow this. Fix > it by adding two extra arguments to the functions. > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/core/qdev.c | 45 +++++++++++++++++++++++++++++++++------------ > include/hw/qdev-core.h | 13 +++++++++---- > 2 files changed, 42 insertions(+), 16 deletions(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 758de9f..1c114b7 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -240,12 +240,12 @@ static int qbus_reset_one(BusState *bus, void *opaque) > > void qdev_reset_all(DeviceState *dev) > { > - qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL); > + qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL); > } > > void qbus_reset_all(BusState *bus) > { > - qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL); > + qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL); > } > > void qbus_reset_all_fn(void *opaque) > @@ -343,49 +343,70 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name) > return NULL; > } > > -int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn, > - qbus_walkerfn *busfn, void *opaque) > +int qbus_walk_children(BusState *bus, > + qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn, > + qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, > + void *opaque) > { It's either pre or post right ? I also thought that the traversal applies to the whole hierarchy not just buses or devices individually.. Why not just have a single parameter that says pre or post, not much difference but atleast one parameter less. Bandan > BusChild *kid; > int err; > > - if (busfn) { > - err = busfn(bus, opaque); > + if (pre_busfn) { > + err = pre_busfn(bus, opaque); > if (err) { > return err; > } > } > > QTAILQ_FOREACH(kid, &bus->children, sibling) { > - err = qdev_walk_children(kid->child, devfn, busfn, opaque); > + err = qdev_walk_children(kid->child, > + pre_devfn, pre_busfn, > + post_devfn, post_busfn, opaque); > if (err < 0) { > return err; > } > } > > + if (post_busfn) { > + err = post_busfn(bus, opaque); > + if (err) { > + return err; > + } > + } > + > return 0; > } > > -int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn, > - qbus_walkerfn *busfn, void *opaque) > +int qdev_walk_children(DeviceState *dev, > + qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn, > + qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, > + void *opaque) > { > BusState *bus; > int err; > > - if (devfn) { > - err = devfn(dev, opaque); > + if (pre_devfn) { > + err = pre_devfn(dev, opaque); > if (err) { > return err; > } > } > > QLIST_FOREACH(bus, &dev->child_bus, sibling) { > - err = qbus_walk_children(bus, devfn, busfn, opaque); > + err = qbus_walk_children(bus, pre_devfn, pre_busfn, > + post_devfn, post_busfn, opaque); > if (err < 0) { > return err; > } > } > > + if (post_devfn) { > + err = post_devfn(dev, opaque); > + if (err) { > + return err; > + } > + } > + > return 0; > } > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index d840f06..21ea2c6 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -274,10 +274,15 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam > /* Returns > 0 if either devfn or busfn skip walk somewhere in cursion, > * < 0 if either devfn or busfn terminate walk somewhere in cursion, > * 0 otherwise. */ > -int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn, > - qbus_walkerfn *busfn, void *opaque); > -int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn, > - qbus_walkerfn *busfn, void *opaque); > +int qbus_walk_children(BusState *bus, > + qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn, > + qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, > + void *opaque); > +int qdev_walk_children(DeviceState *dev, > + qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn, > + qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, > + void *opaque); > + > void qdev_reset_all(DeviceState *dev); > > /**
Il 07/12/2013 00:27, Bandan Das ha scritto: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> Resetting should be done in post-order, not pre-order. However, >> qdev_walk_children and qbus_walk_children do not allow this. Fix >> it by adding two extra arguments to the functions. >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> hw/core/qdev.c | 45 +++++++++++++++++++++++++++++++++------------ >> include/hw/qdev-core.h | 13 +++++++++---- >> 2 files changed, 42 insertions(+), 16 deletions(-) >> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >> index 758de9f..1c114b7 100644 >> --- a/hw/core/qdev.c >> +++ b/hw/core/qdev.c >> @@ -240,12 +240,12 @@ static int qbus_reset_one(BusState *bus, void *opaque) >> >> void qdev_reset_all(DeviceState *dev) >> { >> - qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL); >> + qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL); >> } >> >> void qbus_reset_all(BusState *bus) >> { >> - qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL); >> + qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL); >> } >> >> void qbus_reset_all_fn(void *opaque) >> @@ -343,49 +343,70 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name) >> return NULL; >> } >> >> -int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn, >> - qbus_walkerfn *busfn, void *opaque) >> +int qbus_walk_children(BusState *bus, >> + qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn, >> + qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, >> + void *opaque) >> { > > It's either pre or post right ? I also thought that the traversal > applies to the whole hierarchy not just buses or devices individually.. > Why not just have a single parameter that says pre or post, not much > difference but atleast one parameter less. This is a generic walk function. For reset you want post-order, but in other cases pre-order may make more sense, for example realize. Paolo > Bandan > >> BusChild *kid; >> int err; >> >> - if (busfn) { >> - err = busfn(bus, opaque); >> + if (pre_busfn) { >> + err = pre_busfn(bus, opaque); >> if (err) { >> return err; >> } >> } >> >> QTAILQ_FOREACH(kid, &bus->children, sibling) { >> - err = qdev_walk_children(kid->child, devfn, busfn, opaque); >> + err = qdev_walk_children(kid->child, >> + pre_devfn, pre_busfn, >> + post_devfn, post_busfn, opaque); >> if (err < 0) { >> return err; >> } >> } >> >> + if (post_busfn) { >> + err = post_busfn(bus, opaque); >> + if (err) { >> + return err; >> + } >> + } >> + >> return 0; >> } >> >> -int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn, >> - qbus_walkerfn *busfn, void *opaque) >> +int qdev_walk_children(DeviceState *dev, >> + qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn, >> + qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, >> + void *opaque) >> { >> BusState *bus; >> int err; >> >> - if (devfn) { >> - err = devfn(dev, opaque); >> + if (pre_devfn) { >> + err = pre_devfn(dev, opaque); >> if (err) { >> return err; >> } >> } >> >> QLIST_FOREACH(bus, &dev->child_bus, sibling) { >> - err = qbus_walk_children(bus, devfn, busfn, opaque); >> + err = qbus_walk_children(bus, pre_devfn, pre_busfn, >> + post_devfn, post_busfn, opaque); >> if (err < 0) { >> return err; >> } >> } >> >> + if (post_devfn) { >> + err = post_devfn(dev, opaque); >> + if (err) { >> + return err; >> + } >> + } >> + >> return 0; >> } >> >> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >> index d840f06..21ea2c6 100644 >> --- a/include/hw/qdev-core.h >> +++ b/include/hw/qdev-core.h >> @@ -274,10 +274,15 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam >> /* Returns > 0 if either devfn or busfn skip walk somewhere in cursion, >> * < 0 if either devfn or busfn terminate walk somewhere in cursion, >> * 0 otherwise. */ >> -int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn, >> - qbus_walkerfn *busfn, void *opaque); >> -int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn, >> - qbus_walkerfn *busfn, void *opaque); >> +int qbus_walk_children(BusState *bus, >> + qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn, >> + qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, >> + void *opaque); >> +int qdev_walk_children(DeviceState *dev, >> + qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn, >> + qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, >> + void *opaque); >> + >> void qdev_reset_all(DeviceState *dev); >> >> /**
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 07/12/2013 00:27, Bandan Das ha scritto: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> Resetting should be done in post-order, not pre-order. However, >>> qdev_walk_children and qbus_walk_children do not allow this. Fix >>> it by adding two extra arguments to the functions. >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>> --- >>> hw/core/qdev.c | 45 +++++++++++++++++++++++++++++++++------------ >>> include/hw/qdev-core.h | 13 +++++++++---- >>> 2 files changed, 42 insertions(+), 16 deletions(-) >>> >>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >>> index 758de9f..1c114b7 100644 >>> --- a/hw/core/qdev.c >>> +++ b/hw/core/qdev.c >>> @@ -240,12 +240,12 @@ static int qbus_reset_one(BusState *bus, void *opaque) >>> >>> void qdev_reset_all(DeviceState *dev) >>> { >>> - qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL); >>> + qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL); >>> } >>> >>> void qbus_reset_all(BusState *bus) >>> { >>> - qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL); >>> + qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL); >>> } >>> >>> void qbus_reset_all_fn(void *opaque) >>> @@ -343,49 +343,70 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name) >>> return NULL; >>> } >>> >>> -int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn, >>> - qbus_walkerfn *busfn, void *opaque) >>> +int qbus_walk_children(BusState *bus, >>> + qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn, >>> + qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, >>> + void *opaque) >>> { >> >> It's either pre or post right ? I also thought that the traversal >> applies to the whole hierarchy not just buses or devices individually.. >> Why not just have a single parameter that says pre or post, not much >> difference but atleast one parameter less. > > This is a generic walk function. > For reset you want post-order, but in other cases pre-order may make > more sense, for example realize. Sorry, not sure if I get it. What I meant was will this work ? int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn, - qbus_walkerfn *busfn, void *opaque) + qbus_walkerfn *busfn, bool ispostorder, void *opaque) { BusChild *kid; int err; - if (busfn) { + if (!ispostorder && busfn) { err = busfn(bus, opaque); if (err) { return err; @@ -351,17 +351,24 @@ int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn, } QTAILQ_FOREACH(kid, &bus->children, sibling) { - err = qdev_walk_children(kid->child, devfn, busfn, opaque); + err = qdev_walk_children(kid->child, devfn, busfn, ispostorder, opaque); if (err < 0) { return err; } } + if (ispostorder && busfn) { + err = busfn(bus, opaque); + if (err) { + return err; + } + } + return 0; } Or there's a case where we would like to traverse pre for parent and post for children's buses (or something similar).. > Paolo > >> Bandan >> >>> BusChild *kid; >>> int err; >>> >>> - if (busfn) { >>> - err = busfn(bus, opaque); >>> + if (pre_busfn) { >>> + err = pre_busfn(bus, opaque); >>> if (err) { >>> return err; >>> } >>> } >>> >>> QTAILQ_FOREACH(kid, &bus->children, sibling) { >>> - err = qdev_walk_children(kid->child, devfn, busfn, opaque); >>> + err = qdev_walk_children(kid->child, >>> + pre_devfn, pre_busfn, >>> + post_devfn, post_busfn, opaque); >>> if (err < 0) { >>> return err; >>> } >>> } >>> >>> + if (post_busfn) { >>> + err = post_busfn(bus, opaque); >>> + if (err) { >>> + return err; >>> + } >>> + } >>> + >>> return 0; >>> } >>> >>> -int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn, >>> - qbus_walkerfn *busfn, void *opaque) >>> +int qdev_walk_children(DeviceState *dev, >>> + qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn, >>> + qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, >>> + void *opaque) >>> { >>> BusState *bus; >>> int err; >>> >>> - if (devfn) { >>> - err = devfn(dev, opaque); >>> + if (pre_devfn) { >>> + err = pre_devfn(dev, opaque); >>> if (err) { >>> return err; >>> } >>> } >>> >>> QLIST_FOREACH(bus, &dev->child_bus, sibling) { >>> - err = qbus_walk_children(bus, devfn, busfn, opaque); >>> + err = qbus_walk_children(bus, pre_devfn, pre_busfn, >>> + post_devfn, post_busfn, opaque); >>> if (err < 0) { >>> return err; >>> } >>> } >>> >>> + if (post_devfn) { >>> + err = post_devfn(dev, opaque); >>> + if (err) { >>> + return err; >>> + } >>> + } >>> + >>> return 0; >>> } >>> >>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >>> index d840f06..21ea2c6 100644 >>> --- a/include/hw/qdev-core.h >>> +++ b/include/hw/qdev-core.h >>> @@ -274,10 +274,15 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam >>> /* Returns > 0 if either devfn or busfn skip walk somewhere in cursion, >>> * < 0 if either devfn or busfn terminate walk somewhere in cursion, >>> * 0 otherwise. */ >>> -int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn, >>> - qbus_walkerfn *busfn, void *opaque); >>> -int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn, >>> - qbus_walkerfn *busfn, void *opaque); >>> +int qbus_walk_children(BusState *bus, >>> + qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn, >>> + qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, >>> + void *opaque); >>> +int qdev_walk_children(DeviceState *dev, >>> + qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn, >>> + qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, >>> + void *opaque); >>> + >>> void qdev_reset_all(DeviceState *dev); >>> >>> /**
Il 09/12/2013 18:56, Bandan Das ha scritto: >> > This is a generic walk function. >> > For reset you want post-order, but in other cases pre-order may make >> > more sense, for example realize. > Sorry, not sure if I get it. What I meant was will this work ? > > int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn, > - qbus_walkerfn *busfn, void *opaque) > + qbus_walkerfn *busfn, bool ispostorder, void *opaque) Yes, but it is a bit less flexible. > Or there's a case where we would like to traverse pre for parent and post > for children's buses (or something similar).. Probably not, but there may be a case where you want both pre and post (i.e. before and after). For example a "display tree" functionality where the post-order callbacks simply decrement the current indentation. Paolo
Il 09/12/2013 18:56, Bandan Das ha scritto: >> > This is a generic walk function. >> > For reset you want post-order, but in other cases pre-order may make >> > more sense, for example realize. > Sorry, not sure if I get it. What I meant was will this work ? > > int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn, > - qbus_walkerfn *busfn, void *opaque) > + qbus_walkerfn *busfn, bool ispostorder, void *opaque) Yes, but it is a bit less flexible. If we want to remove the flexibility, I'd rather have two separate functions for pre- and post-order, not a new "bool" parameter. > Or there's a case where we would like to traverse pre for parent and post > for children's buses (or something similar).. Probably not, but there may be a case where you want both pre and post (i.e. before and after). For example a "display tree" functionality where the post-order callbacks simply decrement the current indentation. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 09/12/2013 18:56, Bandan Das ha scritto: >>> > This is a generic walk function. >>> > For reset you want post-order, but in other cases pre-order may make >>> > more sense, for example realize. >> Sorry, not sure if I get it. What I meant was will this work ? >> >> int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn, >> - qbus_walkerfn *busfn, void *opaque) >> + qbus_walkerfn *busfn, bool ispostorder, void *opaque) > > Yes, but it is a bit less flexible. Well, my motivation was that very soon we will have an infinite number of arguments to the walk functions (oh wait! we already have) :) >> Or there's a case where we would like to traverse pre for parent and post >> for children's buses (or something similar).. > > Probably not, but there may be a case where you want both pre and post > (i.e. before and after). For example a "display tree" functionality > where the post-order callbacks simply decrement the current indentation. Oh so there is such a case. Ok, got it now, thanks! > Paolo
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 758de9f..1c114b7 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -240,12 +240,12 @@ static int qbus_reset_one(BusState *bus, void *opaque) void qdev_reset_all(DeviceState *dev) { - qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL); + qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL); } void qbus_reset_all(BusState *bus) { - qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL); + qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL, NULL, NULL); } void qbus_reset_all_fn(void *opaque) @@ -343,49 +343,70 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name) return NULL; } -int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn, - qbus_walkerfn *busfn, void *opaque) +int qbus_walk_children(BusState *bus, + qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn, + qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, + void *opaque) { BusChild *kid; int err; - if (busfn) { - err = busfn(bus, opaque); + if (pre_busfn) { + err = pre_busfn(bus, opaque); if (err) { return err; } } QTAILQ_FOREACH(kid, &bus->children, sibling) { - err = qdev_walk_children(kid->child, devfn, busfn, opaque); + err = qdev_walk_children(kid->child, + pre_devfn, pre_busfn, + post_devfn, post_busfn, opaque); if (err < 0) { return err; } } + if (post_busfn) { + err = post_busfn(bus, opaque); + if (err) { + return err; + } + } + return 0; } -int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn, - qbus_walkerfn *busfn, void *opaque) +int qdev_walk_children(DeviceState *dev, + qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn, + qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, + void *opaque) { BusState *bus; int err; - if (devfn) { - err = devfn(dev, opaque); + if (pre_devfn) { + err = pre_devfn(dev, opaque); if (err) { return err; } } QLIST_FOREACH(bus, &dev->child_bus, sibling) { - err = qbus_walk_children(bus, devfn, busfn, opaque); + err = qbus_walk_children(bus, pre_devfn, pre_busfn, + post_devfn, post_busfn, opaque); if (err < 0) { return err; } } + if (post_devfn) { + err = post_devfn(dev, opaque); + if (err) { + return err; + } + } + return 0; } diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index d840f06..21ea2c6 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -274,10 +274,15 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam /* Returns > 0 if either devfn or busfn skip walk somewhere in cursion, * < 0 if either devfn or busfn terminate walk somewhere in cursion, * 0 otherwise. */ -int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn, - qbus_walkerfn *busfn, void *opaque); -int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn, - qbus_walkerfn *busfn, void *opaque); +int qbus_walk_children(BusState *bus, + qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn, + qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, + void *opaque); +int qdev_walk_children(DeviceState *dev, + qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn, + qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, + void *opaque); + void qdev_reset_all(DeviceState *dev); /**
Resetting should be done in post-order, not pre-order. However, qdev_walk_children and qbus_walk_children do not allow this. Fix it by adding two extra arguments to the functions. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/core/qdev.c | 45 +++++++++++++++++++++++++++++++++------------ include/hw/qdev-core.h | 13 +++++++++---- 2 files changed, 42 insertions(+), 16 deletions(-)