Mon, Apr 07, 2025 at 01:20:05PM +0200, [email protected] 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 <[email protected]>
>Suggested-by: Jakub Kicinski <[email protected]>
>Suggested-by: Jiri Pirko <[email protected]>
>Reviewed-by: Aleksandr Loktionov <[email protected]>
>Signed-off-by: Przemek Kitszel <[email protected]>
>---
>CC: Karol Kolacinski <[email protected]>
>CC: Grzegorz Nitka <[email protected]>
>CC: Michal Schmidt <[email protected]>
>CC: Sergey Temerkhanov <[email protected]>
>
>v2:
> - target to -net (Jiri)
> - mix both halves of u64 DSN on 32bit systems (Jiri)
> - (no changes in terms of fallbacks for pre-prod HW)
> - warn when there is DSN collision after reducing to 32bit
>
>v1:
>https://lore.kernel.org/netdev/[email protected]
>---
> drivers/net/ethernet/intel/ice/ice_adapter.h | 6 ++-
> drivers/net/ethernet/intel/ice/ice_adapter.c | 43 ++++++++------------
> 2 files changed, 20 insertions(+), 29 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h
>b/drivers/net/ethernet/intel/ice/ice_adapter.h
>index e233225848b3..ac15c0d2bc1a 100644
>--- a/drivers/net/ethernet/intel/ice/ice_adapter.h
>+++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
>@@ -32,17 +32,19 @@ struct ice_port_list {
> * @refcount: Reference count. struct ice_pf objects hold the references.
> * @ctrl_pf: Control PF of the adapter
> * @ports: Ports list
>+ * @device_serial_number: DSN cached for collision detection on 32bit systems
> */
> struct ice_adapter {
> refcount_t refcount;
> /* For access to the GLTSYN_TIME register */
> spinlock_t ptp_gltsyn_time_lock;
>
> struct ice_pf *ctrl_pf;
> struct ice_port_list ports;
>+ u64 device_serial_number;
> };
>
>-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..3df3fa6d5129 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,13 @@
> 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(u64 dsn)
> {
>- 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));
>- }
>+#if BITS_PER_LONG == 64
>+ return dsn;
>+#else
>+ return (u32)dsn ^ u32(dsn >> 32);
>+#endif
> }
>
> static struct ice_adapter *ice_adapter_new(void)
>@@ -77,25 +60,29 @@ 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);
>+ u64 dsn = pci_get_dsn(pdev);
> struct ice_adapter *adapter;
>+ unsigned long index;
> int err;
>
>+ index = ice_adapter_index(dsn);
> 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 and done? How unlikely is this? I mean, can this happen in real
world? If yes, that's a bug.
> return adapter;
> }
> if (err)
> return ERR_PTR(err);
>
> adapter = ice_adapter_new();
> if (!adapter)
> return ERR_PTR(-ENOMEM);
>+ adapter->device_serial_number = dsn;
> xa_store(&ice_adapters, index, adapter, GFP_KERNEL);
> }
> return adapter;
>@@ -110,11 +97,13 @@ 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);
>+ u64 dsn = pci_get_dsn(pdev);
> struct ice_adapter *adapter;
>+ unsigned long index;
>
>+ index = ice_adapter_index(dsn);
> scoped_guard(mutex, &ice_adapters_mutex) {
> adapter = xa_load(&ice_adapters, index);
> if (WARN_ON(!adapter))
>--
>2.39.3
>
