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

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:

quoted

On Fri, 9 Dec 2022 10:29:53 +0100 Jiri Pirko wrote:

quoted

Thu, Dec 08, 2022 at 06:05:17PM CET, kuba@kernel.org wrote:

quoted

On Thu, 8 Dec 2022 17:33:28 +0100 Jiri Pirko wrote:

quoted

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

quoted

so that's what I'm commenting on - aligning visibility of objects with
already used permissions.

quoted

quoted

quoted

I can't imagine practically havind the whole dpll instance netns

aware.

quoted

quoted

quoted

quoted

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

quoted

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.