Re: [PATCH v3 13/15] qapi: add HOTPLUG_STATE event

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 10.02.23 13:23, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> 
>>> For PCIe and SHPC hotplug it's important to track led indicators,
>>> especially the power led. Add an event that helps.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> ---

[...]

>>> +##
>>> +# @HotplugState:
>>> +#
>>> +# @hotplug-device: hotplug device id
>>> +# @hotplug-path: hotplug device path
>>> +# @hotplug-slot: hotplug device slot (only for SHPC)
>>> +# @device: device name
>>> +# @path: device path
>>> +# @power-led: Power Indicator
>>> +# @attention-led: Attention Indicator
>>> +# @state: slot state, only for SHPC hotplug controller
>>> +# @power: Power Controller state, only for PCIe hotplug
>> 
>> 
>> 
>>> +#
>>> +# Since: 8.0
>>> +##
>>> +{ 'struct': 'HotplugState',
>>> +  'data': { '*hotplug-device': 'str',
>>> +            'hotplug-path': 'str',
>>> +            '*hotplug-slot': 'int',
>>> +            '*device': 'str',
>>> +            'path': 'str',
>>> +            '*power-led': 'HotplugLedState',
>>> +            '*attention-led': 'HotplugLedState',
>>> +            '*state': 'HotplugSlotState',
>>> +            '*power': 'HotplugPowerState' } }
>> 
>> Too terse.
>
> Will fix)
>
>> 
>> What do @hotplug-device and @device name?  Are these qdev-id?
>> 
>> What kind of paths are @hotplug-path and @path?  Are these paths to an
>> object device in the QOM tree?  Which object?
>
> device / path is same name and path as for DEVICE_DELETED

Got it.  But there we have just one device, and here we have two.  Which
two?

Also, DEVICE_DELETED's doc comment is better:

    # @device: the device's ID if it has one
    #
    # @path: the device's QOM path

Suggest to steal from there.

>> What's a @hotplug-slot?
>
> pci slot. Significant for SHPC
>
>> 
>>> +
>>> +##
>>> +# @HOTPLUG_STATE:
>>> +#
>>> +# Emitted whenever the state of hotplug controller is changed.
>> 
>> Suggest "the state of hotplug controller changes."
>> 
>> Regardless, too terse.  What state changes exactly trigger the event?
>
> Any change of power-led / attention-led / state / power.
>
> Will add a description
>
>> 
>>> +# Only changed values are included into event.
>> 
>> "in the event"
>> 
>> Which values are included for each event trigger?
>
> - device ids and names always included
> - power-led / attention-led / state / power  - only those who changed
>
>> 
>>> +# Only SHPC and PCIe-native hotplug are supported.
>> 
>> Suggest something like "only ... provide this event."
>> 
>> Are parts of HotplugState specific to "SHPC and PCIe-native"?  Or asked
>> differently: when we make other kinds of hotplug send the event, what
>> would we need to change here?
>
> Hmm. Looks like I'd better use a union with type discriminator. This way 
> we'll be able to add any other hotplug later.
>
> (and even now it's better, as not all 4 state fields are shared for PCIe and 
> SHPC)

A union feels like the way to go.

>>> +#
>>> +# Since: 8.0
>>> +##
>>> +{ 'event': 'HOTPLUG_STATE',
>>> +  'data': 'HotplugState' }
>> 
>> [...]