[PATCH RFC v6 1/6] dpll: spec: Add Netlink spec in YAML
Kubalewski, Arkadiusz
arkadiusz.kubalewski at intel.com
Fri Mar 17 11:22:46 PDT 2023
>From: Jiri Pirko <jiri at resnulli.us>
>Sent: Friday, March 17, 2023 5:21 PM
>
>Fri, Mar 17, 2023 at 04:14:45PM CET, arkadiusz.kubalewski at intel.com wrote:
>>
>>>From: Jiri Pirko <jiri at resnulli.us>
>>>Sent: Friday, March 17, 2023 11:05 AM
>>>
>>>Fri, Mar 17, 2023 at 01:52:44AM CET, arkadiusz.kubalewski at intel.com
>>>wrote:
>>>>>From: Jiri Pirko <jiri at resnulli.us>
>>>>>Sent: Thursday, March 16, 2023 2:45 PM
>>>>>
>>>>
>>>>[...]
>>>>
>>>>>>>>+attribute-sets:
>>>>>>>>+ -
>>>>>>>>+ name: dpll
>>>>>>>>+ enum-name: dplla
>>>>>>>>+ attributes:
>>>>>>>>+ -
>>>>>>>>+ name: device
>>>>>>>>+ type: nest
>>>>>>>>+ value: 1
>>>>>>>>+ multi-attr: true
>>>>>>>>+ nested-attributes: device
>>>>>>>
>>>>>>>What is this "device" and what is it good for? Smells like some
>>>>>>>leftover
>>>>>>>and with the nested scheme looks quite odd.
>>>>>>>
>>>>>>
>>>>>>No, it is nested attribute type, used when multiple devices are returned
>>>>>>with netlink:
>>>>>>
>>>>>>- dump of device-get command where all devices are returned, each one
>>>>>>nested
>>>>>>inside it:
>>>>>>[{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0', 'id':0},
>>>>>> {'bus-name': 'pci', 'dev-name': '0000:21:00.0_1', 'id':1}]}]
>>>>>
>>>>>Okay, why is it nested here? The is one netlink msg per dpll device
>>>>>instance. Is this the real output of you made that up?
>>>>>
>>>>>Device nest should not be there for DEVICE_GET, does not make sense.
>>>>>
>>>>
>>>>This was returned by CLI parser on ice with cmd:
>>>>$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml /
>>>>--dump device-get
>>>>
>>>>Please note this relates to 'dump' request , it is rather expected that
>>>there
>>>>are multiple dplls returned, thus we need a nest attribute for each one.
>>>
>>>No, you definitelly don't need to nest them. Dump format and get format
>>>should be exactly the same. Please remove the nest.
>>>
>>>See how that is done in devlink for example: devlink_nl_fill()
>>>This functions fills up one object in the dump. No nesting.
>>>I'm not aware of such nesting approach anywhere in kernel dumps, does
>>>not make sense at all.
>>>
>>
>>Yeah it make sense to have same output on `do` and `dump`, but this is also
>>achievable with nest DPLL_A_DEVICE, still don't need put extra header for it.
>>The difference would be that on `dump` multiple DPLL_A_DEVICE are provided,
>>on `do` only one.
>
>Please don't. This root nesting is not correct.
>
>
>>
>>Will try to fix it.
>>Although could you please explain why it make sense to put extra header
>>(exactly the same header) multiple times in one netlink response message?
>
>This is how it's done for all netlink dumps as far as I know.
So we just following something but we cannot explain why?
>The reason might be that the userspace is parsing exactly the same
>message as if it would be DOIT message.
>
This argument is achievable on both approaches.
[...]
>>>>>>>
>>>>>>>Hmm, shouldn't source-pin-index be here as well?
>>>>>>
>>>>>>No, there is no set for this.
>>>>>>For manual mode user selects the pin by setting enabled state on the
>one
>>>>>>he needs to recover signal from.
>>>>>>
>>>>>>source-pin-index is read only, returns active source.
>>>>>
>>>>>Okay, got it. Then why do we have this assymetric approach? Just have
>>>>>the enabled state to serve the user to see which one is selected, no?
>>>>>This would help to avoid confusion (like mine) and allow not to create
>>>>>inconsistencies (like no pin enabled yet driver to return some source
>>>>>pin index)
>>>>>
>>>>
>>>>This is due to automatic mode were multiple pins are enabled, but actual
>>>>selection is done on hardware level with priorities.
>>>
>>>Okay, this is confusing and I believe wrong.
>>>You have dual meaning for pin state attribute with states
>>>STATE_CONNECTED/DISCONNECTED:
>>>
>>>1) Manual mode, MUX pins (both share the same model):
>>> There is only one pin with STATE_CONNECTED. The others are in
>>> STATE_DISCONNECTED
>>> User changes a state of a pin to make the selection.
>>>
>>> Example:
>>> $ dplltool pin dump
>>> pin 1 state connected
>>> pin 2 state disconnected
>>> $ dplltool pin 2 set state connected
>>> $ dplltool pin dump
>>> pin 1 state disconnected
>>> pin 2 state connected
>>>
>>>2) Automatic mode:
>>> The user by setting "state" decides it the pin should be considered
>>> by the device for auto selection.
>>>
>>> Example:
>>> $ dplltool pin dump:
>>> pin 1 state connected prio 10
>>> pin 2 state connected prio 15
>>> $ dplltool dpll x get:
>>> dpll x source-pin-index 1
>>>
>>>So in manual mode, STATE_CONNECTED means the dpll is connected to this
>>>source pin. However, in automatic mode it means something else. It means
>>>the user allows this pin to be considered for auto selection. The fact
>>>the pin is selected source is exposed over source-pin-index.
>>>
>>>Instead of this, I believe that the semantics of
>>>STATE_CONNECTED/DISCONNECTED should be the same for automatic mode as
>>>well. Unlike the manual mode/mux, where the state is written by user, in
>>>automatic mode the state should be only written by the driver. User
>>>attemts to set the state should fail with graceful explanation (DPLL
>>>netlink/core code should handle that, w/o driver interaction)
>>>
>>>Suggested automatic mode example:
>>> $ dplltool pin dump:
>>> pin 1 state connected prio 10 connectable true
>>> pin 2 state disconnected prio 15 connectable true
>>> $ dplltool pin 1 set connectable false
>>> $ dplltool pin dump:
>>> pin 1 state disconnected prio 10 connectable false
>>> pin 2 state connected prio 15 connectable true
>>> $ dplltool pin 1 set state connected
>>> -EOPNOTSUPP
>>>
>>>Note there is no "source-pin-index" at all. Replaced by pin state here.
>>>There is a new attribute called "connectable", the user uses this
>>>attribute to tell the device, if this source pin could be considered for
>>>auto selection or not.
>>>
>>>Could be called perhaps "selectable", does not matter. The point is, the
>>>meaning of the "state" attribute is consistent for automatic mode,
>>>manual mode and mux pin.
>>>
>>>Makes sense?
>>>
>>
>>Great idea!
>>I will add third enum for pin-state: DPLL_PIN_STATE_SELECTABLE.
>>In the end we will have this:
>> +--------------------------------+
>> | valid DPLL_A_PIN_STATE values |
>> +---------------+----------------+
>>+------------+| requested: | returned: |
>>|DPLL_A_MODE:|| | |
>>|------------++--------------------------------|
>>|AUTOMATIC ||- SELECTABLE | - SELECTABLE |
>>| ||- DISCONNECTED | - DISCONNECTED |
>>| || | - CONNECTED |
>
>"selectable" is something the user sets.
Yes.
>"connected"/"disconnected" is in case of auto mode something that driver
>sets.
>
No. Not really.
"CONNECTED" is only set by driver once a pin is choosen.
"SELECTABLE" is set by the user if he needs to enable a pin for selection,
it is also default state of a pin if it was not selected ("CONNECTED")
"DISCONNECTED" is set by the user if he needs to disable a pin from selection.
>Looks a bit odd to mix them together. That is why I suggested
>to have sepectable as a separate attr. But up to you. Please make sure
>you sanitize the user/driver set of this attr in dpll code.
>
What is odd?
What do you mean by "sanitize the user/driver set of this attr in dpll code"?
Thank you,
Arkadiusz
>
>>|------------++--------------------------------|
>>|MANUAL ||- CONNECTED | - CONNECTED |
>>| ||- DISCONNECTED | - DISCONNECTED |
>>+------------++---------------+----------------+
>>
>>Thank you,
>>Arkadiusz
>>
>>>
>>>>
>>>>[...]
>>>>
>>>>>>>>+
>>>>>>>>+/* DPLL_CMD_DEVICE_SET - do */
>>>>>>>>+static const struct nla_policy
>dpll_device_set_nl_policy[DPLL_A_MODE +
>>>>>>>>1]
>>>>>>>>= {
>>>>>>>>+ [DPLL_A_ID] = { .type = NLA_U32, },
>>>>>>>>+ [DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>>>>>>>+ [DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>>>>>>>+ [DPLL_A_MODE] = NLA_POLICY_MAX(NLA_U8, 5),
>>>>>>>
>>>>>>>Hmm, any idea why the generator does not put define name
>>>>>>>here instead of "5"?
>>>>>>>
>>>>>>
>>>>>>Not really, it probably needs a fix for this.
>>>>>
>>>>>Yeah.
>>>>>
>>>>
>>>>Well, once we done with review maybe we could also fix those, or ask
>>>>Jakub if he could help :)
>>>>
>>>>
>>>>[...]
>>>>
>>>>>>>
>>>>>>>>+ DPLL_A_PIN_PRIO,
>>>>>>>>+ DPLL_A_PIN_STATE,
>>>>>>>>+ DPLL_A_PIN_PARENT,
>>>>>>>>+ DPLL_A_PIN_PARENT_IDX,
>>>>>>>>+ DPLL_A_PIN_RCLK_DEVICE,
>>>>>>>>+ DPLL_A_PIN_DPLL_CAPS,
>>>>>>>
>>>>>>>Just DPLL_A_PIN_CAPS is enough, that would be also consistent with the
>>>>>>>enum name.
>>>>>>
>>>>>>Sure, fixed.
>>>>>
>>>>>
>>>>>Thanks for all your work on this!
>>>>
>>>>Thanks for a great review! :)
>>>
>>>Glad to help.
More information about the linux-arm-kernel
mailing list