| 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
| ||
|
Date: Wed, 6 Mar 2024 20:20:33 +0100
From: Michal Schmidt <mschmidt@...hat.com>
To: Jiri Pirko <jiri@...nulli.us>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
Jacob Keller <jacob.e.keller@...el.com>, Jakub Kicinski <kuba@...nel.org>,
Jesse Brandeburg <jesse.brandeburg@...el.com>,
Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
Karol Kolacinski <karol.kolacinski@...el.com>
Subject: Re: [PATCH net-next v2 1/3] ice: add ice_adapter for shared data
across PFs on the same NIC
On Wed, Mar 6, 2024 at 6:00 PM Jiri Pirko <jiri@...nulli.us> wrote:
> Wed, Mar 06, 2024 at 05:29:05PM CET, mschmidt@...hat.com wrote:
> >There is a need for synchronization between ice PFs on the same physical
> >adapter.
> >
> >Add a "struct ice_adapter" for holding data shared between PFs of the
> >same multifunction PCI device. The struct is refcounted - each ice_pf
> >holds a reference to it.
> >
> >Its first use will be for PTP. I expect it will be useful also to
> >improve the ugliness that is ice_prot_id_tbl.
> >
> >Signed-off-by: Michal Schmidt <mschmidt@...hat.com>
> >---
> > drivers/net/ethernet/intel/ice/Makefile | 3 +-
> > drivers/net/ethernet/intel/ice/ice.h | 2 +
> > drivers/net/ethernet/intel/ice/ice_adapter.c | 85 ++++++++++++++++++++
> > drivers/net/ethernet/intel/ice/ice_adapter.h | 22 +++++
> > drivers/net/ethernet/intel/ice/ice_main.c | 8 ++
> > 5 files changed, 119 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/net/ethernet/intel/ice/ice_adapter.c
> > create mode 100644 drivers/net/ethernet/intel/ice/ice_adapter.h
> >
> >diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
> >index cddd82d4ca0f..4fa09c321440 100644
> >--- a/drivers/net/ethernet/intel/ice/Makefile
> >+++ b/drivers/net/ethernet/intel/ice/Makefile
> >@@ -36,7 +36,8 @@ ice-y := ice_main.o \
> > ice_repr.o \
> > ice_tc_lib.o \
> > ice_fwlog.o \
> >- ice_debugfs.o
> >+ ice_debugfs.o \
> >+ ice_adapter.o
> > ice-$(CONFIG_PCI_IOV) += \
> > ice_sriov.o \
> > ice_virtchnl.o \
> >diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> >index 365c03d1c462..1ffecbdd361a 100644
> >--- a/drivers/net/ethernet/intel/ice/ice.h
> >+++ b/drivers/net/ethernet/intel/ice/ice.h
> >@@ -77,6 +77,7 @@
> > #include "ice_gnss.h"
> > #include "ice_irq.h"
> > #include "ice_dpll.h"
> >+#include "ice_adapter.h"
> >
> > #define ICE_BAR0 0
> > #define ICE_REQ_DESC_MULTIPLE 32
> >@@ -544,6 +545,7 @@ struct ice_agg_node {
> >
> > struct ice_pf {
> > struct pci_dev *pdev;
> >+ struct ice_adapter *adapter;
> >
> > struct devlink_region *nvm_region;
> > struct devlink_region *sram_region;
> >diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
> >new file mode 100644
> >index 000000000000..b93b4db4c04c
> >--- /dev/null
> >+++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
> >@@ -0,0 +1,85 @@
> >+// SPDX-License-Identifier: GPL-2.0-only
> >+// SPDX-FileCopyrightText: Copyright Red Hat
> >+
> >+#include <linux/cleanup.h>
> >+#include <linux/mutex.h>
> >+#include <linux/pci.h>
> >+#include <linux/slab.h>
> >+#include <linux/xarray.h>
> >+#include "ice_adapter.h"
> >+
> >+static DEFINE_XARRAY(ice_adapters);
> >+
> >+static unsigned long ice_adapter_index(const struct pci_dev *pdev)
> >+{
> >+ unsigned int domain = pci_domain_nr(pdev->bus);
> >+
> >+ WARN_ON((unsigned long)domain >> (BITS_PER_LONG - 13));
> >+ return ((unsigned long)domain << 13) |
> >+ ((unsigned long)pdev->bus->number << 5) |
> >+ PCI_SLOT(pdev->devfn);
> >+}
> >+
> >+static struct ice_adapter *ice_adapter_new(void)
> >+{
> >+ struct ice_adapter *a;
> >+
> >+ a = kzalloc(sizeof(*a), GFP_KERNEL);
> >+ if (!a)
> >+ return NULL;
> >+
> >+ refcount_set(&a->refcount, 1);
> >+
> >+ return a;
> >+}
> >+
> >+static void ice_adapter_free(struct ice_adapter *a)
> >+{
> >+ kfree(a);
> >+}
> >+
> >+DEFINE_FREE(ice_adapter_free, struct ice_adapter*, if (_T) ice_adapter_free(_T))
> >+
> >+struct ice_adapter *ice_adapter_get(const struct pci_dev *pdev)
> >+{
> >+ struct ice_adapter *ret, __free(ice_adapter_free) *a = NULL;
> >+ unsigned long index = ice_adapter_index(pdev);
> >+
> >+ a = ice_adapter_new();
>
> Please consider some non-single-letter variable name.
Alright, I can change the name.
> >+ if (!a)
> >+ return NULL;
> >+
> >+ xa_lock(&ice_adapters);
> >+ ret = __xa_cmpxchg(&ice_adapters, index, NULL, a, GFP_KERNEL);
>
> This is atomic section, can't sleep.
It is not atomic. __xa_cmpxchg releases xa_lock before it allocates
memory, then reacquires it.
> >+ if (xa_is_err(ret)) {
> >+ ret = NULL;
>
> Why don't you propagate err through ERR_PTR() ?
It seemed unnecessary. ENOMEM is the only failure that can possibly
happen. EINVAL could be returned only if attempting to store an
unaligned pointer, which won't happen here.
>
> >+ goto unlock;
> >+ }
> >+ if (ret) {
> >+ refcount_inc(&ret->refcount);
> >+ goto unlock;
> >+ }
> >+ ret = no_free_ptr(a);
> >+unlock:
> >+ xa_unlock(&ice_adapters);
> >+ return ret;
> >+}
> >+
> >+void ice_adapter_put(const struct pci_dev *pdev)
> >+{
> >+ unsigned long index = ice_adapter_index(pdev);
> >+ struct ice_adapter *a;
> >+
> >+ xa_lock(&ice_adapters);
> >+ a = xa_load(&ice_adapters, index);
> >+ if (WARN_ON(!a))
> >+ goto unlock;
> >+
> >+ if (!refcount_dec_and_test(&a->refcount))
> >+ goto unlock;
> >+
> >+ WARN_ON(__xa_erase(&ice_adapters, index) != a);
>
> Nice paranoia level :)
>
>
> >+ ice_adapter_free(a);
> >+unlock:
> >+ xa_unlock(&ice_adapters);
> >+}
> >diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
> >new file mode 100644
> >index 000000000000..cb5a02eb24c1
> >--- /dev/null
> >+++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
> >@@ -0,0 +1,22 @@
> >+/* SPDX-License-Identifier: GPL-2.0-only */
> >+/* SPDX-FileCopyrightText: Copyright Red Hat */
> >+
> >+#ifndef _ICE_ADAPTER_H_
> >+#define _ICE_ADAPTER_H_
> >+
> >+#include <linux/refcount_types.h>
> >+
> >+struct pci_dev;
> >+
> >+/**
> >+ * struct ice_adapter - PCI adapter resources shared across PFs
> >+ * @refcount: Reference count. struct ice_pf objects hold the references.
> >+ */
> >+struct ice_adapter {
> >+ refcount_t refcount;
> >+};
> >+
> >+struct ice_adapter *ice_adapter_get(const struct pci_dev *pdev);
> >+void ice_adapter_put(const struct pci_dev *pdev);
> >+
> >+#endif /* _ICE_ADAPTER_H */
> >diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> >index 8f73ba77e835..413219d81a12 100644
> >--- a/drivers/net/ethernet/intel/ice/ice_main.c
> >+++ b/drivers/net/ethernet/intel/ice/ice_main.c
> >@@ -5093,6 +5093,7 @@ static int
> > ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
> > {
> > struct device *dev = &pdev->dev;
> >+ struct ice_adapter *adapter;
> > struct ice_pf *pf;
> > struct ice_hw *hw;
> > int err;
> >@@ -5145,7 +5146,12 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
> >
> > pci_set_master(pdev);
> >
> >+ adapter = ice_adapter_get(pdev);
> >+ if (!adapter)
> >+ return -ENOMEM;
> >+
> > pf->pdev = pdev;
> >+ pf->adapter = adapter;
> > pci_set_drvdata(pdev, pf);
> > set_bit(ICE_DOWN, pf->state);
> > /* Disable service task until DOWN bit is cleared */
> >@@ -5196,6 +5202,7 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
> > err_load:
> > ice_deinit(pf);
> > err_init:
> >+ ice_adapter_put(pdev);
> > pci_disable_device(pdev);
> > return err;
> > }
> >@@ -5302,6 +5309,7 @@ static void ice_remove(struct pci_dev *pdev)
> > ice_setup_mc_magic_wake(pf);
> > ice_set_wake(pf);
> >
> >+ ice_adapter_put(pdev);
> > pci_disable_device(pdev);
> > }
> >
> >--
> >2.43.2
> >
>
Powered by blists - more mailing lists