| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <DM6PR11MB4657F48649CFEB92D28D4ED49BBD9@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Fri, 17 Mar 2023 15:14:45 +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 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.
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?
>
>>
>>>
>>>>
>>>>- 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.
>
>Make sure you don't nest the pin objects for dump (DPLL_A_PIN). Same
>reason as above.
>I don't see a need for DPLL_A_PIN attr existence, please remove it.
>
>
Sure, will try.
>
>
>>
>>[...]
>>
>>>>
>>>>>
>>>>>
>>>>>>+ -
>>>>>>+ 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.
>
>I'm not talking about per device specific items for pins (state and
>prio). That is something else, it's a pin-device tuple. Completely fine.
>
>
>
>>Same as a dpll without pins, right?
>>
>>'do' of DEVICE_GET could just dump it's own status, without the list of
>pins,
>
>Yes please.
>
>
>>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?
>
>No, definitelly not. Please make the object separation clear. Device and
>pins are different objects, they have different commands to work with.
>Don't mix them together.
>
It does, since the user app wouldn't have to dump/get pins continuously.
But yeah, as object separation argument makes sense will try to fix it.
>
>>
>>>
>>>>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?
>
>No, please extend the docs. As I wrote above, could you add some
>examples, like the one you pasted above. Examples always help to
>undestand things much better.
>
Sure, fixed.
>
>>
>>
>>>
>>>>
>>>>
>>>>>
>>>>>>+ 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?
>
>For this, yes. Some small doc note here would be quite convenient.
>
>Also, I almost forgot: Please don't use NLA_U32 for caps flags. Please
>use NLA_BITFIELD32 which was introduced for exactly this purpose. Allows
>to do nicer validation as well.
>
Actually BITFIELD32 is to much for this case, the bit itself would be enough
as we don't need validity bit.
But yeah, as there is no BITMASK yet, will try to change to BITFIELD32.
[...]
>>>>>
>>>>>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 |
|------------++--------------------------------|
|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