Re: [PATCH iwl-next] ice: use DSN instead of PCI BDF for ice_adapter index

[Jacob Keller <jacob.e.keller@intel.com>]

On 3/6/2025 1:11 PM, Przemek Kitszel wrote:
> Use Device Serial Number instead of PCI bus/device/function for
> index of struct ice_adapter.
> Functions on the same physical device should point to the very same
> ice_adapter instance.
> 
> This is not only simplification, but also fixes things up when PF
> is passed to VM (and thus has a random BDF).
> 
> Suggested-by: Jacob Keller <jacob.e.keller@intel.com>
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Suggested-by: Jiri Pirko <jiri@resnulli.us>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---

The only caution I have here is that we might run into issues with
pre-production or poorly flashed boards which don't have DSN properly
flashed. This shouldn't be an impact outside of early testing or
mistakes by devs. I think there is a default ID which is almost all 0s
we could check and log a warning to help prevent confusion in such a case?

A couple systems I've seen have serial numbers like:

  serial_number 00-00-00-00-00-00-00-00
  serial_number 00-00-00-00-00-00-00-00

or

  serial_number 00-01-00-ff-ff-00-00-00
  serial_number 00-01-00-ff-ff-00-00-00


In practice I'm not sure how big a deal breaker this is. Properly
initialized boards should have unique IDs, and if you update via
devlink, or any of our standard update tools, it will maintain the ID
across flash. However, during early development, boards were often
flashed manually which could lead to such non-unique IDs.

> CC: Karol Kolacinski <karol.kolacinski@intel.com>
> CC: Grzegorz Nitka <grzegorz.nitka@intel.com>
> CC: Michal Schmidt <mschmidt@redhat.com>
> CC: Sergey Temerkhanov <sergey.temerkhanov@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_adapter.h |  4 +--
>  drivers/net/ethernet/intel/ice/ice_adapter.c | 29 +++-----------------
>  2 files changed, 6 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
> index e233225848b3..1935163bd32f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adapter.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
> @@ -42,7 +42,7 @@ struct ice_adapter {
>  	struct ice_port_list ports;
>  };
>  
> -struct ice_adapter *ice_adapter_get(const struct pci_dev *pdev);
> -void ice_adapter_put(const struct pci_dev *pdev);
> +struct ice_adapter *ice_adapter_get(struct pci_dev *pdev);
> +void ice_adapter_put(struct pci_dev *pdev);
>  
>  #endif /* _ICE_ADAPTER_H */
> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
> index 01a08cfd0090..b668339ed0ef 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adapter.c
> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
> @@ -1,7 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  // SPDX-FileCopyrightText: Copyright Red Hat
>  
> -#include <linux/bitfield.h>
>  #include <linux/cleanup.h>
>  #include <linux/mutex.h>
>  #include <linux/pci.h>
> @@ -14,29 +13,9 @@
>  static DEFINE_XARRAY(ice_adapters);
>  static DEFINE_MUTEX(ice_adapters_mutex);
>  
> -/* PCI bus number is 8 bits. Slot is 5 bits. Domain can have the rest. */
> -#define INDEX_FIELD_DOMAIN GENMASK(BITS_PER_LONG - 1, 13)
> -#define INDEX_FIELD_DEV    GENMASK(31, 16)
> -#define INDEX_FIELD_BUS    GENMASK(12, 5)
> -#define INDEX_FIELD_SLOT   GENMASK(4, 0)
> -
> -static unsigned long ice_adapter_index(const struct pci_dev *pdev)
> +static unsigned long ice_adapter_index(struct pci_dev *pdev)
>  {
> -	unsigned int domain = pci_domain_nr(pdev->bus);
> -
> -	WARN_ON(domain > FIELD_MAX(INDEX_FIELD_DOMAIN));
> -
> -	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:
> -		return FIELD_PREP(INDEX_FIELD_DEV, pdev->device);
> -	default:
> -		return FIELD_PREP(INDEX_FIELD_DOMAIN, domain) |
> -		       FIELD_PREP(INDEX_FIELD_BUS,    pdev->bus->number) |
> -		       FIELD_PREP(INDEX_FIELD_SLOT,   PCI_SLOT(pdev->devfn));
> -	}
> +	return (unsigned long)pci_get_dsn(pdev);

Much simpler :D

>  }
>  
>  static struct ice_adapter *ice_adapter_new(void)
> @@ -77,7 +56,7 @@ static void ice_adapter_free(struct ice_adapter *adapter)
>   * Return:  Pointer to ice_adapter on success.
>   *          ERR_PTR() on error. -ENOMEM is the only possible error.
>   */
> -struct ice_adapter *ice_adapter_get(const struct pci_dev *pdev)
> +struct ice_adapter *ice_adapter_get(struct pci_dev *pdev)
>  {
>  	unsigned long index = ice_adapter_index(pdev);
>  	struct ice_adapter *adapter;
> @@ -110,7 +89,7 @@ struct ice_adapter *ice_adapter_get(const struct pci_dev *pdev)
>   *
>   * Context: Process, may sleep.
>   */
> -void ice_adapter_put(const struct pci_dev *pdev)
> +void ice_adapter_put(struct pci_dev *pdev)
>  {

A bit of a shame that this needs to be non const now.. Could
pci_get_dsn() be made const? Or does it do something which might modify
the device somehow?

>  	unsigned long index = ice_adapter_index(pdev);
>  	struct ice_adapter *adapter;