Message-ID: <4a9555f6-a494-4ee1-85a0-d95d8f0c5703@intel.com>
Date: Mon, 4 Aug 2025 14:18:12 -0700
From: Tony Nguyen <anthony.l.nguyen@...el.com>
To: Jacob Keller <jacob.e.keller@...el.com>, 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>
CC: Aleksandr Loktionov <aleksandr.loktionov@...el.com>, Jiri Pirko
	<jiri@...nulli.us>
Subject: Re: [PATCH iwl-net] ice: use fixed adapter index for E825C embedded
 devices



On 8/1/2025 4:08 PM, Jacob Keller wrote:
> 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.

cc Jiri
>> 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?

Yea, I can do that.

Thanks,
Tony

Powered by blists - more mailing lists