Message-ID: <158dad4c-1eb4-4344-ac6b-7ea4737e0535@intel.com>
Date: Mon, 4 Aug 2025 14:33:37 -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>, Jiri Pirko <jiri@...nulli.us>,
	Jiri Pirko <jiri@...dia.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.
> ---

+Jiri,

I'd appreciate your opinion on this change, since you were the one to
originally suggest use of the device serial number.

Thanks,
Jake

>  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)
> +
> +#define ICE_ADAPTER_INDEX_E825C	\
> +	(ICE_DEV_ID_E825C_BACKPLANE | ICE_ADAPTER_FIXED_INDEX)
> +
> +static u64 ice_adapter_index(struct pci_dev *pdev)
>  {
> +	switch (pdev->device) {
> +	case ICE_DEV_ID_E825C_BACKPLANE:
> +	case ICE_DEV_ID_E825C_QSFP:
> +	case ICE_DEV_ID_E825C_SFP:
> +	case ICE_DEV_ID_E825C_SGMII:
> +		/* E825C devices have multiple NACs which are connected to the
> +		 * same clock source, and which must share the same
> +		 * ice_adapter structure. We can't use the serial number since
> +		 * each NAC has its own NVM generated with its own unique
> +		 * Device Serial Number. Instead, rely on the embedded nature
> +		 * of the E825C devices, and use a fixed index. This relies on
> +		 * the fact that all E825C physical functions in a given
> +		 * system are part of the same overall device.
> +		 */
> +		return ICE_ADAPTER_INDEX_E825C;
> +	default:
> +		return pci_get_dsn(pdev) & ~ICE_ADAPTER_FIXED_INDEX;
> +	}
> +}
> +
> +static unsigned long ice_adapter_xa_index(struct pci_dev *pdev)
> +{
> +	u64 index = ice_adapter_index(pdev);
> +
>  #if BITS_PER_LONG == 64
> -	return dsn;
> +	return index;
>  #else
> -	return (u32)dsn ^ (u32)(dsn >> 32);
> +	return (u32)index ^ (u32)(index >> 32);
>  #endif
>  }
>  
> -static struct ice_adapter *ice_adapter_new(u64 dsn)
> +static struct ice_adapter *ice_adapter_new(struct pci_dev *pdev)
>  {
>  	struct ice_adapter *adapter;
>  
> @@ -30,7 +59,7 @@ static struct ice_adapter *ice_adapter_new(u64 dsn)
>  	if (!adapter)
>  		return NULL;
>  
> -	adapter->device_serial_number = dsn;
> +	adapter->index = ice_adapter_index(pdev);
>  	spin_lock_init(&adapter->ptp_gltsyn_time_lock);
>  	spin_lock_init(&adapter->txq_ctx_lock);
>  	refcount_set(&adapter->refcount, 1);
> @@ -64,24 +93,23 @@ static void ice_adapter_free(struct ice_adapter *adapter)
>   */
>  struct ice_adapter *ice_adapter_get(struct pci_dev *pdev)
>  {
> -	u64 dsn = pci_get_dsn(pdev);
>  	struct ice_adapter *adapter;
>  	unsigned long index;
>  	int err;
>  
> -	index = ice_adapter_index(dsn);
> +	index = ice_adapter_xa_index(pdev);
>  	scoped_guard(mutex, &ice_adapters_mutex) {
>  		err = xa_insert(&ice_adapters, index, NULL, GFP_KERNEL);
>  		if (err == -EBUSY) {
>  			adapter = xa_load(&ice_adapters, index);
>  			refcount_inc(&adapter->refcount);
> -			WARN_ON_ONCE(adapter->device_serial_number != dsn);
> +			WARN_ON_ONCE(adapter->index != ice_adapter_index(pdev));
>  			return adapter;
>  		}
>  		if (err)
>  			return ERR_PTR(err);
>  
> -		adapter = ice_adapter_new(dsn);
> +		adapter = ice_adapter_new(pdev);
>  		if (!adapter)
>  			return ERR_PTR(-ENOMEM);
>  		xa_store(&ice_adapters, index, adapter, GFP_KERNEL);
> @@ -100,11 +128,10 @@ struct ice_adapter *ice_adapter_get(struct pci_dev *pdev)
>   */
>  void ice_adapter_put(struct pci_dev *pdev)
>  {
> -	u64 dsn = pci_get_dsn(pdev);
>  	struct ice_adapter *adapter;
>  	unsigned long index;
>  
> -	index = ice_adapter_index(dsn);
> +	index = ice_adapter_xa_index(pdev);
>  	scoped_guard(mutex, &ice_adapters_mutex) {
>  		adapter = xa_load(&ice_adapters, index);
>  		if (WARN_ON(!adapter))
> 
> ---
> base-commit: 01051012887329ea78eaca19b1d2eac4c9f601b5
> change-id: 20250731-jk-fix-e825c-ice-adapter-54428f5fcbe8
> 
> Best regards,
> --  
> Jacob Keller <jacob.e.keller@...el.com>
> 



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

Powered by blists - more mailing lists