Re: [PATCH net-next 1/3] ice: add ice_adapter for shared data across PFs on the same NIC

[Michal Schmidt <mschmidt@redhat.com>]

On 2/27/24 08:05, Jiri Pirko wrote:
> Mon, Feb 26, 2024 at 04:11:23PM 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 | 67 ++++++++++++++++++++
>> drivers/net/ethernet/intel/ice/ice_adapter.h | 22 +++++++
>> drivers/net/ethernet/intel/ice/ice_main.c    |  8 +++
>> 5 files changed, 101 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..deb063401238
>> --- /dev/null
>> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
>> @@ -0,0 +1,67 @@
>> +// 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_MUTEX(ice_adapters_lock);
> 
> Why you need and extra mutex and not just rely on xarray lock?

I suppose I could use xa_lock() and the __xa_{load,store} calls. 
Alright, let's see what it will look like...
Thanks,
Michal


>> +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);
>> +}
>> +
>> +struct ice_adapter *ice_adapter_get(const struct pci_dev *pdev)
>> +{
>> +	unsigned long index = ice_adapter_index(pdev);
>> +	struct ice_adapter *a;
>> +
>> +	guard(mutex)(&ice_adapters_lock);
>> +
>> +	a = xa_load(&ice_adapters, index);
>> +	if (a) {
>> +		refcount_inc(&a->refcount);
>> +		return a;
>> +	}
>> +
>> +	a = kzalloc(sizeof(*a), GFP_KERNEL);
>> +	if (!a)
>> +		return NULL;
>> +
>> +	refcount_set(&a->refcount, 1);
>> +
>> +	if (xa_is_err(xa_store(&ice_adapters, index, a, GFP_KERNEL))) {
>> +		kfree(a);
>> +		return NULL;
>> +	}
>> +
>> +	return a;
>> +}
>> +
>> +void ice_adapter_put(const struct pci_dev *pdev)
>> +{
>> +	unsigned long index = ice_adapter_index(pdev);
>> +	struct ice_adapter *a;
>> +
>> +	guard(mutex)(&ice_adapters_lock);
>> +
>> +	a = xa_load(&ice_adapters, index);
>> +	if (WARN_ON(!a))
>> +		return;
>> +
>> +	if (!refcount_dec_and_test(&a->refcount))
>> +		return;
>> +
>> +	WARN_ON(xa_erase(&ice_adapters, index) != a);
>> +	kfree(a);
>> +}
>> 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 9c2c8637b4a7..4a60957221fc 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
>>
>>
>