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

[Jiri Pirko <jiri@resnulli.us>]

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:
>> Thu, May 04, 2023 at 04:16:43AM CEST, kuba@kernel.org wrote:
>> >On Wed, 3 May 2023 09:56:57 +0200 Jiri Pirko wrote:  
>> >> 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 :/


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





>
>>   <- DPLL_A_ID
>> 
>> CMD_GET_PIN_ID
>>   -> DPLL_A_MODULE_NAME  
>>      DPLL_A_CLOCK_ID
>
>> 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.


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