netdev - RE: [RFC PATCH v7 0/8] Create common DPLL configuration API

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <DM6PR11MB46572080791FCA02107289549B479@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Fri, 26 May 2023 10:14:00 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>, Vadim Fedorenko <vadfed@...a.com>
CC: Jakub Kicinski <kuba@...nel.org>, Jonathan Lemon
	<jonathan.lemon@...il.com>, Paolo Abeni <pabeni@...hat.com>, "Olech, Milena"
	<milena.olech@...el.com>, "Michalik, Michal" <michal.michalik@...el.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, Vadim Fedorenko
	<vadim.fedorenko@...ux.dev>, poros <poros@...hat.com>, mschmidt
	<mschmidt@...hat.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>
Subject: RE: [RFC PATCH v7 0/8] Create common DPLL configuration API

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Wednesday, May 17, 2023 12:19 PM
>
>Let me summarize the outcome of the discussion between me and Jakub
>regarding attributes, handles and ID lookups in the RFCv7 thread:
>
>--------------------------------------------------------------
>** Needed changes for RFCv8 **
>
>1) No scoped indexes.
>   The indexes passed from driver to dpll core during call of:
>        dpll_device_get() - device_idx
>        dpll_pin_get() - pin_idx
>   should be for INTERNAL kernel use only and NOT EXPOSED over uapi.
>   Therefore following attributes need to be removed:
>   DPLL_A_PIN_IDX
>   DPLL_A_PIN_PARENT_IDX
>

Seems doable.
So just to be clear, configuring a pin-pair (MUXed pins) will now be done
with DPLL_A_PIN_PARENT nested attribute.
I.e. configuring state of pin on parent:
DPLL_CMD_PIN_SET
	DPLL_A_PIN_ID		(id of pin being configured)
	DPLL_A_PIN_PARENT	(nested)
		DPLL_A_PIN_ID	(id of parent pin)
		DPLL_A_PIN_STATE(expected state)
		...		(other pin-pair attributes to be set)

Is that ok, or we need separated attribute like DPLL_A_PIN_PARENT_ID??
I think there is no need for separated one, documentation shall just reflect
that.
Also we have nested attribute DPLL_A_DEVICE which is used to show connections
between PIN and multiple dpll devices, to make it consistent I will rename
it to `DPLL_A_DEVICE_PARENT` and make configuration set cmd for the pin-dpll
pair similar to the above:
DPLL_CMD_PIN_SET
	DPLL_A_PIN_ID		(id of pin being configured)
	DPLL_A_DEVICE_PARENT	(nested)
		DPLL_A_ID	(id of parent dpll)
		DPLL_A_PIN_STATE(expected state)
		...		(other pin-dpll attributes to be set)

Does it make sense?


>2) For device, the handle will be DPLL_A_ID == dpll->id.
>   This will be the only handle for device for every
>   device related GET, SET command and every device related notification.
>   Note: this ID is not deterministing and may be different depending on
>   order of device probes etc.
>

Seems doable.

>3) For pin, the handle will be DPLL_A_PIN_ID == pin->id
>   This will be the only handle for pin for every
>   pin related GET, SET command and every pin related notification.
>   Note: this ID is not deterministing and may be different depending on
>   order of device probes etc.
>

Seems doable.

>4) Remove attribute:
>   DPLL_A_PIN_LABEL
>   and replace it with:
>   DPLL_A_PIN_PANEL_LABEL (string)
>   DPLL_A_PIN_XXX (string)
>   where XXX is a label type, like for example:
>     DPLL_A_PIN_BOARD_LABEL
>     DPLL_A_PIN_BOARD_TRACE
>     DPLL_A_PIN_PACKAGE_PIN
>

Sorry, I don't get this idea, what are those types?
What are they for?

>5) Make sure you expose following attributes for every device and
>   pin GET/DUMP command reply message:
>   DPLL_A_MODULE_NAME
>   DPLL_A_CLOCK_ID
>

Seems doable.

>6) Remove attributes:
>   DPLL_A_DEV_NAME
>   DPLL_A_BUS_NAME
>   as they no longer have any value and do no make sense (even in RFCv7)
>

Seems doable.

>
>--------------------------------------------------------------
>** Lookup commands **
>
>Basically these would allow user to query DEVICE_ID and PIN_ID
>according to provided atributes (see examples below).
>
>These would be from my perspective optional for this patchsets.
>I believe we can do it as follow-up if needed. For example for mlx5
>I don't have usecase for it, since I can consistently get PIN_ID
>using RT netlink for given netdev. But I can imagine that for non-SyncE
>dpll driver this would make sense to have.
>
>1) Introduce CMD_GET_ID - query the kernel for a dpll device
>                          specified by given attrs
>   Example:
>   -> DPLL_A_MODULE_NAME
>      DPLL_A_CLOCK_ID
>      DPLL_A_TYPE
>   <- DPLL_A_ID
>   Example:
>   -> DPLL_A_MODULE_NAME
>      DPLL_A_CLOCK_ID
>   <- DPLL_A_ID
>   Example:
>   -> DPLL_A_MODULE_NAME
>   <- -EINVAL (Extack: "multiple devices matched")
>
>   If user passes a subset of attrs which would not result in
>   a single match, kernel returns -EINVAL and proper extack message.
>

Seems ok.

>2) Introduce CMD_GET_PIN_ID - query the kernel for a dpll pin
>                              specified by given attrs
>   Example:
>   -> DPLL_A_MODULE_NAME
>      DPLL_A_CLOCK_ID
>      DPLL_A_PIN_TYPE
>      DPLL_A_PIN_PANEL_LABEL
>   <- DPLL_A_PIN_ID
>   Example:
>   -> DPLL_A_MODULE_NAME
>      DPLL_A_CLOCK_ID
>   <- DPLL_A_PIN_ID    (There was only one pin for given module/clock_id)
>   Example:
>   -> DPLL_A_MODULE_NAME
>      DPLL_A_CLOCK_ID
>   <- -EINVAL (Extack: "multiple pins matched")
>
>   If user passes a subset of attrs which would not result in
>   a single match, kernel returns -EINVAL and proper extack message.


Seems ok.

Will try to implement those now.

Thank you,
Arkadiusz