RE: [RFC PATCH v4 2/4] dpll: Add DPLL framework base functions

From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@intel.com>
To: Jiri Pirko <jiri@resnulli.us>, Jakub Kicinski <kuba@kernel.org>
Cc: Vadim Fedorenko <vfedorenko@novek.ru>,
	Jonathan Lemon <jonathan.lemon@gmail.com>,
	Paolo Abeni <pabeni@redhat.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Vadim Fedorenko <vadfed@fb.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
	"Olech, Milena" <milena.olech@intel.com>,
	"Michalik, Michal" <michal.michalik@intel.com>
Subject: RE: [RFC PATCH v4 2/4] dpll: Add DPLL framework base functions
Date: Tue, 13 Dec 2022 18:08:13 +0000	[thread overview]
Message-ID: <DM6PR11MB46577F9AB422103140778D529BE39@DM6PR11MB4657.namprd11.prod.outlook.com> (raw)
In-Reply-To: <Y5cucrZjsMgZcHDf@nanopsycho>

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Monday, December 12, 2022 2:37 PM
>To: Jakub Kicinski <kuba@kernel.org>
>
>Fri, Dec 09, 2022 at 05:19:42PM CET, kuba@kernel.org wrote:
>>On Fri, 9 Dec 2022 10:29:53 +0100 Jiri Pirko wrote:
>>> Thu, Dec 08, 2022 at 06:05:17PM CET, kuba@kernel.org wrote:
>>> >On Thu, 8 Dec 2022 17:33:28 +0100 Jiri Pirko wrote:
>>> >> For any synce pin manipulation over dpll netlink, we can use the
>>> >> netns check of the linked netdev. This is the netns aware leg of
>>> >> the dpll, it should be checked for.
>>> >
>>> >The OCP card is an atomic clock, it does not have any networking.
>>>
>>> Sure, so why it has to be netns aware if it has nothing to do with
>>> networking?
>>
>>That's a larger question, IDK if broadening the scope of the discussion
>>will help us reach a conclusion.
>>
>>The patchset as is uses network namespaces for permissions:
>>
>>+		.flags	= GENL_UNS_ADMIN_PERM,
>
>Yeah, I wonder if just GENL_ADMIN_PERM wuldn't be more suitable here...
>
>
>>
>>so that's what I'm commenting on - aligning visibility of objects with
>>already used permissions.
>>
>>> >> I can't imagine practically havind the whole dpll instance netns
>aware.
>>> >> Omitting the fact that it really has no meaning for non-synce
>>> >> pins, what would be the behaviour when for example pin 1 is in
>>> >> netns a, pin 2 in netns b and dpll itself in netns c?
>>> >
>>> >To be clear I don't think it's a bad idea in general, I've done the
>>> >same thing for my WIP PSP patches. But we already have one device
>>> >without netdevs, hence I thought maybe devlink. So maybe we do the
>>> >same thing with devlink? I mean - allow multiple devlink instances
>>> >to be linked and require caps on any of them?
>>>
>>> I read this 5 times, I'm lost, don't understand what you mean :/
>>
>>Sorry I was replying to both paragraphs here, sorry.
>>What I thought you suggested is we scope the DPLL to whatever the
>>linked netdevs are scoped to? If netns has any of the netdevs attached
>>to the DPLL then it can see the DPLL and control it as well.
>
>Okay, that would make sense.
>GENL_UNS_ADMIN_PERM | GENL_UNS_ADMIN_PERM then.
>

I guess a typo here? Shall be: 'GENL_UNS_ADMIN_PERM | GENL_ADMIN_PERM'?
Going to:
- apply those bits for all the dpll netlink commands,
- remove DPLLA_NETIFINDEX,
- leave pin DPLLA_PIN_NETIFINDEX as is.

Or I have missed something?

Thanks,
Arkadiusz

>>
>>What I was saying is some DPLL have no netdevs. So we can do the same
>>thing with devlinks. Let the driver link the DPLL to one or more
>>devlink instances, and if any of the devlink instances is in current
>>netns then you can see the DPLL.
>
>I don't think that would be needed to pull devlink into the picture.
>If not netdev is linked to dpll, GENL_ADMIN_PERM would apply.