Re: [PATCH v4 02/10] hw/core: create Resettable QOM interface

[Damien Hedde]

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