On Fri, 12 Apr 2024 at 14:38, Edgar E. Iglesias <edgar.igles...@gmail.com> wrote: > > On Fri, Apr 12, 2024 at 3:05 PM Peter Maydell <peter.mayd...@linaro.org> > wrote: > > > > On Thu, 11 Apr 2024 at 18:23, Philippe Mathieu-Daudé <phi...@linaro.org> > > wrote: > > > > > > On 11/4/24 15:43, Peter Maydell wrote: > > > > On Wed, 21 Aug 2019 at 17:34, Damien Hedde <damien.he...@greensocs.com> > > > > wrote: > > > >> > > > >> This commit defines an interface allowing multi-phase reset. This aims > > > >> to solve a problem of the actual single-phase reset (built in > > > >> DeviceClass and BusClass): reset behavior is dependent on the order > > > >> in which reset handlers are called. In particular doing external > > > >> side-effect (like setting an qemu_irq) is problematic because receiving > > > >> object may not be reset yet. > > > > > > > > So, I wanted to drag up this ancient patch to ask a couple > > > > of Resettable questions, because I'm working on adding a > > > > new ResetType (the equivalent of SHUTDOWN_CAUSE_SNAPSHOT_LOAD). > > > > > > > >> +/** > > > >> + * ResetType: > > > >> + * Types of reset. > > > >> + * > > > >> + * + Cold: reset resulting from a power cycle of the object. > > > >> + * > > > >> + * TODO: Support has to be added to handle more types. In particular, > > > >> + * ResetState structure needs to be expanded. > > > >> + */ > > > > > > > > Does anybody remember what this TODO comment is about? What > > > > in particular would need to be in the ResetState struct > > > > to allow another type to be added? > > > > > > IIRC this comes from this discussion: > > > https://lore.kernel.org/qemu-devel/7c193b33-8188-2cda-cbf2-fb5452544...@greensocs.com/ > > > Updated in this patch (see after '---' description): > > > https://lore.kernel.org/qemu-devel/20191018150630.31099-9-damien.he...@greensocs.com/ > > > > Hmm, I can't see anything in there that mentions this > > TODO or what we'd need more ResetState fields to handle. > > I guess I'll go ahead with adding my new ResetType and ignore > > this TODO, because I can't see any reason why we need to > > do anything in particular for a new ResetType... > > > > > > > > > >> +typedef enum ResetType { > > > >> + RESET_TYPE_COLD, > > > >> +} ResetType; > > > > > > > >> +typedef void (*ResettableInitPhase)(Object *obj, ResetType type); > > > >> +typedef void (*ResettableHoldPhase)(Object *obj); > > > >> +typedef void (*ResettableExitPhase)(Object *obj); > > > > > > > > Was there a reason why we only pass the ResetType to the init > > > > phase method, and not also to the hold and exit phases ? > > > > Given that many devices don't need to implement init, it > > > > seems awkward to require them to do so just to stash the > > > > ResetType somewhere so they can look at it in the hold > > > > or exit phase, so I was thinking about adding the argument > > > > to the other two phase methods. > > > > > > You are right, the type should be propagated to to all phase > > > handlers. > > > > I have some patches which do this; I'll probably send them out > > in a series next week once I've figured out whether they fit > > better in with other patches that give the motivation.
> I don't remember the details on your first questions but I also agree > with adding the type to the other callbacks! I've now posted the series that adds the type the the hold and exit callbacks, and adds a new RESET_TYPE_SNAPSHOT_LOAD: https://patchew.org/QEMU/20240412160809.1260625-1-peter.mayd...@linaro.org/ thanks -- PMM
