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

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

[Przemek Kitszel]

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>
---
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);
 }
 
 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)
 {
 	unsigned long index = ice_adapter_index(pdev);
 	struct ice_adapter *adapter;
-- 
2.46.0

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

[Jacob Keller]

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;

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

[Jiri Pirko]

Fri, Mar 07, 2025 at 12:53:05AM +0100, jacob.e.keller@intel.com wrote:
>
>
>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.

Do we need a workaround for pre-production buggy hw now? Sounds a bit
weird tbh.


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

Would make sense to me to make it const.


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

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

[Jacob Keller]

On 3/7/2025 4:40 AM, Jiri Pirko wrote:
> Fri, Mar 07, 2025 at 12:53:05AM +0100, jacob.e.keller@intel.com wrote:
>>
>>
>> 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.
> 
> Do we need a workaround for pre-production buggy hw now? Sounds a bit
> weird tbh.
> 

I agree that use of the serial number is preferred over BDF for the
reasons described in this thread.

But I also know that sometimes the DSN is not available, or is not set
properly during pre-production and early testing. This could cause
issues for early development. These issues can likely be worked around
and should not impact what we do for properly functioning boards.

I just want to make it clear on the record, since it is likely that we
would see this if using an old or badly flashed board, or if we use this
same scheme on a future hardware (or even just a spin of the ice
hardware). In those cases, developers might have breaking functionality
like multiple adapters being tied to the same adapter structure.

I *don't* want those to be reported as issues with this code, as they
are really issues with the flash data. Perhaps we could have some sort
of warning message to go "this doesn't look right" when the DSN
capability doesn't exist or when the DSN is 0.

In the end, the places where this is likely to fail are also the places
where hopefully the developers are smart enough to know what is going on.

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

[Przemek Kitszel]

>>> -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?
> 
> Would make sense to me to make it const.

It would indeed, but to do so, one have to constify at least a few other
pci_* functions, I didn't even got to the bottom.
While I appreciate the added value of typechecks, I would like to focus
on different work, especially that there are contributors that seems to
be focused on the hunt of such cases :)

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

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

[Jiri Pirko]

Thu, Mar 06, 2025 at 10:11:46PM +0100, przemyslaw.kitszel@intel.com 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>

From my perspective, this is a bug fix, makes sense to me to send to
-net tree.


>---
>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);

How do you ensure there is no xarray index collision then you
cut the number like this?


> }
> 
> 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)
> {
> 	unsigned long index = ice_adapter_index(pdev);
> 	struct ice_adapter *adapter;
>-- 
>2.46.0
>

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

[Temerkhanov, Sergey]

Comments inline

-----Original Message-----
From: Jiri Pirko <jiri@resnulli.us> 
Sent: Friday, March 7, 2025 1:39 PM
To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; netdev@vger.kernel.org; Keller, Jacob E <jacob.e.keller@intel.com>; Jakub Kicinski <kuba@kernel.org>; Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; Kolacinski, Karol <karol.kolacinski@intel.com>; Nitka, Grzegorz <grzegorz.nitka@intel.com>; Schmidt, Michal <mschmidt@redhat.com>; Temerkhanov, Sergey <sergey.temerkhanov@intel.com>
Subject: Re: [PATCH iwl-next] ice: use DSN instead of PCI BDF for ice_adapter index

Thu, Mar 06, 2025 at 10:11:46PM +0100, przemyslaw.kitszel@intel.com 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).

It might be worth checking behavior of different hypervisors/VMMs with multifunction PCI devices passthrough.
QEMU, for example, has options to set the BDFs explicitly.

>
>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>

>-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);

>How do you ensure there is no xarray index collision then you cut the number like this?

It is also probably necessary to check if all devices supported by the driver have DSN capability enabled.

Regards,
Sergey

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

[Przemek Kitszel]

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

regarding -net vs -next, no one have complained that this bug hurts

>> +	return (unsigned long)pci_get_dsn(pdev);
> 
>> How do you ensure there is no xarray index collision then you cut the number like this?

The reduction occurs only on "32b" systems, which are unlikely to have
this device. And any mixing of the upper and lower 4B part still could
collide.

> 
> It is also probably necessary to check if all devices supported by the driver have DSN capability enabled.

I will double check on the SoC you have in mind.

> 
> Regards,
> Sergey

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

[Jiri Pirko]

Mon, Mar 10, 2025 at 09:40:16AM +0100, przemyslaw.kitszel@intel.com wrote:
>> Subject: Re: [PATCH iwl-next] ice: use DSN instead of PCI BDF for ice_adapter index
>
>regarding -net vs -next, no one have complained that this bug hurts

Wait, so we are now waiting for someone to hit the bug and complain,
before we do fix? Does not make any sense to me.


>
>> > +	return (unsigned long)pci_get_dsn(pdev);
>> 
>> > How do you ensure there is no xarray index collision then you cut the number like this?
>
>The reduction occurs only on "32b" systems, which are unlikely to have
>this device. And any mixing of the upper and lower 4B part still could
>collide.

Passtrough to 32 bit qemu machine? Even how unlikely is that, you are
risking a user to hit a bug for newly introduced code without good
reason. Why?


>
>> 
>> It is also probably necessary to check if all devices supported by the driver have DSN capability enabled.
>
>I will double check on the SoC you have in mind.
>
>> 
>> Regards,
>> Sergey
>

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

[Przemek Kitszel]

On 3/10/25 13:23, Jiri Pirko wrote:
> Mon, Mar 10, 2025 at 09:40:16AM +0100, przemyslaw.kitszel@intel.com wrote:
>>> Subject: Re: [PATCH iwl-next] ice: use DSN instead of PCI BDF for ice_adapter index
>>
>> regarding -net vs -next, no one have complained that this bug hurts
> 
> Wait, so we are now waiting for someone to hit the bug and complain,
> before we do fix? Does not make any sense to me.

no one is waiting for a fix, but it could affect users with weird NVM
images, so -next seems reasonable

> 
> 
>>
>>>> +	return (unsigned long)pci_get_dsn(pdev);
>>>
>>>> How do you ensure there is no xarray index collision then you cut the number like this?
>>
>> The reduction occurs only on "32b" systems, which are unlikely to have
>> this device. And any mixing of the upper and lower 4B part still could
>> collide.
> 
> Passtrough to 32 bit qemu machine? Even how unlikely is that, you are
> risking a user to hit a bug for newly introduced code without good
> reason. Why?

I will combine the two, by simple xor

> 
> 
>>
>>>
>>> It is also probably necessary to check if all devices supported by the driver have DSN capability enabled.
>>
>> I will double check on the SoC you have in mind.

IMO an NVM issue, will handle this offline