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: <b3f45ab3-348b-4e3e-95af-5dc16bb1be96@redhat.com>
Date: Wed, 29 Oct 2025 08:44:52 +0100
From: Ivan Vecera <ivecera@...hat.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, Michal Schmidt <mschmidt@...hat.com>,
 Petr Oros <poros@...hat.com>, Prathosh Satish
 <Prathosh.Satish@...rochip.com>, Vadim Fedorenko
 <vadim.fedorenko@...ux.dev>,
 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>,
 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

Hi Kuba,

On 10/29/25 2:39 AM, Jakub Kicinski wrote:
> On Fri, 24 Oct 2025 16:49:26 +0200 Ivan Vecera wrote:
>> +      -
>> +        name: phase-adjust-gran
>> +        type: s32
>> +        doc: |
>> +          Granularity of phase adjustment, in picoseconds. The value of
>> +          phase adjustment must be a multiple of this granularity.
> 
> Do we need this to be signed?
> 
To have it unsigned brings a need to use explicit type casting in the 
core and driver's code. The phase adjustment can be both positive and
negative it has to be signed. The granularity specifies that adjustment
has to be multiple of granularity value so the core checks for zero
remainder (this patch) and the driver converts the given adjustment
value using division by the granularity.

If we would have phase-adjust-gran and corresponding structure fields
defined as u32 then we have to explicitly cast the granularity to s32
because for:

<snip>
s32 phase_adjust, remainder;
u32 phase_gran = 1000;

phase_adjust = 5000;
remainder = phase_adjust % phase_gran;
/* remainder = 0 -> OK for positive adjust */

phase_adjust = -5000;
remainder = phase_adjust % phase_gran;
/* remainder = 296
  * Wrong for negative adjustment because phase_adjust is casted to u32
  * prior division -> 2^32 - 5000 = 4294962296.
  * 4294962296 % 1000 = 296
  */

  remainder = phase_adjust % (s32)phase_gran;
  /* remainder = 0
   * Now OK because phase_adjust remains to be s32
   */
</snip>

Similarly for division in the driver code if the granularity would be
u32.

So I have proposed phase adjustment granularity to be s32 to avoid these
explicit type castings and potential bugs in drivers.

Thanks,
Ivan