netdev - Re: [PATCH net-next 1/2] dpll: add phase-adjust-gran pin attribute

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d397e88f-acb5-4e0f-aaae-8f6eddf60164@redhat.com>
Date: Wed, 29 Oct 2025 12:17:52 +0100
From: Ivan Vecera <ivecera@...hat.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>, netdev@...r.kernel.org
Cc: Michal Schmidt <mschmidt@...hat.com>, Petr Oros <poros@...hat.com>,
 Prathosh Satish <Prathosh.Satish@...rochip.com>,
 Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
 Jiri Pirko <jiri@...nulli.us>, Jonathan Corbet <corbet@....net>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Simon Horman <horms@...nel.org>, Donald Hunter <donald.hunter@...il.com>,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 1/2] dpll: add phase-adjust-gran pin attribute

On 10/29/25 11:20 AM, Vadim Fedorenko wrote:
> On 24/10/2025 15:49, Ivan Vecera wrote:
>> Phase-adjust values are currently limited by a min-max range. Some
>> hardware requires, for certain pin types, that values be multiples of
>> a specific granularity, as in the zl3073x driver.
>>
>> Add a `phase-adjust-gran` pin attribute and an appropriate field in
>> dpll_pin_properties. If set by the driver, use its value to validate
>> user-provided phase-adjust values.
>>
>> Reviewed-by: Michal Schmidt <mschmidt@...hat.com>
>> Reviewed-by: Petr Oros <poros@...hat.com>
>> Tested-by: Prathosh Satish <Prathosh.Satish@...rochip.com>
>> Signed-off-by: Ivan Vecera <ivecera@...hat.com>
>> ---
>>   Documentation/driver-api/dpll.rst     | 36 +++++++++++++++------------
>>   Documentation/netlink/specs/dpll.yaml |  7 ++++++
>>   drivers/dpll/dpll_netlink.c           | 12 ++++++++-
>>   include/linux/dpll.h                  |  1 +
>>   include/uapi/linux/dpll.h             |  1 +
>>   5 files changed, 40 insertions(+), 17 deletions(-)
>>
>> @@ -1261,7 +1265,13 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, 
>> struct nlattr *phase_adj_attr,
>>       if (phase_adj > pin->prop.phase_range.max ||
>>           phase_adj < pin->prop.phase_range.min) {
>>           NL_SET_ERR_MSG_ATTR(extack, phase_adj_attr,
>> -                    "phase adjust value not supported");
>> +                    "phase adjust value of out range");
>> +        return -EINVAL;
>> +    }
>> +    if (pin->prop.phase_gran && phase_adj % pin->prop.phase_gran) {
>> +        NL_SET_ERR_MSG_ATTR_FMT(extack, phase_adj_attr,
>> +                    "phase adjust value not multiple of %u",
>> +                    pin->prop.phase_gran);
> 
> That is pretty strict on the uAPI input. Maybe it's better to allow any
> value, but report back the one that is really applied based on driver's
> understanding of hardware? I mean the driver is doing some math before
> applying the math, it can potentially round any value to the closest
> acceptable and report it back?

I’d prefer to use the same approach as for phase-adjust-{min,max}.
Because we could treat them the same way - the user sets a value
above/below the max/min, and the driver clamps it.

Would it be better? I don't think so.

Let’s say the granularity is 1000, and the user sets a value of 499...
then the driver rounds it and sets it to 0. The user then reads the
current value via pin-get and finds that it hasn’t changed - quite
confusing, I’d say. If the user knows the granularity in advance,
they can adjust accordingly.

IMHO, strict behavior is better than smart behavior.

Ivan