| Message ID | 20240306162907.84247-2-mschmidt@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ice: lighter locking for PTP time reading | expand |
Wed, Mar 06, 2024 at 05:29:05PM CET, mschmidt@redhat.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@redhat.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. >+ 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. >+ if (xa_is_err(ret)) { >+ ret = NULL; Why don't you propagate err through ERR_PTR() ? >+ 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 >
On Wed, Mar 6, 2024 at 6:00 PM Jiri Pirko <jiri@resnulli.us> wrote: > Wed, Mar 06, 2024 at 05:29:05PM CET, mschmidt@redhat.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@redhat.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 > > >
Wed, Mar 06, 2024 at 08:20:33PM CET, mschmidt@redhat.com wrote: >On Wed, Mar 6, 2024 at 6:00 PM Jiri Pirko <jiri@resnulli.us> wrote: >> Wed, Mar 06, 2024 at 05:29:05PM CET, mschmidt@redhat.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@redhat.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. Ah, cool. > >> >+ 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. Yeah, the point is that you have valid err, you toss it out, the caller then does: adapter = ice_adapter_get(pdev); if (!adapter) return -ENOMEM; And reinvents err. So my point was to propagate it through. > >> >> >+ 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 >> > >> >
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(); + if (!a) + return NULL; + + xa_lock(&ice_adapters); + ret = __xa_cmpxchg(&ice_adapters, index, NULL, a, GFP_KERNEL); + if (xa_is_err(ret)) { + ret = NULL; + 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); + 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); }
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@redhat.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