netdev - Re: [PATCH iwl-net] ice: fix max values for dpll pin phase adjust

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: <20241203100515.GB9361@kernel.org>
Date: Tue, 3 Dec 2024 10:05:15 +0000
From: Simon Horman <horms@...nel.org>
To: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>
Subject: Re: [PATCH iwl-net] ice: fix max values for dpll pin phase adjust

On Wed, Nov 20, 2024 at 08:51:12AM +0100, Arkadiusz Kubalewski wrote:
> Mask admin command returned max phase adjust value for both input and
> output pins. Only 31 bits are relevant, last released data sheet wrongly
> points that 32 bits are valid - see [1] 3.2.6.4.1 Get CCU Capabilities
> Command for reference. Fix of the datasheet itself is in progress.
> 
> Fix the min/max assignment logic, previously the value was wrongly
> considered as negative value due to most significant bit being set.

Thanks Arkadiusz,

I understand the most-significant-bit issue and see that is addressed
through the use of ICE_AQC_GET_CGU_MAX_PHASE_ADJ. I also agree that this is
a fix.

But, although I like simplification afforded ice_dpll_phase_range_set()
I'm not convinced it is a part of the fix. Does the code behave correctly
without those changes? If so, I'm wondering if that part should be broken
out into a separate follow-up patch for iwl.

> 
> Example of previous broken behavior:
> $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \
> --do pin-get --json '{"id":1}'| grep phase-adjust
>  'phase-adjust': 0,
>  'phase-adjust-max': 16723,
>  'phase-adjust-min': -16723,

I'm curious to know if the values for max and min above are inverted.
I.e. if, sude to the most-significant-bit issue they are:

  'phase-adjust-max': -16723,
  'phase-adjust-min': 16723,

> 
> Correct behavior with the fix:
> $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \
> --do pin-get --json '{"id":1}'| grep phase-adjust
>  'phase-adjust': 0,
>  'phase-adjust-max': 2147466925,
>  'phase-adjust-min': -2147466925,
> 
> [1] https://cdrdv2.intel.com/v1/dl/getContent/613875?explicitVersion=true
> 
> Fixes: 90e1c90750d7 ("ice: dpll: implement phase related callbacks")
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>

...