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

[Markus Armbruster]

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>
> ---
>  qapi/qdev.json       | 62 ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/pci/pci.h | 15 +++++++++++
>  hw/pci/pci.c         | 33 +++++++++++++++++++++++
>  hw/pci/pcie.c        | 49 ++++++++++++++++++++++++++++++++++
>  hw/pci/shpc.c        | 42 ++++++++++++++++++++++++++++++
>  5 files changed, 201 insertions(+)
>
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 2708fb4e99..40dc34f091 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -158,3 +158,65 @@
>  ##
>  { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>    'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @HotplugLedState:
> +#

No documentation?

> +# Since: 8.0
> +##
> +{ 'enum': 'HotplugLedState',
> +  'data': [ 'on', 'blink', 'off' ] }
> +
> +##
> +# @HotplugPowerState:

No documentation?

> +#
> +# Since: 8.0
> +##
> +{ 'enum': 'HotplugPowerState',
> +  'data': [ 'on', 'off' ] }

Why not bool?

> +##
> +# @HotplugSlotState:

No documentation?

> +#
> +# Since: 8.0
> +##
> +{ 'enum': 'HotplugSlotState',
> +  'data': [ 'power-only', 'enabled', 'disabled' ] }
> +
> +##
> +# @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.

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?

What's a @hotplug-slot?

> +
> +##
> +# @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?

> +# Only changed values are included into event.

"in the event"

Which values are included for each event trigger?

> +# 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?

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

[...]