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

[Jiri Pirko <jiri@resnulli.us>]

Tue, Dec 13, 2022 at 07:08:13PM CET, arkadiusz.kubalewski@intel.com wrote:
>>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'?

Yes, sure.

>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?

I believe it is ok.

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