Hi All,
Do you think I should respin with the sugestions made by David so far ?
+ reset type removal
+ s/init/enter/ for the phases terminology
+ handling of parent changes during reset
On 9/18/19 11:11 AM, David Gibson wrote:
> On Wed, Sep 11, 2019 at 04:56:13PM +0200, Damien Hedde wrote:
>>
>>
>> On 9/11/19 10:06 AM, David Gibson wrote:
>>> On Wed, Aug 21, 2019 at 06:33:33PM +0200, Damien Hedde wrote:
>>>> This commit defines an interface allowing multi-phase reset.
>>>
>>> So, I certainly prefer the more general "reset type" approach taken in
>>> this version. That said, I find it pretty hard to imagine what types
>>> of reset other than cold will exist that have well enough defined
>>> semantics to be meaningfully used from an external subsystem.
>>
>> Maybe I should completely remove the type then ?
>
> That makes sense to me. I don't know if other possible users of the
> mechanism have different opinions though.
>
>>>
>>>> +static void resettable_init_reset(Object *obj, ResetType type)
>>>
>>> I wonder if "enter reset" would be better terminology so this doesn't
>>> get confused with the initial, well, initialization of the device.
>>
>> Do you mean for the function here or in general for the name of the phase ?
>
> In general.
>
>>>> + /*
>>>> + * we could assert that count > 0 but there are some corner cases
>>>> + * where we prefer to let it go as it is probably harmless.
>>>> + * For example: if there is reset support addition between
>>>> + * hosts when doing a migration. We may do such things as
>>>> + * deassert a non-existing reset.
>>>> + */
>>>> + if (s->count > 0) {
>>>> + s->count -= 1;
>>>> + } else {
>>>> + trace_resettable_count_underflow(obj);
>>>
>>> Should this be an assert(), IIUC this could only come about from an
>>> error within the qemu code, right?
>>
>> Initially I was thinking that so I put an assert.
>>
>> But while testing I found out that it is triggered by the raspi machine
>> during its reset because the qbus tree is modified during it.
>>
>> So it depends if we consider this kind of action to be faulty. With no
>> migration support, the only way to trigger it is to modify the qdev
>> hierarchy during reset.
>
> Hm, I see. It feels like just ignoring underflow is ignoring the
> error rather than really addressing it. When we add a device to the
> heirarchy, do we need to initialize its reset count based on its
> parent's current count or something.
>
I can add that.
Thanks,
--
Damien
