| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <umik2eecoutyaf666hy3h2g2bbbchvfb7veqwrcqkb6aevntxz@yhslcltafqxy>
Date: Mon, 7 Apr 2025 14:03:30 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org,
Tony Nguyen <anthony.l.nguyen@...el.com>, netdev@...r.kernel.org, Jacob Keller <jacob.e.keller@...el.com>,
Jakub Kicinski <kuba@...nel.org>, Aleksandr Loktionov <aleksandr.loktionov@...el.com>,
Karol Kolacinski <karol.kolacinski@...el.com>, Grzegorz Nitka <grzegorz.nitka@...el.com>,
Michal Schmidt <mschmidt@...hat.com>, Sergey Temerkhanov <sergey.temerkhanov@...el.com>
Subject: Re: [PATCH iwl-net v2] ice: use DSN instead of PCI BDF for
ice_adapter index
Mon, Apr 07, 2025 at 01:20:05PM +0200, przemyslaw.kitszel@...el.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@...el.com>
>Suggested-by: Jakub Kicinski <kuba@...nel.org>
>Suggested-by: Jiri Pirko <jiri@...nulli.us>
>Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
>Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>---
>CC: Karol Kolacinski <karol.kolacinski@...el.com>
>CC: Grzegorz Nitka <grzegorz.nitka@...el.com>
>CC: Michal Schmidt <mschmidt@...hat.com>
>CC: Sergey Temerkhanov <sergey.temerkhanov@...el.com>
>
>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/20250306211159.3697-2-przemyslaw.kitszel@intel.com
>---
> 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
>
Powered by blists - more mailing lists