Message-ID: <8a676506-dd13-4198-813b-b61ad2953603@intel.com>
Date: Fri, 1 Aug 2025 16:08:00 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Sergey Temerkhanov <sergey.temerkhanov@...el.com>, Przemek Kitszel
	<przemyslaw.kitszel@...el.com>, Grzegorz Nitka <grzegorz.nitka@...el.com>,
	<netdev@...r.kernel.org>, Intel Wired LAN <intel-wired-lan@...ts.osuosl.org>,
	Anthony Nguyen <anthony.l.nguyen@...el.com>
CC: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
Subject: Re: [PATCH iwl-net] ice: use fixed adapter index for E825C embedded
 devices



On 8/1/2025 3:27 PM, Jacob Keller wrote:
> The ice_adapter structure is used by the ice driver to connect multiple
> physical functions of a device in software. It was introduced by
> commit 0e2bddf9e5f9 ("ice: add ice_adapter for shared data across PFs on
> the same NIC") and is primarily used for PTP support, as well as for
> handling certain cross-PF synchronization.
> 
> The original design of ice_adapter used PCI address information to
> determine which devices should be connected. This was extended to support
> E825C devices by commit fdb7f54700b1 ("ice: Initial support for E825C
> hardware in ice_adapter"), which used the device ID for E825C devices
> instead of the PCI address.
> 
> Later, commit 0093cb194a75 ("ice: use DSN instead of PCI BDF for
> ice_adapter index") replaced the use of Bus/Device/Function addressing with
> use of the device serial number.
> 
> E825C devices may appear in "Dual NAC" configuration which has multiple
> physical devices tied to the same clock source and which need to use the
> same ice_adapter. Unfortunately, each "NAC" has its own NVM which has its
> own unique Device Serial Number. Thus, use of the DSN for connecting
> ice_adapter does not work properly. It "worked" in the pre-production
> systems because the DSN was not initialized on the test NVMs and all the
> NACs had the same zero'd serial number.
> 
> Since we cannot rely on the DSN, lets fall back to the logic in the
> original E825C support which used the device ID. This is safe for E825C
> only because of the embedded nature of the device. It isn't a discreet
> adapter that can be plugged into an arbitrary system. All E825C devices on
> a given system are connected to the same clock source and need to be
> configured through the same PTP clock.
> 
> To make this separation clear, reserve bit 63 of the 64-bit index values as
> a "fixed index" indicator. Always clear this bit when using the device
> serial number as an index.
> 
> For E825C, use a fixed value defined as the 0x579C E825C backplane device
> ID bitwise ORed with the fixed index indicator. This is slightly different
> than the original logic of just using the device ID directly. Doing so
> prevents a potential issue with systems where only one of the NACs is
> connected with an external PHY over SGMII. In that case, one NAC would
> have the E825C_SGMII device ID, but the other would not.
> 
> Separate the determination of the full 64-bit index from the 32-bit
> reduction logic. Provide both ice_adapter_index() and a wrapping
> ice_adapter_xa_index() which handles reducing the index to a long on 32-bit
> systems. As before, cache the full index value in the adapter structure to
> warn about collisions.
> 
> This fixes issues with E825C not initializing PTP on both NACs, due to
> failure to connect the appropriate devices to the same ice_adapter.
> 
> Fixes: 0093cb194a75 ("ice: use DSN instead of PCI BDF for ice_adapter index")
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> Reviewed-by: Grzegorz Nitka <grzegorz.nitka@...el.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
> ---
> It turns out that using the device serial number does not work for E825C
> boards. I spoke with the team involved in the NVM image generation, and its
> not feasible at this point to change the process for generating the NVMs
> for E825C. We're stuck with the case that E825C Dual-NAC boards will have
> independent DSN for each NAC.
> 
> As far as I can tell, the only suitable fallback is to rely on the embedded
> nature of the E825C device. We know that all current systems with E825C
> need to have their ice_adapter connected. There are no plans to build
> platforms with multiple E825C devices. The E825C variant is not a discreet
> board, so customers can't simply plug an extra in. Thus, this change
> reverts back to using the device ID for E825C systems, instead of the
> serial number.
> ---
>  drivers/net/ethernet/intel/ice/ice_adapter.h |  4 +--
>  drivers/net/ethernet/intel/ice/ice_adapter.c | 49 +++++++++++++++++++++-------
>  2 files changed, 40 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
> index db66d03c9f96..e95266c7f20b 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adapter.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
> @@ -33,7 +33,7 @@ struct ice_port_list {
>   * @txq_ctx_lock: Spinlock protecting access to the GLCOMM_QTX_CNTX_CTL register
>   * @ctrl_pf: Control PF of the adapter
>   * @ports: Ports list
> - * @device_serial_number: DSN cached for collision detection on 32bit systems
> + * @index: 64-bit index cached for collision detection on 32bit systems
>   */
>  struct ice_adapter {
>  	refcount_t refcount;
> @@ -44,7 +44,7 @@ struct ice_adapter {
>  
>  	struct ice_pf *ctrl_pf;
>  	struct ice_port_list ports;
> -	u64 device_serial_number;
> +	u64 index;
>  };
>  
>  struct ice_adapter *ice_adapter_get(struct pci_dev *pdev);
> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
> index 9e4adc43e474..9ec2a815a3f7 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adapter.c
> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
> @@ -13,16 +13,45 @@
>  static DEFINE_XARRAY(ice_adapters);
>  static DEFINE_MUTEX(ice_adapters_mutex);
>  
> -static unsigned long ice_adapter_index(u64 dsn)
> +#define ICE_ADAPTER_FIXED_INDEX	BIT(63)
> +

This needs to be BIT_ULL :(

Tony, would it be possible for you to fix this up locally?

Thanks,
Jake


Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)

Powered by blists - more mailing lists