linux-rdma.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [net-next] i40iw/i40e: Remove link dependency on i40e
@ 2018-05-22 20:38 Jeff Kirsher
  2018-05-22 20:56 ` Jason Gunthorpe
  2018-05-23 19:00 ` David Miller
  0 siblings, 2 replies; 11+ messages in thread
From: Jeff Kirsher @ 2018-05-22 20:38 UTC (permalink / raw)
  To: davem, dledford, jgg
  Cc: Sindhu Devale, netdev, linux-rdma, nhorman, sassmann, jogreene,
	Shiraz Saleem, Jeff Kirsher

From: Sindhu Devale <sindhu.devale@intel.com>

Currently i40iw is dependent on i40e symbols
i40e_register_client and i40e_unregister_client due to
which i40iw cannot be loaded without i40e being loaded.

This patch allows RDMA driver to build and load without
linking to LAN driver and without LAN driver being loaded
first. Once the LAN driver is loaded, the RDMA driver
is notified through the netdevice notifiers to register
as client to the LAN driver. Add function pointers to IDC
register/unregister in the private VSI structure. This
allows a RDMA driver to build without linking to i40e.

Signed-off-by: Sindhu Devale <sindhu.devale@intel.com>
Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/infiniband/hw/i40iw/i40iw.h           |  24 +++
 drivers/infiniband/hw/i40iw/i40iw_main.c      | 141 ++++++++++++++++--
 drivers/infiniband/hw/i40iw/i40iw_utils.c     |   5 +-
 drivers/net/ethernet/intel/i40e/i40e.h        |   1 +
 drivers/net/ethernet/intel/i40e/i40e_client.h |   9 ++
 drivers/net/ethernet/intel/i40e/i40e_main.c   |   6 +
 6 files changed, 173 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/hw/i40iw/i40iw.h b/drivers/infiniband/hw/i40iw/i40iw.h
index d5d8c1be345a..c6398b73a8da 100644
--- a/drivers/infiniband/hw/i40iw/i40iw.h
+++ b/drivers/infiniband/hw/i40iw/i40iw.h
@@ -119,6 +119,30 @@
 #define I40IW_CQP_COMPL_SQ_WQE_FLUSHED    3
 #define I40IW_CQP_COMPL_RQ_SQ_WQE_FLUSHED 4
 
+enum I40IW_IDC_STATE {
+	I40IW_STATE_INVALID,
+	I40IW_STATE_VALID,
+	I40IW_STATE_REG_FAILED
+};
+
+struct i40iw_peer {
+	struct module *module;
+#define MAX_PEER_NAME_SIZE 8
+	char name[MAX_PEER_NAME_SIZE];
+	enum I40IW_IDC_STATE state;
+	atomic_t ref_count;
+	int (*idc_reg_peer_driver)(struct i40e_client *i40iw_client);
+	int (*idc_unreg_peer_driver)(struct i40e_client *i40iw_client);
+};
+
+struct i40iw_peer_drv {
+	struct i40e_client i40iw_client;
+	struct i40iw_peer peer;
+};
+
+bool i40iw_is_new_peer(struct net_device *netdev);
+void i40iw_reg_peer(void);
+
 struct i40iw_cqp_compl_info {
 	u32 op_ret_val;
 	u16 maj_err_code;
diff --git a/drivers/infiniband/hw/i40iw/i40iw_main.c b/drivers/infiniband/hw/i40iw/i40iw_main.c
index 9cd0d3ef9057..f4c5be11c1d4 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_main.c
+++ b/drivers/infiniband/hw/i40iw/i40iw_main.c
@@ -78,6 +78,7 @@ MODULE_AUTHOR("Intel Corporation, <e1000-rdma@lists.sourceforge.net>");
 MODULE_DESCRIPTION("Intel(R) Ethernet Connection X722 iWARP RDMA Driver");
 MODULE_LICENSE("Dual BSD/GPL");
 
+static struct i40iw_peer_drv peer_drv;
 static struct i40e_client i40iw_client;
 static char i40iw_client_name[I40E_CLIENT_STR_LENGTH] = "i40iw";
 
@@ -103,6 +104,30 @@ static struct notifier_block i40iw_netdevice_notifier = {
 	.notifier_call = i40iw_netdevice_event
 };
 
+/**
+ * i40iw_open_inc_ref - Increment ref count for a open
+ */
+static void i40iw_open_inc_ref(void)
+{
+	atomic_inc(&peer_drv.peer.ref_count);
+}
+
+/**
+ * i40iw_open_dec_ref - Decrement ref count for a open
+ */
+static void i40iw_open_dec_ref(void)
+{
+	struct i40iw_peer *peer;
+
+	peer = &peer_drv.peer;
+	if (peer->state == I40IW_STATE_VALID &&
+	    atomic_dec_and_test(&peer->ref_count)) {
+		peer->state = I40IW_STATE_INVALID;
+		peer->idc_unreg_peer_driver(&peer_drv.i40iw_client);
+		module_put(peer->module);
+	}
+}
+
 /**
  * i40iw_find_i40e_handler - find a handler given a client info
  * @ldev: pointer to a client info
@@ -1710,6 +1735,7 @@ static int i40iw_open(struct i40e_info *ldev, struct i40e_client *client)
 		if(iwdev->param_wq == NULL)
 			break;
 		i40iw_pr_info("i40iw_open completed\n");
+		i40iw_open_inc_ref();
 		return 0;
 	} while (0);
 
@@ -1801,6 +1827,7 @@ static void i40iw_close(struct i40e_info *ldev, struct i40e_client *client, bool
 	i40iw_cm_teardown_connections(iwdev, NULL, NULL, true);
 	destroy_workqueue(iwdev->virtchnl_wq);
 	i40iw_deinit_device(iwdev);
+	i40iw_open_dec_ref();
 }
 
 /**
@@ -2024,6 +2051,104 @@ static const struct i40e_client_ops i40e_ops = {
 	.vf_capable = i40iw_vf_capable
 };
 
+/**
+ * i40iw_is_new_peer - check netdev of the peer driver
+ * @netdev: netdev of peer driver
+ */
+bool i40iw_is_new_peer(struct net_device *netdev)
+{
+	struct idc_srv_provider *sp;
+	struct i40iw_peer *peer;
+
+	peer = &peer_drv.peer;
+	if (peer->state == I40IW_STATE_VALID)
+		return false;
+
+	if (netdev->dev.parent && netdev->dev.parent->driver &&
+	    !strncmp(netdev->dev.parent->driver->name, peer->name, sizeof(peer->name))) {
+		sp = (struct idc_srv_provider *)netdev_priv(netdev);
+		if (sp->signature != IDC_SIGNATURE || sp->version)
+			return false;
+
+		/* Found the driver */
+		peer->idc_reg_peer_driver = sp->idc_reg_peer_driver;
+		peer->idc_unreg_peer_driver = sp->idc_unreg_peer_driver;
+		peer->module = netdev->dev.parent->driver->owner;
+
+		return true;
+	}
+
+	return false;
+}
+
+/**
+ * i40iw_initialize_client - Setup client struct
+ */
+static void i40iw_initialize_client(void)
+{
+	struct i40e_client *i40iw_client = &peer_drv.i40iw_client;
+
+	i40iw_client->version.major = CLIENT_IW_INTERFACE_VERSION_MAJOR;
+	i40iw_client->version.minor = CLIENT_IW_INTERFACE_VERSION_MINOR;
+	i40iw_client->version.build = CLIENT_IW_INTERFACE_VERSION_BUILD;
+	i40iw_client->ops = &i40e_ops;
+	memcpy(i40iw_client->name, i40iw_client_name, I40E_CLIENT_STR_LENGTH);
+	i40iw_client->type = I40E_CLIENT_IWARP;
+	strncpy(peer_drv.peer.name, "i40e", sizeof(peer_drv.peer.name));
+}
+
+/**
+ * i40iw_reg_peer - Register with peer
+ */
+void i40iw_reg_peer(void)
+{
+	struct i40iw_peer *peer;
+
+	peer = &peer_drv.peer;
+
+	if (peer->state == I40IW_STATE_VALID)
+		return;
+
+	if (peer->idc_reg_peer_driver &&
+	    !peer->idc_reg_peer_driver(&peer_drv.i40iw_client)) {
+		peer->state = I40IW_STATE_VALID;
+		try_module_get(peer->module);
+	} else {
+		peer->state = I40IW_STATE_REG_FAILED;
+	}
+}
+
+/**
+ * i40iw_find_idc_peer - Search netdevs for a peer driver
+ */
+static void i40iw_find_idc_peer(void)
+{
+	struct net_device *dev;
+
+	rcu_read_lock();
+	for_each_netdev_rcu(&init_net, dev) {
+		if (i40iw_is_new_peer(dev))
+			break;
+	}
+	rcu_read_unlock();
+	i40iw_reg_peer();
+}
+
+/**
+ * i40iw_unreg_peer - Unregister with peer
+ */
+static void i40iw_unreg_peer(void)
+{
+	struct i40iw_peer *peer;
+
+	peer = &peer_drv.peer;
+	if (peer->state == I40IW_STATE_VALID) {
+		peer->state = I40IW_STATE_INVALID;
+		peer->idc_unreg_peer_driver(&peer_drv.i40iw_client);
+		module_put(peer->module);
+	}
+}
+
 /**
  * i40iw_init_module - driver initialization function
  *
@@ -2032,20 +2157,12 @@ static const struct i40e_client_ops i40e_ops = {
  */
 static int __init i40iw_init_module(void)
 {
-	int ret;
-
-	memset(&i40iw_client, 0, sizeof(i40iw_client));
-	i40iw_client.version.major = CLIENT_IW_INTERFACE_VERSION_MAJOR;
-	i40iw_client.version.minor = CLIENT_IW_INTERFACE_VERSION_MINOR;
-	i40iw_client.version.build = CLIENT_IW_INTERFACE_VERSION_BUILD;
-	i40iw_client.ops = &i40e_ops;
-	memcpy(i40iw_client.name, i40iw_client_name, I40E_CLIENT_STR_LENGTH);
-	i40iw_client.type = I40E_CLIENT_IWARP;
 	spin_lock_init(&i40iw_handler_lock);
-	ret = i40e_register_client(&i40iw_client);
+	i40iw_initialize_client();
+	i40iw_find_idc_peer();
 	i40iw_register_notifiers();
 
-	return ret;
+	return 0;
 }
 
 /**
@@ -2057,7 +2174,7 @@ static int __init i40iw_init_module(void)
 static void __exit i40iw_exit_module(void)
 {
 	i40iw_unregister_notifiers();
-	i40e_unregister_client(&i40iw_client);
+	i40iw_unreg_peer();
 }
 
 module_init(i40iw_init_module);
diff --git a/drivers/infiniband/hw/i40iw/i40iw_utils.c b/drivers/infiniband/hw/i40iw/i40iw_utils.c
index a9ea966877f2..264939942da0 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_utils.c
+++ b/drivers/infiniband/hw/i40iw/i40iw_utils.c
@@ -314,8 +314,11 @@ int i40iw_netdevice_event(struct notifier_block *notifier,
 	event_netdev = netdev_notifier_info_to_dev(ptr);
 
 	hdl = i40iw_find_netdev(event_netdev);
-	if (!hdl)
+	if (!hdl) {
+		if (i40iw_is_new_peer(event_netdev))
+			i40iw_reg_peer();
 		return NOTIFY_DONE;
+	}
 
 	iwdev = &hdl->device;
 	if (iwdev->init_state < RDMA_DEV_REGISTERED || iwdev->closing)
diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index 7a80652e2500..e3171b696848 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -789,6 +789,7 @@ struct i40e_vsi {
 } ____cacheline_internodealigned_in_smp;
 
 struct i40e_netdev_priv {
+	struct idc_srv_provider prov_callbacks;
 	struct i40e_vsi *vsi;
 };
 
diff --git a/drivers/net/ethernet/intel/i40e/i40e_client.h b/drivers/net/ethernet/intel/i40e/i40e_client.h
index 72994baf4941..95a47df9c104 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_client.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_client.h
@@ -44,6 +44,15 @@ struct i40e_client;
 #define I40E_QUEUE_TYPE_PE_AEQ  0x80
 #define I40E_QUEUE_INVALID_IDX	0xFFFF
 
+#define IDC_SIGNATURE 0x494e54454c494443ULL	/* INTELIDC */
+struct idc_srv_provider {
+	u64 signature;
+	u8 version;
+	u8 rsvd[7];
+	int (*idc_reg_peer_driver)(struct i40e_client *client);
+	int (*idc_unreg_peer_driver)(struct i40e_client *client);
+};
+
 struct i40e_qv_info {
 	u32 v_idx; /* msix_vector */
 	u16 ceq_idx;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index b5daa5c9c7de..984001ae7680 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -11913,6 +11913,12 @@ static int i40e_config_netdev(struct i40e_vsi *vsi)
 	np = netdev_priv(netdev);
 	np->vsi = vsi;
 
+	np->prov_callbacks.signature = IDC_SIGNATURE;
+	np->prov_callbacks.version = 0;
+	memset(np->prov_callbacks.rsvd, 0, sizeof(np->prov_callbacks.rsvd));
+	np->prov_callbacks.idc_reg_peer_driver = i40e_register_client;
+	np->prov_callbacks.idc_unreg_peer_driver = i40e_unregister_client;
+
 	hw_enc_features = NETIF_F_SG			|
 			  NETIF_F_IP_CSUM		|
 			  NETIF_F_IPV6_CSUM		|
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [net-next] i40iw/i40e: Remove link dependency on i40e
  2018-05-22 20:38 [net-next] i40iw/i40e: Remove link dependency on i40e Jeff Kirsher
@ 2018-05-22 20:56 ` Jason Gunthorpe
  2018-05-22 21:04   ` Jeff Kirsher
  2018-05-23 19:00 ` David Miller
  1 sibling, 1 reply; 11+ messages in thread
From: Jason Gunthorpe @ 2018-05-22 20:56 UTC (permalink / raw)
  To: Jeff Kirsher
  Cc: davem, dledford, Sindhu Devale, netdev, linux-rdma, nhorman,
	sassmann, jogreene, Shiraz Saleem

On Tue, May 22, 2018 at 01:38:31PM -0700, Jeff Kirsher wrote:
> From: Sindhu Devale <sindhu.devale@intel.com>
> 
> Currently i40iw is dependent on i40e symbols
> i40e_register_client and i40e_unregister_client due to
> which i40iw cannot be loaded without i40e being loaded.
> 
> This patch allows RDMA driver to build and load without
> linking to LAN driver and without LAN driver being loaded
> first. Once the LAN driver is loaded, the RDMA driver
> is notified through the netdevice notifiers to register
> as client to the LAN driver. Add function pointers to IDC
> register/unregister in the private VSI structure. This
> allows a RDMA driver to build without linking to i40e.

Why would you want to do this? The rdma driver is non-functional
without the ethernet driver, so why on earth would we want to defeat
the module dependency mechanism?

Jason

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [net-next] i40iw/i40e: Remove link dependency on i40e
  2018-05-22 20:56 ` Jason Gunthorpe
@ 2018-05-22 21:04   ` Jeff Kirsher
  2018-05-22 21:33     ` Jason Gunthorpe
  2018-05-23  6:19     ` Christoph Hellwig
  0 siblings, 2 replies; 11+ messages in thread
From: Jeff Kirsher @ 2018-05-22 21:04 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: davem, dledford, Sindhu Devale, netdev, linux-rdma, nhorman,
	sassmann, jogreene, Shiraz Saleem

[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]

On Tue, 2018-05-22 at 14:56 -0600, Jason Gunthorpe wrote:
> On Tue, May 22, 2018 at 01:38:31PM -0700, Jeff Kirsher wrote:
> > From: Sindhu Devale <sindhu.devale@intel.com>
> > 
> > Currently i40iw is dependent on i40e symbols
> > i40e_register_client and i40e_unregister_client due to
> > which i40iw cannot be loaded without i40e being loaded.
> > 
> > This patch allows RDMA driver to build and load without
> > linking to LAN driver and without LAN driver being loaded
> > first. Once the LAN driver is loaded, the RDMA driver
> > is notified through the netdevice notifiers to register
> > as client to the LAN driver. Add function pointers to IDC
> > register/unregister in the private VSI structure. This
> > allows a RDMA driver to build without linking to i40e.
> 
> Why would you want to do this? The rdma driver is non-functional
> without the ethernet driver, so why on earth would we want to defeat
> the module dependency mechanism?

This change is driven by the OSV's like Red Hat, where customer's were
updating the i40e driver, which in turn broke i40iw.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [net-next] i40iw/i40e: Remove link dependency on i40e
  2018-05-22 21:04   ` Jeff Kirsher
@ 2018-05-22 21:33     ` Jason Gunthorpe
  2018-05-22 21:50       ` Jeff Kirsher
  2018-05-23  6:19     ` Christoph Hellwig
  1 sibling, 1 reply; 11+ messages in thread
From: Jason Gunthorpe @ 2018-05-22 21:33 UTC (permalink / raw)
  To: Jeff Kirsher
  Cc: davem, dledford, Sindhu Devale, netdev, linux-rdma, nhorman,
	sassmann, jogreene, Shiraz Saleem

On Tue, May 22, 2018 at 02:04:06PM -0700, Jeff Kirsher wrote:
> On Tue, 2018-05-22 at 14:56 -0600, Jason Gunthorpe wrote:
> > On Tue, May 22, 2018 at 01:38:31PM -0700, Jeff Kirsher wrote:
> > > From: Sindhu Devale <sindhu.devale@intel.com>
> > > 
> > > Currently i40iw is dependent on i40e symbols
> > > i40e_register_client and i40e_unregister_client due to
> > > which i40iw cannot be loaded without i40e being loaded.
> > > 
> > > This patch allows RDMA driver to build and load without
> > > linking to LAN driver and without LAN driver being loaded
> > > first. Once the LAN driver is loaded, the RDMA driver
> > > is notified through the netdevice notifiers to register
> > > as client to the LAN driver. Add function pointers to IDC
> > > register/unregister in the private VSI structure. This
> > > allows a RDMA driver to build without linking to i40e.
> > 
> > Why would you want to do this? The rdma driver is non-functional
> > without the ethernet driver, so why on earth would we want to defeat
> > the module dependency mechanism?
> 
> This change is driven by the OSV's like Red Hat, where customer's were
> updating the i40e driver, which in turn broke i40iw.

So that isn't a reason to put something into the main line kernel, and
I'm deeply skeptical this change is even sane.

It has been a while since I've looked at RH's kernel, but AFAIK,
breakage should only happen if the ABIs around
i40e_unregister_client/etc change..

So if the i40e module updates breaks the ABI, then stuffing that same
broken ABI through a function pointer is *totally* wrong.

Looks like a NAK for the RDMA side.

Jason

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [net-next] i40iw/i40e: Remove link dependency on i40e
  2018-05-22 21:33     ` Jason Gunthorpe
@ 2018-05-22 21:50       ` Jeff Kirsher
  2018-05-22 21:54         ` Jason Gunthorpe
  2018-05-23  6:20         ` Christoph Hellwig
  0 siblings, 2 replies; 11+ messages in thread
From: Jeff Kirsher @ 2018-05-22 21:50 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: davem, dledford, Sindhu Devale, netdev, linux-rdma, nhorman,
	sassmann, jogreene, Shiraz Saleem

[-- Attachment #1: Type: text/plain, Size: 1968 bytes --]

On Tue, 2018-05-22 at 15:33 -0600, Jason Gunthorpe wrote:
> On Tue, May 22, 2018 at 02:04:06PM -0700, Jeff Kirsher wrote:
> > On Tue, 2018-05-22 at 14:56 -0600, Jason Gunthorpe wrote:
> > > On Tue, May 22, 2018 at 01:38:31PM -0700, Jeff Kirsher wrote:
> > > > From: Sindhu Devale <sindhu.devale@intel.com>
> > > > 
> > > > Currently i40iw is dependent on i40e symbols
> > > > i40e_register_client and i40e_unregister_client due to
> > > > which i40iw cannot be loaded without i40e being loaded.
> > > > 
> > > > This patch allows RDMA driver to build and load without
> > > > linking to LAN driver and without LAN driver being loaded
> > > > first. Once the LAN driver is loaded, the RDMA driver
> > > > is notified through the netdevice notifiers to register
> > > > as client to the LAN driver. Add function pointers to IDC
> > > > register/unregister in the private VSI structure. This
> > > > allows a RDMA driver to build without linking to i40e.
> > > 
> > > Why would you want to do this? The rdma driver is non-functional
> > > without the ethernet driver, so why on earth would we want to
> > > defeat
> > > the module dependency mechanism?
> > 
> > This change is driven by the OSV's like Red Hat, where customer's
> > were
> > updating the i40e driver, which in turn broke i40iw.
> 
> So that isn't a reason to put something into the main line kernel,
> and
> I'm deeply skeptical this change is even sane.
> 
> It has been a while since I've looked at RH's kernel, but AFAIK,
> breakage should only happen if the ABIs around
> i40e_unregister_client/etc change..
> 
> So if the i40e module updates breaks the ABI, then stuffing that same
> broken ABI through a function pointer is *totally* wrong.
> 
> Looks like a NAK for the RDMA side.

The ABI rarely changes, if at all.  The issue OSV's are seeing is that
upgrading i40e, requires that i40iw be recompiled even though there
were no updates/changes to the ABI.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [net-next] i40iw/i40e: Remove link dependency on i40e
  2018-05-22 21:50       ` Jeff Kirsher
@ 2018-05-22 21:54         ` Jason Gunthorpe
  2018-05-23  6:20         ` Christoph Hellwig
  1 sibling, 0 replies; 11+ messages in thread
From: Jason Gunthorpe @ 2018-05-22 21:54 UTC (permalink / raw)
  To: Jeff Kirsher
  Cc: davem, dledford, Sindhu Devale, netdev, linux-rdma, nhorman,
	sassmann, jogreene, Shiraz Saleem

On Tue, May 22, 2018 at 02:50:32PM -0700, Jeff Kirsher wrote:
> On Tue, 2018-05-22 at 15:33 -0600, Jason Gunthorpe wrote:
> > On Tue, May 22, 2018 at 02:04:06PM -0700, Jeff Kirsher wrote:
> > > On Tue, 2018-05-22 at 14:56 -0600, Jason Gunthorpe wrote:
> > > > On Tue, May 22, 2018 at 01:38:31PM -0700, Jeff Kirsher wrote:
> > > > > From: Sindhu Devale <sindhu.devale@intel.com>
> > > > > 
> > > > > Currently i40iw is dependent on i40e symbols
> > > > > i40e_register_client and i40e_unregister_client due to
> > > > > which i40iw cannot be loaded without i40e being loaded.
> > > > > 
> > > > > This patch allows RDMA driver to build and load without
> > > > > linking to LAN driver and without LAN driver being loaded
> > > > > first. Once the LAN driver is loaded, the RDMA driver
> > > > > is notified through the netdevice notifiers to register
> > > > > as client to the LAN driver. Add function pointers to IDC
> > > > > register/unregister in the private VSI structure. This
> > > > > allows a RDMA driver to build without linking to i40e.
> > > > 
> > > > Why would you want to do this? The rdma driver is non-functional
> > > > without the ethernet driver, so why on earth would we want to
> > > > defeat
> > > > the module dependency mechanism?
> > > 
> > > This change is driven by the OSV's like Red Hat, where customer's
> > > were
> > > updating the i40e driver, which in turn broke i40iw.
> > 
> > So that isn't a reason to put something into the main line kernel,
> > and
> > I'm deeply skeptical this change is even sane.
> > 
> > It has been a while since I've looked at RH's kernel, but AFAIK,
> > breakage should only happen if the ABIs around
> > i40e_unregister_client/etc change..
> > 
> > So if the i40e module updates breaks the ABI, then stuffing that same
> > broken ABI through a function pointer is *totally* wrong.
> > 
> > Looks like a NAK for the RDMA side.
> 
> The ABI rarely changes, if at all.  The issue OSV's are seeing is that
> upgrading i40e, requires that i40iw be recompiled even though there
> were no updates/changes to the ABI.

Why does it need recompiling? Details please.

Jason

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [net-next] i40iw/i40e: Remove link dependency on i40e
  2018-05-22 21:04   ` Jeff Kirsher
  2018-05-22 21:33     ` Jason Gunthorpe
@ 2018-05-23  6:19     ` Christoph Hellwig
  2018-05-23 15:03       ` Alexander Duyck
  1 sibling, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2018-05-23  6:19 UTC (permalink / raw)
  To: Jeff Kirsher
  Cc: Jason Gunthorpe, davem, dledford, Sindhu Devale, netdev,
	linux-rdma, nhorman, sassmann, jogreene, Shiraz Saleem

On Tue, May 22, 2018 at 02:04:06PM -0700, Jeff Kirsher wrote:
> > Why would you want to do this? The rdma driver is non-functional
> > without the ethernet driver, so why on earth would we want to defeat
> > the module dependency mechanism?
> 
> This change is driven by the OSV's like Red Hat, where customer's were
> updating the i40e driver, which in turn broke i40iw.

Doctor it hurts when I do this..

There is no reason to make a mess of our drivers because people are
doing things they should haver never done and that aren't supported
in Linux.

If Intel didn;t offer any out of tree drivers I'm pretty sure no
customer would even attempt this.  So fix this where the problem is.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [net-next] i40iw/i40e: Remove link dependency on i40e
  2018-05-22 21:50       ` Jeff Kirsher
  2018-05-22 21:54         ` Jason Gunthorpe
@ 2018-05-23  6:20         ` Christoph Hellwig
  1 sibling, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2018-05-23  6:20 UTC (permalink / raw)
  To: Jeff Kirsher
  Cc: Jason Gunthorpe, davem, dledford, Sindhu Devale, netdev,
	linux-rdma, nhorman, sassmann, jogreene, Shiraz Saleem

On Tue, May 22, 2018 at 02:50:32PM -0700, Jeff Kirsher wrote:
> The ABI rarely changes, if at all.  The issue OSV's are seeing is that
> upgrading i40e, requires that i40iw be recompiled even though there
> were no updates/changes to the ABI.

So fscking what.  If you upgrade one part of the kernel you have to
rebuild anbything.  If people try to get away without that that is
their problem, an certainly not something in any way supported by
Linux.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [net-next] i40iw/i40e: Remove link dependency on i40e
  2018-05-23  6:19     ` Christoph Hellwig
@ 2018-05-23 15:03       ` Alexander Duyck
  2018-05-23 15:18         ` Jason Gunthorpe
  0 siblings, 1 reply; 11+ messages in thread
From: Alexander Duyck @ 2018-05-23 15:03 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jeff Kirsher, Jason Gunthorpe, David Miller, Doug Ledford,
	Sindhu Devale, Netdev, linux-rdma, Neil Horman, Stefan Assmann,
	John Greene, Shiraz Saleem

On Tue, May 22, 2018 at 11:19 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, May 22, 2018 at 02:04:06PM -0700, Jeff Kirsher wrote:
>> > Why would you want to do this? The rdma driver is non-functional
>> > without the ethernet driver, so why on earth would we want to defeat
>> > the module dependency mechanism?
>>
>> This change is driven by the OSV's like Red Hat, where customer's were
>> updating the i40e driver, which in turn broke i40iw.
>
> Doctor it hurts when I do this..
>
> There is no reason to make a mess of our drivers because people are
> doing things they should haver never done and that aren't supported
> in Linux.
>
> If Intel didn;t offer any out of tree drivers I'm pretty sure no
> customer would even attempt this.  So fix this where the problem is.

Are you serious? You are never going to see out-of-tree drivers go
away. They exist for the simple reason that most customers/OSVs are
slow to upgrade their kernels so we have people running on a 3.10
something kernel on their RHEL 7.X and want to use the latest greatest
hardware.

I find it ridiculous that you expect us to support a product with
in-kernel only and it is pretty short sighted to insist that that is
the only way to support a product.

With that said it probably wouldn't hurt to throw a WARN_ONCE in here
somewhere that basically informs the user they are doing something
stupid and essentially disabling the i40iw driver if they remove i40e.

- Alex

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [net-next] i40iw/i40e: Remove link dependency on i40e
  2018-05-23 15:03       ` Alexander Duyck
@ 2018-05-23 15:18         ` Jason Gunthorpe
  0 siblings, 0 replies; 11+ messages in thread
From: Jason Gunthorpe @ 2018-05-23 15:18 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: Christoph Hellwig, Jeff Kirsher, David Miller, Doug Ledford,
	Sindhu Devale, Netdev, linux-rdma, Neil Horman, Stefan Assmann,
	John Greene, Shiraz Saleem

On Wed, May 23, 2018 at 08:03:44AM -0700, Alexander Duyck wrote:
> On Tue, May 22, 2018 at 11:19 PM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Tue, May 22, 2018 at 02:04:06PM -0700, Jeff Kirsher wrote:
> >> > Why would you want to do this? The rdma driver is non-functional
> >> > without the ethernet driver, so why on earth would we want to defeat
> >> > the module dependency mechanism?
> >>
> >> This change is driven by the OSV's like Red Hat, where customer's were
> >> updating the i40e driver, which in turn broke i40iw.
> >
> > Doctor it hurts when I do this..
> >
> > There is no reason to make a mess of our drivers because people are
> > doing things they should haver never done and that aren't supported
> > in Linux.
> >
> > If Intel didn;t offer any out of tree drivers I'm pretty sure no
> > customer would even attempt this.  So fix this where the problem is.
> 
> Are you serious? You are never going to see out-of-tree drivers go
> away. They exist for the simple reason that most customers/OSVs are
> slow to upgrade their kernels so we have people running on a 3.10
> something kernel on their RHEL 7.X and want to use the latest greatest
> hardware.

So provide the i40iw module when providing the i40e upgrade module?

I still can't understand why this is a problem that needs to be
solved in mainline, or why it deserves a special and unique fix to
i40e, or even what the *actual* problem is..

Jason

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [net-next] i40iw/i40e: Remove link dependency on i40e
  2018-05-22 20:38 [net-next] i40iw/i40e: Remove link dependency on i40e Jeff Kirsher
  2018-05-22 20:56 ` Jason Gunthorpe
@ 2018-05-23 19:00 ` David Miller
  1 sibling, 0 replies; 11+ messages in thread
From: David Miller @ 2018-05-23 19:00 UTC (permalink / raw)
  To: jeffrey.t.kirsher
  Cc: dledford, jgg, sindhu.devale, netdev, linux-rdma, nhorman,
	sassmann, jogreene, shiraz.saleem

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Tue, 22 May 2018 13:38:31 -0700

> From: Sindhu Devale <sindhu.devale@intel.com>
> 
> Currently i40iw is dependent on i40e symbols
> i40e_register_client and i40e_unregister_client due to
> which i40iw cannot be loaded without i40e being loaded.
> 
> This patch allows RDMA driver to build and load without
> linking to LAN driver and without LAN driver being loaded
> first. Once the LAN driver is loaded, the RDMA driver
> is notified through the netdevice notifiers to register
> as client to the LAN driver. Add function pointers to IDC
> register/unregister in the private VSI structure. This
> allows a RDMA driver to build without linking to i40e.
> 
> Signed-off-by: Sindhu Devale <sindhu.devale@intel.com>
> Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com>
> Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

If two drivers depend upon eachother, and a change to one can create
an incompatibility with the other, by definition they must be upgraded
together.

This doesn't even get into recompiling or anything like that, it's a
simple fact of life.

I'm not applying this sorry.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-05-23 19:00 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-22 20:38 [net-next] i40iw/i40e: Remove link dependency on i40e Jeff Kirsher
2018-05-22 20:56 ` Jason Gunthorpe
2018-05-22 21:04   ` Jeff Kirsher
2018-05-22 21:33     ` Jason Gunthorpe
2018-05-22 21:50       ` Jeff Kirsher
2018-05-22 21:54         ` Jason Gunthorpe
2018-05-23  6:20         ` Christoph Hellwig
2018-05-23  6:19     ` Christoph Hellwig
2018-05-23 15:03       ` Alexander Duyck
2018-05-23 15:18         ` Jason Gunthorpe
2018-05-23 19:00 ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).