Re: [PATCH RFC v6 2/6] dpll: Add DPLL framework base functions

Thu, May 04, 2023 at 06:04:01PM CEST, kuba@kernel.org wrote:

On Thu, 4 May 2023 13:00:42 +0200 Jiri Pirko wrote:

quoted

Thu, May 04, 2023 at 04:16:43AM CEST, kuba@kernel.org wrote:

quoted

On Wed, 3 May 2023 09:56:57 +0200 Jiri Pirko wrote:  

quoted

Okay.

When netdev will have pin ID in the RT netlink message (as it is done
in RFCv7), it is easy to get the pin/dpll for netdev. No problem there.

However, for non-SyncE usecase, how do you imagine scripts to work?
I mean, the script have to obtain dpll/pin ID by deterministic
module_name/clock_id/idx tuple.  

No scoped idx.  

That means, no index defined by a driver if I undestand you correctly,
right?

Yes, my suggestion did not include a scoped index with no
globally defined semantics.

Okay, makes sense. Devlink port index didn't end up well :/

quoted

quoted

quoted

There are 2 options to do that:
1) dump all dplls/pins and do lookup in userspace
2) get a dpll/pin according to given module_name/clock_id/idx tuple

The first approach is not very nice.
The currently pushed RFCv7 of the patchset does not support 2)

Now if we add support for 2), we basically use module_name/clock_id/idx
as a handle for "get cmd". My point is, why can't we use it for "set
cmd" as well and avoid the ID entirely?  

Sure, we don't _have_ to have an ID, but it seems go against normal
data normalization rules. And I don't see any harm in it.

But you're asking for per-device "idx" and that's a no-go for me,
given already cited experience.

The user space can look up the ID based on identifying information it
has. IMO it's better to support multiple different intelligible elements  

Do you mean fixed tuple or variable tuple?

CMD_GET_ID
  -> DPLL_A_MODULE_NAME  
     DPLL_A_CLOCK_ID

quoted

What is the next intelligible element to identify DPLL device here?

I don't know. We can always add more as needed.
We presuppose that the devices are identifiable, so whatever info
is used to identify them goes here.

Allright. So in case of ptp_ocp and mlx5, module_name and clock_id
are enough. In case of ice, DPLL_A_TYPE, attr is the one to make
distinction between the 2 dpll instances there

So for now, we can have:
 CMD_GET_ID
   -> DPLL_A_MODULE_NAME
      DPLL_A_CLOCK_ID
      DPLL_A_TYPE
   <- DPLL_A_ID


if user passes a subset which would not provide a single match, we error
out with -EINVAL and proper exack message. Makes sense?

quoted

  <- DPLL_A_ID

CMD_GET_PIN_ID
  -> DPLL_A_MODULE_NAME  
     DPLL_A_CLOCK_ID

quoted

What is the next intelligible element to identify a pin here?

Same answer. Could be a name of the pin according to ASIC docs.
Could be the ball name for a BGA package. Anything that's meaningful.

Okay, for pin, the type and label would probably do:
 CMD_GET_PIN_ID
   -> DPLL_A_MODULE_NAME
      DPLL_A_CLOCK_ID
      DPLL_A_PIN_TYPE
      DPLL_A_PIN_LABEL
   <- DPLL_A_PIN_ID

Again, if user passes a subset which would not provide a single match,
we error out with -EINVAL and proper exack message.

If there is only one pin for example, user query of DPLL_A_MODULE_NAME
and DPLL_A_CLOCK_ID would do return a single match. No need to pass
anything else.

I think this could work with both ice and ptp_ocp, correct guys?

For mlx5, I will have 2 or more pins with same module name, clock id
and type. For these SyncE pins the label does not really make sense.
But I don't have to query, because the PIN_ID is going to be exposed for
netdev over RT netlink. Clicks.

Makes sense?

My point is that we don't want a field simply called "index". Because
then for one vendor it will mean Ethernet port, for another SMA
connector number and for the third pin of the package. Those are
different attributes.

Got you and agree.

quoted

  <- DPLL_A_PIN_ID

quoted

than single integer index into which drivers will start encoding all
sort of info, using locally invented schemes.  

There could be multiple DPLL and pin instances for a single
module/clock_id tuple we have to distinguish somehow. If the driver
can't pass "index" of DPLL or a pin, how we distinguish them?

Plus is is possible that 2 driver instances share the same dpll
instance, then to get the dpll pointer reference, they do:
INSTANCE A:
dpll_0 = dpll_device_get(clock_id, 0, THIS_MODULE);
dpll_1 = dpll_device_get(clock_id, 1, THIS_MODULE);

INSTANCE B:
dpll_0 = dpll_device_get(clock_id, 0, THIS_MODULE);
dpll_1 = dpll_device_get(clock_id, 1, THIS_MODULE);

My point is, event if we don't expose the index to the userspace,
we need to have it internally.

That's fine, I guess. I'd prefer driver matching to be the same as user
space matching to force driver authors to have the same perspective as
the user. But a "driver coookie" not visible to user space it probably
fine.

Allright, lets leave them for now. As internal kernel API, could be
changed in the future if needed.

Arkadiusz, Vadim, are you following this?