netdev - Re: [Intel-wired-lan] [PATCH v2 iwl-next 06/10] ice: use bitfields instead of unions for CGU regs

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: <452ea0c4-d221-484b-a492-c3b6ca930a0c@intel.com>
Date: Thu, 17 Apr 2025 14:38:14 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Karol Kolacinski <karol.kolacinski@...el.com>,
	<intel-wired-lan@...ts.osuosl.org>
CC: <netdev@...r.kernel.org>, <anthony.l.nguyen@...el.com>,
	<przemyslaw.kitszel@...el.com>, Milena Olech <milena.olech@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH v2 iwl-next 06/10] ice: use bitfields
 instead of unions for CGU regs



On 4/9/2025 5:25 AM, Karol Kolacinski wrote:
> diff --git a/drivers/net/ethernet/intel/ice/ice_tspll.c b/drivers/net/ethernet/intel/ice/ice_tspll.c
> index 6bbb570841bb..15b07b7842d2 100644
> --- a/drivers/net/ethernet/intel/ice/ice_tspll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_tspll.c
> @@ -268,76 +270,89 @@ static int ice_tspll_cfg_e825c(struct ice_hw *hw, enum ice_tspll_freq clk_freq,
>  		return -EINVAL;
>  	}
>  
> -	ICE_READ_CGU_REG_OR_DIE(hw, ICE_CGU_R9, &dw9.val);
> -	ICE_READ_CGU_REG_OR_DIE(hw, ICE_CGU_R24, &dw24.val);
> -	ICE_READ_CGU_REG_OR_DIE(hw, ICE_CGU_R16, &dw16.val);
> -	ICE_READ_CGU_REG_OR_DIE(hw, ICE_CGU_R23, &dw23.val);
> -	ICE_READ_CGU_REG_OR_DIE(hw, TSPLL_RO_LOCK_E825C, &ro_lock.val);
> +	ICE_READ_CGU_REG_OR_DIE(hw, ICE_CGU_R9, &r9);
> +	ICE_READ_CGU_REG_OR_DIE(hw, ICE_CGU_R23, &r23);
> +	ICE_READ_CGU_REG_OR_DIE(hw, ICE_CGU_RO_LOCK, &val);
>  
> -	ice_tspll_log_cfg(hw, dw24.ts_pll_enable, dw23.time_ref_sel,
> -			  dw9.time_ref_freq_sel,
> -			  ro_lock.plllock_true_lock_cri, false);
> +	ice_tspll_log_cfg(hw, !!FIELD_GET(ICE_CGU_R23_R24_TSPLL_ENABLE, r23),
> +			  FIELD_GET(ICE_CGU_R23_R24_TIME_REF_SEL, r23),
> +			  FIELD_GET(ICE_CGU_R9_TIME_REF_FREQ_SEL, r9),
> +			  !!FIELD_GET(ICE_CGU_RO_LOCK_TRUE_LOCK, val),
> +			  false);
>  
E825-C used to check dw24 for the enable flag, but now checks dw23. This
is a bug fix for the log message, but you don't call that out in the commit.

I would prefer to see this split so that we do the functional changes
either before or after the rework of the names.

At a minimum, please call out any of the behavioral changes or bug fixes
in the commit message.


> +	if (FIELD_GET(ICE_CGU_R9_TIME_SYNC_EN, r9)) {
> +		r9 &= ~ICE_CGU_R9_TIME_SYNC_EN;
> +		ICE_WRITE_CGU_REG_OR_DIE(hw, ICE_CGU_R9, r9);
>  	}


Ditto here for the r9 changes which don't appear to be in the pre-image
either.

Thanks,
Jake