Fri, Mar 17, 2023 at 07:22:46PM CET, arkadiusz.kubalewski@intel.com wrote:

quoted

From: Jiri Pirko <jiri@resnulli.us>
Sent: Friday, March 17, 2023 5:21 PM

Fri, Mar 17, 2023 at 04:14:45PM CET, arkadiusz.kubalewski@intel.com wrote:

quoted

quoted

From: Jiri Pirko <jiri@resnulli.us>
Sent: Friday, March 17, 2023 11:05 AM

Fri, Mar 17, 2023 at 01:52:44AM CET, arkadiusz.kubalewski@intel.com
wrote:

quoted

quoted

From: Jiri Pirko <jiri@resnulli.us>
Sent: Thursday, March 16, 2023 2:45 PM

[...]

quoted

quoted

quoted

quoted

+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

quoted

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.

quoted

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?

I thought it is obvious. With what you suggest, each generic netlink
user would have to have all commands that implement both do and dump
with message format of a root nest. Not only that, for the sake of
consistency, all the rest of the commands would have to implement same
root nesting format. Both ways, to and from kernel.
So no matter what, all the messages would look like:

CMD_X MSG:
  SUBSYS_ATTR_ROOT
    SUBSYS_ATTR_1
    SUBSYS_ATTR_2
    ***
    SUBSYS_ATTR_N

What I say, this extra level of nesting is not needed and in fact it is
not good for anything. Remove it and just have:

CMD_X MSG:
  SUBSYS_ATTR_1
  SUBSYS_ATTR_2
  ***
  SUBSYS_ATTR_N

Clear and simple. For dump, you repeat the header, I don't see why that
is problem. All implementations are doing that, all userspace apps are
used to it. It is simple and consistent even if you consider multiple
skbs used for dump. I'm lacking to understand your need to reinvent
the wheel here.

quoted

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.

[...]

quoted

quoted

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

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.

quoted

"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.

Sure.

quoted

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?

To mix them together in a single attr. But I'm fine with that.

What do you mean by "sanitize the user/driver set of this attr in dpll code"?

What I mean is that in each mode there has to be clearly documented
which entity changes state, how and under which circumstance. This
behaviour needs to be enforced in the dpll code.

Thank you,
Arkadiusz

quoted

quoted

|------------++--------------------------------|
|MANUAL      ||- CONNECTED    | - CONNECTED    |
|            ||- DISCONNECTED | - DISCONNECTED |
+------------++---------------+----------------+

Thank you,
Arkadiusz

quoted

quoted

[...]

quoted

quoted

quoted

quoted

+
+/* DPLL_CMD_DEVICE_SET - do */
+static const struct nla_policy

dpll_device_set_nl_policy[DPLL_A_MODE +

quoted

quoted

quoted

quoted

quoted

quoted

quoted

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 :)


[...]

quoted

quoted

quoted

quoted

+	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.