Message-ID: <DM6PR11MB465709F2E30586AFCCE1461E9BBD9@DM6PR11MB4657.namprd11.prod.outlook.com>
Date:   Fri, 17 Mar 2023 18:22:46 +0000
From:   "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To:     Jiri Pirko <jiri@...nulli.us>
CC:     Vadim Fedorenko <vadfed@...a.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
        poros <poros@...hat.com>, mschmidt <mschmidt@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
        "Michalik, Michal" <michal.michalik@...el.com>
Subject: RE: [PATCH RFC v6 1/6] dpll: spec: Add Netlink spec in YAML

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Friday, March 17, 2023 5:21 PM
>
>Fri, Mar 17, 2023 at 04:14:45PM CET, arkadiusz.kubalewski@...el.com wrote:
>>
>>>From: Jiri Pirko <jiri@...nulli.us>
>>>Sent: Friday, March 17, 2023 11:05 AM
>>>
>>>Fri, Mar 17, 2023 at 01:52:44AM CET, arkadiusz.kubalewski@...el.com
>>>wrote:
>>>>>From: Jiri Pirko <jiri@...nulli.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.

Powered by blists - more mailing lists