| 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
| ||
|
Date: Fri, 17 Mar 2023 17:20:32 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
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
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.
The reason might be that the userspace is parsing exactly the same
message as if it would be DOIT 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.
Cool.
>
>>
>>
>>>
>>>[...]
>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>+ -
>>>>>>>+ 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.
I don't follow. For every pin change there is going to be pin object
dumped over monitoring netlink.
>But yeah, as object separation argument makes sense will try to fix it.
Awesome.
>
>>
>>>
>>>>
>>>>>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 |
"selectable" is something the user sets.
"connected"/"disconnected" is in case of auto mode something that driver
sets.
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.
>|------------++--------------------------------|
>|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