| Message ID | 20200123132823.1117486-9-damien.hedde@greensocs.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Multi-phase reset mechanism | expand |
On 1/23/20 2:28 PM, Damien Hedde wrote: > Deprecate device_legacy_reset(), qdev_reset_all() and > qbus_reset_all() to be replaced by new functions > device_cold_reset() and bus_cold_reset() which uses resettable API. > > Also introduce resettable_cold_reset_fn() which may be used as a > replacement for qdev_reset_all_fn and qbus_reset_all_fn(). > > Following patches will be needed to look at legacy reset call sites > and switch to resettable api. The legacy functions will be removed > when unused. > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > include/hw/qdev-core.h | 27 +++++++++++++++++++++++++++ > include/hw/resettable.h | 9 +++++++++ > hw/core/bus.c | 5 +++++ > hw/core/qdev.c | 5 +++++ > hw/core/resettable.c | 5 +++++ > 5 files changed, 51 insertions(+) > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 1b4b420617..b84fcc32bf 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -406,6 +406,13 @@ int qdev_walk_children(DeviceState *dev, > qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, > void *opaque); > > +/** > + * @qdev_reset_all: > + * Reset @dev. See @qbus_reset_all() for more details. > + * > + * Note: This function is deprecated and will be removed when it becomes unused. > + * Please use device_cold_reset() now. > + */ > void qdev_reset_all(DeviceState *dev); > void qdev_reset_all_fn(void *opaque); > > @@ -418,10 +425,28 @@ void qdev_reset_all_fn(void *opaque); > * hard reset means that qbus_reset_all will reset all state of the device. > * For PCI devices, for example, this will include the base address registers > * or configuration space. > + * > + * Note: This function is deprecated and will be removed when it becomes unused. > + * Please use bus_cold_reset() now. Some time passed, so looking at this with some retrospective. If there is an effort to introduce a new API replacing another one, we should try convert all the uses of the old API to the new one, instead of declaring it legacy. Declare an API legacy/deprecated should be the last resort if there is no way to remove it. I'd recommend to move the deprecated/legacy declarations in a separate header, with the '_legacy' suffix. Else: 1/ we never finish API conversions, 2/ the new API might not be ready for all the legacy API use cases, 3/ we end up having to maintain 2 different APIs. So the recommendation is to use bus_cold_reset(), but it isn't used anywhere...: $ git grep bus_cold_reset docs/devel/reset.rst:64:- ``bus_cold_reset()`` hw/core/bus.c:73:void bus_cold_reset(BusState *bus) include/hw/qdev-core.h:715: * Please use bus_cold_reset() now. include/hw/qdev-core.h:728: * bus_cold_reset: include/hw/qdev-core.h:733:void bus_cold_reset(BusState *bus); IMHO we shouldn't add new public prototypes without callers. I see it is similar to device_cold_reset(), but TBH I'm scared to be the first one using it. Regards, Phil. > */ > void qbus_reset_all(BusState *bus); > void qbus_reset_all_fn(void *opaque); > > +/** > + * device_cold_reset: > + * Reset device @dev and perform a recursive processing using the resettable > + * interface. It triggers a RESET_TYPE_COLD. > + */ > +void device_cold_reset(DeviceState *dev); > + > +/** > + * bus_cold_reset: > + * > + * Reset bus @bus and perform a recursive processing using the resettable > + * interface. It triggers a RESET_TYPE_COLD. > + */ > +void bus_cold_reset(BusState *bus);
On Tue, Apr 27, 2021 at 02:21:28PM +0200, Philippe Mathieu-Daudé wrote: > On 1/23/20 2:28 PM, Damien Hedde wrote: > > Deprecate device_legacy_reset(), qdev_reset_all() and > > qbus_reset_all() to be replaced by new functions > > device_cold_reset() and bus_cold_reset() which uses resettable API. > > > > Also introduce resettable_cold_reset_fn() which may be used as a > > replacement for qdev_reset_all_fn and qbus_reset_all_fn(). > > > > Following patches will be needed to look at legacy reset call sites > > and switch to resettable api. The legacy functions will be removed > > when unused. > > > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > --- > > include/hw/qdev-core.h | 27 +++++++++++++++++++++++++++ > > include/hw/resettable.h | 9 +++++++++ > > hw/core/bus.c | 5 +++++ > > hw/core/qdev.c | 5 +++++ > > hw/core/resettable.c | 5 +++++ > > 5 files changed, 51 insertions(+) > > > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > index 1b4b420617..b84fcc32bf 100644 > > --- a/include/hw/qdev-core.h > > +++ b/include/hw/qdev-core.h > > @@ -406,6 +406,13 @@ int qdev_walk_children(DeviceState *dev, > > qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, > > void *opaque); > > > > +/** > > + * @qdev_reset_all: > > + * Reset @dev. See @qbus_reset_all() for more details. > > + * > > + * Note: This function is deprecated and will be removed when it becomes unused. > > + * Please use device_cold_reset() now. > > + */ > > void qdev_reset_all(DeviceState *dev); > > void qdev_reset_all_fn(void *opaque); > > > > @@ -418,10 +425,28 @@ void qdev_reset_all_fn(void *opaque); > > * hard reset means that qbus_reset_all will reset all state of the device. > > * For PCI devices, for example, this will include the base address registers > > * or configuration space. > > + * > > + * Note: This function is deprecated and will be removed when it becomes unused. > > + * Please use bus_cold_reset() now. > > Some time passed, so looking at this with some retrospective. > > If there is an effort to introduce a new API replacing another one, > we should try convert all the uses of the old API to the new one, > instead of declaring it legacy. > > Declare an API legacy/deprecated should be the last resort if there > is no way to remove it. I'd recommend to move the deprecated/legacy > declarations in a separate header, with the '_legacy' suffix. > > Else: > > 1/ we never finish API conversions, > 2/ the new API might not be ready for all the legacy API use cases, > 3/ we end up having to maintain 2 different APIs. > > > So the recommendation is to use bus_cold_reset(), but it isn't > used anywhere...: > > $ git grep bus_cold_reset > docs/devel/reset.rst:64:- ``bus_cold_reset()`` > hw/core/bus.c:73:void bus_cold_reset(BusState *bus) > include/hw/qdev-core.h:715: * Please use bus_cold_reset() now. > include/hw/qdev-core.h:728: * bus_cold_reset: > include/hw/qdev-core.h:733:void bus_cold_reset(BusState *bus); > > IMHO we shouldn't add new public prototypes without callers. I agree. We should make at least some effort to convert code to the new API, if only to serve as reference for people doing the conversion. I'm surprised that a new function was added more than a year ago and nobody is using it. What happened here? Was there some plan to convert existing code but it was abandoned? > > I see it is similar to device_cold_reset(), but TBH I'm scared > to be the first one using it. > > Regards, > > Phil. > > > */ > > void qbus_reset_all(BusState *bus); > > void qbus_reset_all_fn(void *opaque); > > > > +/** > > + * device_cold_reset: > > + * Reset device @dev and perform a recursive processing using the resettable > > + * interface. It triggers a RESET_TYPE_COLD. > > + */ > > +void device_cold_reset(DeviceState *dev); > > + > > +/** > > + * bus_cold_reset: > > + * > > + * Reset bus @bus and perform a recursive processing using the resettable > > + * interface. It triggers a RESET_TYPE_COLD. > > + */ > > +void bus_cold_reset(BusState *bus); >
Eduardo Habkost <ehabkost@redhat.com> writes: > On Tue, Apr 27, 2021 at 02:21:28PM +0200, Philippe Mathieu-Daudé wrote: >> On 1/23/20 2:28 PM, Damien Hedde wrote: >> > Deprecate device_legacy_reset(), qdev_reset_all() and >> > qbus_reset_all() to be replaced by new functions >> > device_cold_reset() and bus_cold_reset() which uses resettable API. >> > >> > Also introduce resettable_cold_reset_fn() which may be used as a >> > replacement for qdev_reset_all_fn and qbus_reset_all_fn(). >> > >> > Following patches will be needed to look at legacy reset call sites >> > and switch to resettable api. The legacy functions will be removed >> > when unused. >> > >> > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> >> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> >> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> > Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> > --- >> > include/hw/qdev-core.h | 27 +++++++++++++++++++++++++++ >> > include/hw/resettable.h | 9 +++++++++ >> > hw/core/bus.c | 5 +++++ >> > hw/core/qdev.c | 5 +++++ >> > hw/core/resettable.c | 5 +++++ >> > 5 files changed, 51 insertions(+) >> > >> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >> > index 1b4b420617..b84fcc32bf 100644 >> > --- a/include/hw/qdev-core.h >> > +++ b/include/hw/qdev-core.h >> > @@ -406,6 +406,13 @@ int qdev_walk_children(DeviceState *dev, >> > qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, >> > void *opaque); >> > >> > +/** >> > + * @qdev_reset_all: >> > + * Reset @dev. See @qbus_reset_all() for more details. >> > + * >> > + * Note: This function is deprecated and will be removed when it becomes unused. >> > + * Please use device_cold_reset() now. >> > + */ >> > void qdev_reset_all(DeviceState *dev); >> > void qdev_reset_all_fn(void *opaque); >> > >> > @@ -418,10 +425,28 @@ void qdev_reset_all_fn(void *opaque); >> > * hard reset means that qbus_reset_all will reset all state of the device. >> > * For PCI devices, for example, this will include the base address registers >> > * or configuration space. >> > + * >> > + * Note: This function is deprecated and will be removed when it becomes unused. >> > + * Please use bus_cold_reset() now. >> >> Some time passed, so looking at this with some retrospective. >> >> If there is an effort to introduce a new API replacing another one, >> we should try convert all the uses of the old API to the new one, >> instead of declaring it legacy. >> >> Declare an API legacy/deprecated should be the last resort if there >> is no way to remove it. I'd recommend to move the deprecated/legacy >> declarations in a separate header, with the '_legacy' suffix. >> >> Else: >> >> 1/ we never finish API conversions, >> 2/ the new API might not be ready for all the legacy API use cases, >> 3/ we end up having to maintain 2 different APIs. >> >> >> So the recommendation is to use bus_cold_reset(), but it isn't >> used anywhere...: >> >> $ git grep bus_cold_reset >> docs/devel/reset.rst:64:- ``bus_cold_reset()`` >> hw/core/bus.c:73:void bus_cold_reset(BusState *bus) >> include/hw/qdev-core.h:715: * Please use bus_cold_reset() now. >> include/hw/qdev-core.h:728: * bus_cold_reset: >> include/hw/qdev-core.h:733:void bus_cold_reset(BusState *bus); >> >> IMHO we shouldn't add new public prototypes without callers. > > I agree. We should make at least some effort to convert code to > the new API, if only to serve as reference for people doing the > conversion. I'm surprised that a new function was added more > than a year ago and nobody is using it. > > What happened here? Was there some plan to convert existing code > but it was abandoned? Commit abb89dbf2 introduced bus_cold_reset() and device_cold_reset(). It was posted as part of "[PATCH v8 00/11] Multi-phase reset mechanism". The series did not add any users. The cover letter explains: The purpose of this series is to split the current reset procedure into multiple phases. This will help to solve some ordering difficulties we have during reset. This is a ready to merge version. I've taken the few remarks of Philippe about v7 in account. Thanks to him for all the tests he did. This series adds resettable interface and transitions base Device and Bus classes (sysbus subclasses are ok too). It provides new reset functions but does not switch anymore the old functions (device_reset() and qdev/qbus_reset_all()) to resettable interface. These functions keep the exact same behavior as before. The series also transition the main reset handlers registration which has no impact until devices and buses are transitioned. The series is organized as follows: Patch 1 prepare the reset transition. Patch 2 adds some utility trace events. Patches 3 to 8 adds resettable api in devices and buses. Patch 9 adds some documentation. Patches 10 and 11 transition the call sites of qemu_register_reset(qdev/qbus_reset_all_fn, ...). After this series, the plan is then to transition devices, buses and legacy reset call sites. Devices and buses have to be transitioned from mother class to daughter classes order but until the final (daughter) class is transitioned, old monolitic reset behavior will be kept for this class. bus_cold_reset() has never seen any use. The only transitioning to device_cold_reset() I can find is Peter Maydell's 781c67ca55 cpu: Use DeviceClass reset instead of a special CPUClass reset Then there's a QOMification series that uses device_cold_reset() temporarily, also by Peter: 4bebb9ad4e hw/arm/stellaris: Convert SSYS to QOM device 14711b6f54 hw/arm/stellaris: Remove board-creation reset of STELLARIS_SYS Finally, Bin Meng, Philippe Mathieu-Daudé, and Luc Michel added new code using it: c696e1f2b3 hw/sd: Add Cadence SDHCI emulation 65ad1da23e hw/misc/mps2-fpgaio: Use the LED device 435db7ebf5 hw/misc/mps2-scc: Use the LED device 1e986e25d0 hw/misc/bcm2835_cprman: add a PLL skeleton implementation 09d56bbc9b hw/misc/bcm2835_cprman: add a PLL channel skeleton implementation 7281362484 hw/misc/bcm2835_cprman: add a clock mux skeleton implementation 502960ca04 hw/misc/bcm2835_cprman: add the DSI0HSCK multiplexer >> I see it is similar to device_cold_reset(), but TBH I'm scared >> to be the first one using it. For what it's worth, Damien further explained the two helpers in docs/devel/reset.rst: For Devices and Buses, the following helper functions exist: - ``device_cold_reset()`` - ``bus_cold_reset()`` These are simple wrappers around resettable_reset() function; they only cast the Device or Bus into an Object and pass the cold reset type. When possible prefer to use these functions instead of ``resettable_reset()``. I figure what's missing is guidance on how to transition code from legacy reset to multi-phase reset. Ideally with a working example people can study. Damien, can you help us out?
On Wed, 28 Apr 2021 at 06:03, Markus Armbruster <armbru@redhat.com> wrote: > For what it's worth, Damien further explained the two helpers in > docs/devel/reset.rst: > > For Devices and Buses, the following helper functions exist: > > - ``device_cold_reset()`` > - ``bus_cold_reset()`` > > These are simple wrappers around resettable_reset() function; they only cast the > Device or Bus into an Object and pass the cold reset type. When possible > prefer to use these functions instead of ``resettable_reset()``. > > I figure what's missing is guidance on how to transition code from > legacy reset to multi-phase reset. Ideally with a working example > people can study. Damien, can you help us out? These are two separate things: 1) callsites which want to reset some other device, and currently mostly use eg device_legacy_reset() -- the transition to be done is to move them to use device_cold_reset(). Similarly, callsites which currently reset a bus with qbus_reset_all() and should move to bus_cold_reset(). 2) devices which implement reset and currently do so with a single reset method -- the transition to be done is to have them implement however many phases of 3-phase reset they need (1) is easier than (2) because there are many fewer callsites trying to manually reset devices or buses than there are devices that implement reset. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Wed, 28 Apr 2021 at 06:03, Markus Armbruster <armbru@redhat.com> wrote: >> For what it's worth, Damien further explained the two helpers in >> docs/devel/reset.rst: >> >> For Devices and Buses, the following helper functions exist: >> >> - ``device_cold_reset()`` >> - ``bus_cold_reset()`` >> >> These are simple wrappers around resettable_reset() function; they only cast the >> Device or Bus into an Object and pass the cold reset type. When possible >> prefer to use these functions instead of ``resettable_reset()``. >> >> I figure what's missing is guidance on how to transition code from >> legacy reset to multi-phase reset. Ideally with a working example >> people can study. Damien, can you help us out? > > These are two separate things: > 1) callsites which want to reset some other device, and currently > mostly use eg device_legacy_reset() -- the transition to be done > is to move them to use device_cold_reset(). Similarly, callsites > which currently reset a bus with qbus_reset_all() and should move > to bus_cold_reset(). > 2) devices which implement reset and currently do so with a > single reset method -- the transition to be done is to have > them implement however many phases of 3-phase reset they need > > (1) is easier than (2) because there are many fewer callsites > trying to manually reset devices or buses than there are > devices that implement reset. My ignorance on "these are two separate things" is further evidence that working examples are needed. Damien? Peter? Pretty-please?
On Wed, 28 Apr 2021 at 14:29, Markus Armbruster <armbru@redhat.com> wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > > These are two separate things: > > 1) callsites which want to reset some other device, and currently > > mostly use eg device_legacy_reset() -- the transition to be done > > is to move them to use device_cold_reset(). Similarly, callsites > > which currently reset a bus with qbus_reset_all() and should move > > to bus_cold_reset(). > > 2) devices which implement reset and currently do so with a > > single reset method -- the transition to be done is to have > > them implement however many phases of 3-phase reset they need > > > > (1) is easier than (2) because there are many fewer callsites > > trying to manually reset devices or buses than there are > > devices that implement reset. > > My ignorance on "these are two separate things" is further evidence that > working examples are needed. Damien? Peter? Pretty-please? On (1), see the other email thread we had a day or so ago. The conversion is mostly going to be "replace the device_legacy_reset call with device_cold_reset" but the tricky bit is checking that the device being reset doesn't have any qbuses attached, or if it does that we want them to be reset. On (2), there are a few examples of converted devices in the tree. thanks -- PMM
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 1b4b420617..b84fcc32bf 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -406,6 +406,13 @@ int qdev_walk_children(DeviceState *dev, qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, void *opaque); +/** + * @qdev_reset_all: + * Reset @dev. See @qbus_reset_all() for more details. + * + * Note: This function is deprecated and will be removed when it becomes unused. + * Please use device_cold_reset() now. + */ void qdev_reset_all(DeviceState *dev); void qdev_reset_all_fn(void *opaque); @@ -418,10 +425,28 @@ void qdev_reset_all_fn(void *opaque); * hard reset means that qbus_reset_all will reset all state of the device. * For PCI devices, for example, this will include the base address registers * or configuration space. + * + * Note: This function is deprecated and will be removed when it becomes unused. + * Please use bus_cold_reset() now. */ void qbus_reset_all(BusState *bus); void qbus_reset_all_fn(void *opaque); +/** + * device_cold_reset: + * Reset device @dev and perform a recursive processing using the resettable + * interface. It triggers a RESET_TYPE_COLD. + */ +void device_cold_reset(DeviceState *dev); + +/** + * bus_cold_reset: + * + * Reset bus @bus and perform a recursive processing using the resettable + * interface. It triggers a RESET_TYPE_COLD. + */ +void bus_cold_reset(BusState *bus); + /** * device_is_in_reset: * Return true if the device @dev is currently being reset. @@ -452,6 +477,8 @@ void qdev_machine_init(void); * device_legacy_reset: * * Reset a single device (by calling the reset method). + * Note: This function is deprecated and will be removed when it becomes unused. + * Please use device_cold_reset() now. */ void device_legacy_reset(DeviceState *dev); diff --git a/include/hw/resettable.h b/include/hw/resettable.h index 5e215d94e4..f4c4bab0ef 100644 --- a/include/hw/resettable.h +++ b/include/hw/resettable.h @@ -221,6 +221,15 @@ bool resettable_is_in_reset(Object *obj); */ void resettable_change_parent(Object *obj, Object *newp, Object *oldp); +/** + * resettable_cold_reset_fn: + * Helper to call resettable_reset((Object *) opaque, RESET_TYPE_COLD). + * + * This function is typically useful to register a reset handler with + * qemu_register_reset. + */ +void resettable_cold_reset_fn(void *opaque); + /** * resettable_class_set_parent_phases: * diff --git a/hw/core/bus.c b/hw/core/bus.c index 2698f715bd..3dc0a825f0 100644 --- a/hw/core/bus.c +++ b/hw/core/bus.c @@ -68,6 +68,11 @@ int qbus_walk_children(BusState *bus, return 0; } +void bus_cold_reset(BusState *bus) +{ + resettable_reset(OBJECT(bus), RESET_TYPE_COLD); +} + bool bus_is_in_reset(BusState *bus) { return resettable_is_in_reset(OBJECT(bus)); diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 36bfcbc697..7e6017fb3d 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -361,6 +361,11 @@ void qbus_reset_all_fn(void *opaque) qbus_reset_all(bus); } +void device_cold_reset(DeviceState *dev) +{ + resettable_reset(OBJECT(dev), RESET_TYPE_COLD); +} + bool device_is_in_reset(DeviceState *dev) { return resettable_is_in_reset(OBJECT(dev)); diff --git a/hw/core/resettable.c b/hw/core/resettable.c index 6e0b0f492f..96a99ce39e 100644 --- a/hw/core/resettable.c +++ b/hw/core/resettable.c @@ -264,6 +264,11 @@ void resettable_change_parent(Object *obj, Object *newp, Object *oldp) } } +void resettable_cold_reset_fn(void *opaque) +{ + resettable_reset((Object *) opaque, RESET_TYPE_COLD); +} + void resettable_class_set_parent_phases(ResettableClass *rc, ResettableEnterPhase enter, ResettableHoldPhase hold,