diff mbox series

[04/22] ice: Register auxiliary device to provide RDMA

Message ID 20210122234827.1353-5-shiraz.saleem@intel.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series Add Intel Ethernet Protocol Driver for RDMA (irdma) | expand

Commit Message

Shiraz Saleem Jan. 22, 2021, 11:48 p.m. UTC
From: Dave Ertman <david.m.ertman@intel.com>

Register ice client auxiliary RDMA device on the auxiliary bus per
PCIe device function for the auxiliary driver (irdma) to attach to.

Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/Kconfig       |  1 +
 drivers/net/ethernet/intel/ice/ice.h     |  1 +
 drivers/net/ethernet/intel/ice/ice_idc.c | 74 +++++++++++++++++++++++++++++++-
 3 files changed, 74 insertions(+), 2 deletions(-)

Comments

Jason Gunthorpe Jan. 25, 2021, 7:09 p.m. UTC | #1
On Fri, Jan 22, 2021 at 05:48:09PM -0600, Shiraz Saleem wrote:
> +static void ice_peer_adev_release(struct device *dev)
> +{
> +	struct iidc_auxiliary_object *abo;
> +	struct auxiliary_device *adev;
> +
> +	adev = container_of(dev, struct auxiliary_device, dev);
> +	abo = container_of(adev, struct iidc_auxiliary_object, adev);

This is just

 container_of(dev, struct iidc_auxiliary_object, adev.dev);

> @@ -1254,20 +1282,37 @@ int ice_init_peer_devices(struct ice_pf *pf)
>  		 * |--> iidc_peer_obj
>  		 * |--> *ice_peer_drv_int
>  		 *
> +		 * iidc_auxiliary_object (container_of parent for adev)
> +		 * |--> auxiliary_device
> +		 * |--> *iidc_peer_obj (pointer from internal struct)
> +		 *
>  		 * ice_peer_drv_int (internal only peer_drv struct)
>  		 */
>  		peer_obj_int = kzalloc(sizeof(*peer_obj_int), GFP_KERNEL);
> -		if (!peer_obj_int)
> +		if (!peer_obj_int) {
> +			ida_simple_remove(&ice_peer_ida, id);
>  			return -ENOMEM;
> +		}

Why is this allocated memory with a lifetime different from the aux
device?

This whole peer_dev/aux_dev split needs to go, why on earth does
peer_obj need an entire state machine for driver binding? This is what
the aux device and driver core or supposed to provide.

> +		abo = kzalloc(sizeof(*abo), GFP_KERNEL);
> +		if (!abo) {
> +			ida_simple_remove(&ice_peer_ida, id);
> +			kfree(peer_obj_int);
> +			return -ENOMEM;
> +		}

Put the auxiliary_device_init() directly after kzalloc.

Even better is to put everything up to the
kzalloc/auxiliary_device_init() into a function called
'alloc_aux_device'

Then all the error unwind here doesn't look so bad

Jason
Shiraz Saleem Feb. 5, 2021, 3:23 p.m. UTC | #2
> Subject: Re: [PATCH 04/22] ice: Register auxiliary device to provide RDMA
>  
> > @@ -1254,20 +1282,37 @@ int ice_init_peer_devices(struct ice_pf *pf)
> >  		 * |--> iidc_peer_obj
> >  		 * |--> *ice_peer_drv_int
> >  		 *
> > +		 * iidc_auxiliary_object (container_of parent for adev)
> > +		 * |--> auxiliary_device
> > +		 * |--> *iidc_peer_obj (pointer from internal struct)
> > +		 *
> >  		 * ice_peer_drv_int (internal only peer_drv struct)
> >  		 */
> >  		peer_obj_int = kzalloc(sizeof(*peer_obj_int), GFP_KERNEL);
> > -		if (!peer_obj_int)
> > +		if (!peer_obj_int) {
> > +			ida_simple_remove(&ice_peer_ida, id);
> >  			return -ENOMEM;
> > +		}
> 
> Why is this allocated memory with a lifetime different from the aux device?

This ice_peer_obj_int is the PCI driver internal only info about the peer_obj (not exposed externally)
like the state machine, per PF. But Dave is re-writing all of this with the feedback about getting rid
of state machine, and this peer_obj_int will likely be culled.

I think what we will end up with is an iidc_peer_obj per PF which is exported to aux driver with lifetime as described below.

/* structure layout needed for container_of's looks like:  
                  * iidc_auxiliary_dev (container_of parent for adev)
                  * |--> auxiliary_device
                  * |--> *iidc_peer_obj (pointer from peer_obj struct)
                  *
                  * The iidc_auxiliary device has a lifespan as long as it is
                  * on the bus.  Once removed it will be freed and a new
                  * one allocated if needed to re-add.
                  *
                  * The peer_obj is tied to the life of the PF, and will
                  * exist as long as the PF driver is loaded.  It will be
                  * freed in the remove flow for the PF driver.
                  */
Jason Gunthorpe Feb. 5, 2021, 3:27 p.m. UTC | #3
On Fri, Feb 05, 2021 at 03:23:12PM +0000, Saleem, Shiraz wrote:
> > Subject: Re: [PATCH 04/22] ice: Register auxiliary device to provide RDMA
> >  
> > > @@ -1254,20 +1282,37 @@ int ice_init_peer_devices(struct ice_pf *pf)
> > >  		 * |--> iidc_peer_obj
> > >  		 * |--> *ice_peer_drv_int
> > >  		 *
> > > +		 * iidc_auxiliary_object (container_of parent for adev)
> > > +		 * |--> auxiliary_device
> > > +		 * |--> *iidc_peer_obj (pointer from internal struct)
> > > +		 *
> > >  		 * ice_peer_drv_int (internal only peer_drv struct)
> > >  		 */
> > >  		peer_obj_int = kzalloc(sizeof(*peer_obj_int), GFP_KERNEL);
> > > -		if (!peer_obj_int)
> > > +		if (!peer_obj_int) {
> > > +			ida_simple_remove(&ice_peer_ida, id);
> > >  			return -ENOMEM;
> > > +		}
> > 
> > Why is this allocated memory with a lifetime different from the aux device?
> 
> This ice_peer_obj_int is the PCI driver internal only info about the peer_obj (not exposed externally)
> like the state machine, per PF. But Dave is re-writing all of this with the feedback about getting rid
> of state machine, and this peer_obj_int will likely be culled.
> 
> I think what we will end up with is an iidc_peer_obj per PF which is
> exported to aux driver with lifetime as described below.

I wouldn't call it 'peer' anything, this object represents the
programming API of the PCI device. The object and the API should be
understandable from the header files

A good design will have netdev also sit on this programming API, even
if it doesn't have the aux device. mlx5 used mlx5_core as the name,
I'd suggest something similar.

Jason
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
index 5aa8631..cbc5968 100644
--- a/drivers/net/ethernet/intel/Kconfig
+++ b/drivers/net/ethernet/intel/Kconfig
@@ -294,6 +294,7 @@  config ICE
 	tristate "Intel(R) Ethernet Connection E800 Series Support"
 	default n
 	depends on PCI_MSI
+	select AUXILIARY_BUS
 	select NET_DEVLINK
 	select PLDMFW
 	help
diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index b79ffdc..8bf16f4 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -34,6 +34,7 @@ 
 #include <linux/if_bridge.h>
 #include <linux/ctype.h>
 #include <linux/bpf.h>
+#include <linux/auxiliary_bus.h>
 #include <linux/avf/virtchnl.h>
 #include <linux/cpu_rmap.h>
 #include <net/devlink.h>
diff --git a/drivers/net/ethernet/intel/ice/ice_idc.c b/drivers/net/ethernet/intel/ice/ice_idc.c
index e7dd958..703c6bd 100644
--- a/drivers/net/ethernet/intel/ice/ice_idc.c
+++ b/drivers/net/ethernet/intel/ice/ice_idc.c
@@ -6,6 +6,8 @@ 
 #include "ice_lib.h"
 #include "ice_dcb_lib.h"
 
+static DEFINE_IDA(ice_peer_ida);
+
 static struct peer_obj_id ice_peers[] = ASSIGN_PEER_INFO;
 
 /**
@@ -484,6 +486,9 @@  static void ice_check_peer_drv_for_events(struct iidc_peer_obj *peer_obj)
 	if (!peer_obj_int)
 		return 0;
 
+	auxiliary_device_delete(peer_obj_int->peer_obj.adev);
+	auxiliary_device_uninit(peer_obj_int->peer_obj.adev);
+
 	peer_drv_int = peer_obj_int->peer_drv_int;
 
 	if (peer_obj_int->ice_peer_wq) {
@@ -1220,6 +1225,20 @@  int ice_peer_update_vsi(struct ice_peer_obj_int *peer_obj_int, void *data)
 };
 
 /**
+ * ice_peer_adev_release - function to map to aux device's release callback
+ * @dev: pointer to device to free
+ */
+static void ice_peer_adev_release(struct device *dev)
+{
+	struct iidc_auxiliary_object *abo;
+	struct auxiliary_device *adev;
+
+	adev = container_of(dev, struct auxiliary_device, dev);
+	abo = container_of(adev, struct iidc_auxiliary_object, adev);
+	kfree(abo);
+}
+
+/**
  * ice_init_peer_devices - initializes peer objects and aux devices
  * @pf: ptr to ice_pf
  *
@@ -1232,7 +1251,7 @@  int ice_init_peer_devices(struct ice_pf *pf)
 	struct pci_dev *pdev = pf->pdev;
 	struct device *dev = &pdev->dev;
 	unsigned int i;
-	int ret;
+	int id, ret;
 
 	/* Reserve vector resources */
 	ret = ice_reserve_peer_qvector(pf);
@@ -1241,12 +1260,21 @@  int ice_init_peer_devices(struct ice_pf *pf)
 		return ret;
 	}
 
+	/* This PFs auxiliary ID value */
+	id = ida_alloc(&ice_peer_ida, GFP_KERNEL);
+	if (id < 0) {
+		dev_err(dev, "failed to allocate device ID for peers\n");
+		return -ENOMEM;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(ice_peers); i++) {
 		struct ice_peer_obj_int *peer_obj_int;
 		struct ice_peer_drv_int *peer_drv_int;
+		struct iidc_auxiliary_object *abo;
 		struct iidc_qos_params *qos_info;
 		struct msix_entry *entry = NULL;
 		struct iidc_peer_obj *peer_obj;
+		struct auxiliary_device *adev;
 		int j;
 
 		/* structure layout needed for container_of's looks like:
@@ -1254,20 +1282,37 @@  int ice_init_peer_devices(struct ice_pf *pf)
 		 * |--> iidc_peer_obj
 		 * |--> *ice_peer_drv_int
 		 *
+		 * iidc_auxiliary_object (container_of parent for adev)
+		 * |--> auxiliary_device
+		 * |--> *iidc_peer_obj (pointer from internal struct)
+		 *
 		 * ice_peer_drv_int (internal only peer_drv struct)
 		 */
 		peer_obj_int = kzalloc(sizeof(*peer_obj_int), GFP_KERNEL);
-		if (!peer_obj_int)
+		if (!peer_obj_int) {
+			ida_simple_remove(&ice_peer_ida, id);
 			return -ENOMEM;
+		}
+
+		abo = kzalloc(sizeof(*abo), GFP_KERNEL);
+		if (!abo) {
+			ida_simple_remove(&ice_peer_ida, id);
+			kfree(peer_obj_int);
+			return -ENOMEM;
+		}
 
 		peer_drv_int = kzalloc(sizeof(*peer_drv_int), GFP_KERNEL);
 		if (!peer_drv_int) {
+			ida_simple_remove(&ice_peer_ida, id);
 			kfree(peer_obj_int);
+			kfree(abo);
 			return -ENOMEM;
 		}
 
 		pf->peers[i] = peer_obj_int;
+		abo->peer_obj = ice_get_peer_obj(peer_obj_int);
 		peer_obj_int->peer_drv_int = peer_drv_int;
+		peer_obj_int->peer_obj.adev = &abo->adev;
 
 		/* Initialize driver values */
 		for (j = 0; j < IIDC_EVENT_NBITS; j++)
@@ -1289,8 +1334,10 @@  int ice_init_peer_devices(struct ice_pf *pf)
 			alloc_ordered_workqueue("ice_peer_wq_%d", WQ_UNBOUND,
 						i);
 		if (!peer_obj_int->ice_peer_wq) {
+			ida_simple_remove(&ice_peer_ida, id);
 			kfree(peer_obj_int);
 			kfree(peer_drv_int);
+			kfree(abo);
 			return -ENOMEM;
 		}
 		INIT_WORK(&peer_obj_int->peer_close_task, ice_peer_close_task);
@@ -1342,6 +1389,27 @@  int ice_init_peer_devices(struct ice_pf *pf)
 		peer_obj->msix_entries = entry;
 		ice_peer_state_change(peer_obj_int, ICE_PEER_OBJ_STATE_INIT,
 				      false);
+
+		adev = &abo->adev;
+		adev->name = ice_peers[i].name;
+		adev->id = id;
+		adev->dev.release = ice_peer_adev_release;
+		adev->dev.parent = &pdev->dev;
+
+		ret = auxiliary_device_init(adev);
+		if (ret) {
+			ida_simple_remove(&ice_peer_ida, id);
+			kfree(peer_obj_int);
+			kfree(peer_drv_int);
+			adev = NULL;
+			return ret;
+		}
+
+		ret = auxiliary_device_add(adev);
+		if (ret) {
+			auxiliary_device_uninit(adev);
+			return ret;
+		}
 	}
 
 	return ret;
@@ -1357,4 +1425,6 @@  void ice_uninit_peer_devices(struct ice_pf *pf)
 		ice_for_each_peer(pf, NULL, ice_unreg_peer_obj);
 		devm_kfree(&pf->pdev->dev, pf->peers);
 	}
+
+	ida_destroy(&ice_peer_ida);
 }