[PATCH RFC v6 1/6] dpll: spec: Add Netlink spec in YAML
Kubalewski, Arkadiusz
arkadiusz.kubalewski at intel.com
Thu Mar 16 17:52:44 PDT 2023
>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.
>
>>
>>- do/dump of pin-get, in case of shared pins, each pin contains number of
>dpll
>>handles it connects with:
>>[{'pin': [{'device': [{'bus-name': 'pci',
>> 'dev-name': '0000:21:00.0_0',
>> 'id': 0,
>> 'pin-prio': 6,
>> 'pin-state': {'doc': 'pin connected',
>> 'name': 'connected'}},
>> {'bus-name': 'pci',
>> 'dev-name': '0000:21:00.0_1',
>> 'id': 1,
>> 'pin-prio': 8,
>> 'pin-state': {'doc': 'pin connected',
>> 'name': 'connected'}}],
>
>Okay, here I understand it contains device specific pin items. Makes
>sense!
>
Good.
[...]
>>
>>>
>>>
>>>>+ -
>>>>+ name: pin-prio
>>>>+ type: u32
>>>>+ -
>>>>+ name: pin-state
>>>>+ type: u8
>>>>+ enum: pin-state
>>>>+ -
>>>>+ name: pin-parent
>>>>+ type: nest
>>>>+ multi-attr: true
>>>>+ nested-attributes: pin
>>>>+ value: 23
>>>
>>>Value 23? What's this?
>>>You have it specified for some attrs all over the place.
>>>What is the reason for it?
>>>
>>
>>Actually this particular one is not needed (also value: 12 on pin above),
>>I will remove those.
>>But the others you are refering to (the ones in nested attribute list),
>>are required because of cli.py parser issue, maybe Kuba knows a better way
>to
>>prevent the issue?
>>Basically, without those values, cli.py brakes on parsing responses, after
>>every "jump" to nested attribute list it is assigning first attribute
>there
>>with value=0, thus there is a need to assign a proper value, same as it is
>on
>>'main' attribute list.
>
>That's weird. Looks like a bug then?
>
Guess we could call it a bug, I haven't investigated the parser that much,
AFAIR, other specs are doing the same way.
>
>>
>>>
>>>>+ -
>>>>+ name: pin-parent-idx
>>>>+ type: u32
>>>>+ -
>>>>+ name: pin-rclk-device
>>>>+ type: string
>>>>+ -
>>>>+ name: pin-dpll-caps
>>>>+ type: u32
>>>>+ -
>>>>+ name: device
>>>>+ subset-of: dpll
>>>>+ attributes:
>>>>+ -
>>>>+ name: id
>>>>+ type: u32
>>>>+ value: 2
>>>>+ -
>>>>+ name: dev-name
>>>>+ type: string
>>>>+ -
>>>>+ name: bus-name
>>>>+ type: string
>>>>+ -
>>>>+ name: mode
>>>>+ type: u8
>>>>+ enum: mode
>>>>+ -
>>>>+ name: mode-supported
>>>>+ type: u8
>>>>+ enum: mode
>>>>+ multi-attr: true
>>>>+ -
>>>>+ name: source-pin-idx
>>>>+ type: u32
>>>>+ -
>>>>+ name: lock-status
>>>>+ type: u8
>>>>+ enum: lock-status
>>>>+ -
>>>>+ name: temp
>>>>+ type: s32
>>>>+ -
>>>>+ name: clock-id
>>>>+ type: u64
>>>>+ -
>>>>+ name: type
>>>>+ type: u8
>>>>+ enum: type
>>>>+ -
>>>>+ name: pin
>>>>+ type: nest
>>>>+ value: 12
>>>>+ multi-attr: true
>>>>+ nested-attributes: pin
>>>
>>>This does not belong here.
>>>
>>
>>What do you mean?
>>With device-get 'do' request the list of pins connected to the dpll is
>>returned, each pin is nested in this attribute.
>
>No, wait a sec. You have 2 object types: device and pin. Each have
>separate netlink CMDs to get and dump individual objects.
>Don't mix those together like this. I thought it became clear in the
>past. :/
>
For pins we must, as pins without a handle to a dpll are pointless.
Same as a dpll without pins, right?
'do' of DEVICE_GET could just dump it's own status, without the list of pins,
but it feels easier for handling it's state on userspace counterpart if that
command also returns currently registered pins. Don't you think so?
>
>>This is required by parser to work.
>>
>>>
>>>>+ -
>>>>+ name: pin-prio
>>>>+ type: u32
>>>>+ value: 21
>>>>+ -
>>>>+ name: pin-state
>>>>+ type: u8
>>>>+ enum: pin-state
>>>>+ -
>>>>+ name: pin-dpll-caps
>>>>+ type: u32
>>>>+ value: 26
>>>
>>>All these 3 do not belong here are well.
>>>
>>
>>Same as above explanation.
>
>Same as above reply.
>
>
>>
>>>
>>>
>>>>+ -
>>>>+ name: pin
>>>>+ subset-of: dpll
>>>>+ attributes:
>>>>+ -
>>>>+ name: device
>>>>+ type: nest
>>>>+ value: 1
>>>>+ multi-attr: true
>>>>+ nested-attributes: device
>>>>+ -
>>>>+ name: pin-idx
>>>>+ type: u32
>>>>+ value: 13
>>>>+ -
>>>>+ name: pin-description
>>>>+ type: string
>>>>+ -
>>>>+ name: pin-type
>>>>+ type: u8
>>>>+ enum: pin-type
>>>>+ -
>>>>+ name: pin-direction
>>>>+ type: u8
>>>>+ enum: pin-direction
>>>>+ -
>>>>+ name: pin-frequency
>>>>+ type: u32
>>>>+ -
>>>>+ name: pin-frequency-supported
>>>>+ type: u32
>>>>+ multi-attr: true
>>>>+ -
>>>>+ name: pin-any-frequency-min
>>>>+ type: u32
>>>>+ -
>>>>+ name: pin-any-frequency-max
>>>>+ type: u32
>>>>+ -
>>>>+ name: pin-prio
>>>>+ type: u32
>>>>+ -
>>>>+ name: pin-state
>>>>+ type: u8
>>>>+ enum: pin-state
>>>>+ -
>>>>+ name: pin-parent
>>>>+ type: nest
>>>>+ multi-attr: true
>>>
>>>Multiple parents? How is that supposed to work?
>>>
>>
>>As we have agreed, MUXed pins can have multiple parents.
>>In our case:
>>/tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --do
>>pin-get --json '{"id": 0, "pin-idx":13}'
>>{'pin': [{'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}],
>> 'pin-description': '0000:21:00.0',
>> 'pin-direction': {'doc': 'pin used as a source of a signal',
>> 'name': 'source'},
>> 'pin-idx': 13,
>> 'pin-parent': [{'pin-parent-idx': 2,
>> 'pin-state': {'doc': 'pin disconnected',
>> 'name': 'disconnected'}},
>> {'pin-parent-idx': 3,
>> 'pin-state': {'doc': 'pin disconnected',
>> 'name': 'disconnected'}}],
>> 'pin-rclk-device': '0000:21:00.0',
>> 'pin-type': {'doc': "ethernet port PHY's recovered clock",
>> 'name': 'synce-eth-port'}}]}
>
>Got it, it is still a bit hard to me to follow this. Could you
>perhaps extend the Documentation to describe in more details
>with examples? Would help a lot for slower people like me to understand
>what's what.
>
Actually this is already explained in "MUX-type pins" paragraph of
Documentation/networking/dpll.rst.
Do we want to duplicate this explanation here?
>
>>
>>
>>>
>>>>+ nested-attributes: pin-parent
>>>>+ value: 23
>>>>+ -
>>>>+ name: pin-rclk-device
>>>>+ type: string
>>>>+ value: 25
>>>>+ -
>>>>+ name: pin-dpll-caps
>>>>+ type: u32
>>>
>>>Missing "enum: "
>>>
>>
>>It is actually a bitmask, this is why didn't set as enum, with enum type
>>parser won't parse it.
>
>Ah! Got it. Perhaps a docs note with the enum pointer then?
>
Same as above, explained in Documentation/networking/dpll.rst, do wan't to
duplicate?
>
>>
>>>
>>>>+ -
>>>>+ name: pin-parent
>>>>+ subset-of: dpll
>>>>+ attributes:
>>>>+ -
>>>>+ name: pin-state
>>>>+ type: u8
>>>>+ value: 22
>>>>+ enum: pin-state
>>>>+ -
>>>>+ name: pin-parent-idx
>>>>+ type: u32
>>>>+ value: 24
>>>>+ -
>>>>+ name: pin-rclk-device
>>>>+ type: string
>>>
>>>Yeah, as I wrote in the other email, this really smells to
>>>have like a simple string like this. What is it supposed to be?
>>>
>>
>>Yes, let's discuss there.
>
>Yep.
>
>>
>>>
>>>>+
>>>>+
>>>>+operations:
>>>>+ list:
>>>>+ -
>>>>+ name: unspec
>>>>+ doc: unused
>>>>+
>>>>+ -
>>>>+ name: device-get
>>>>+ doc: |
>>>>+ Get list of DPLL devices (dump) or attributes of a single dpll
>>>device
>>>>+ attribute-set: dpll
>>>
>>>Shouldn't this be "device"?
>>>
>>
>>It would brake the parser, again I hope Jakub Kicinski could take a look
>>on this.
>
>Odd.
>
Yes, seems a bit odd.
>>
>>>
>>>>+ flags: [ admin-perm ]
>>>>+
>>>>+ do:
>>>>+ pre: dpll-pre-doit
>>>>+ post: dpll-post-doit
>>>>+ request:
>>>>+ attributes:
>>>>+ - id
>>>>+ - bus-name
>>>>+ - dev-name
>>>>+ reply:
>>>>+ attributes:
>>>>+ - device
>>>>+
>>>>+ dump:
>>>>+ pre: dpll-pre-dumpit
>>>>+ post: dpll-post-dumpit
>>>>+ reply:
>>>>+ attributes:
>>>>+ - device
>>>>+
>>>>+ -
>>>>+ name: device-set
>>>>+ doc: Set attributes for a DPLL device
>>>>+ attribute-set: dpll
>>>
>>>"device" here as well?
>>>
>>
>>Same as above.
>>
>>>
>>>>+ flags: [ admin-perm ]
>>>>+
>>>>+ do:
>>>>+ pre: dpll-pre-doit
>>>>+ post: dpll-post-doit
>>>>+ request:
>>>>+ attributes:
>>>>+ - id
>>>>+ - bus-name
>>>>+ - dev-name
>>>>+ - mode
>>>
>>>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.
[...]
>>>>+
>>>>+/* 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! :)
More information about the linux-arm-kernel
mailing list