quoted

+static void nvme_rdma_add_one(struct ib_device *ib_device);
+static void nvme_rdma_remove_one(struct ib_device *ib_device,
+				void *client_data);

Can we avoid the forward declarations?

Yes, will do.

quoted

+static void nvme_rdma_add_one(struct ib_device *ib_device)
+{
+	/* devices are added dynamically as targets use them */
+}

Is the add callback mandatory in the core?  Would be nice if we could
fix that up in an incremental patch.

We can just not populate the add function.  From ib_register_device():

---
        list_for_each_entry(client, &client_list, list)
                if (client->add && !add_client_context(device, client))
                        client->add(device);
---

quoted

+static void nvme_rdma_remove_one(struct ib_device *ib_device, void

*client_data)

quoted

+{
+	struct nvme_rdma_ctrl *ctrl, *tmp;
+
+	pr_info("Removing resources for device %s\n", ib_device->name);
+
+	mutex_lock(&nvme_rdma_ctrl_mutex);
+	list_for_each_entry_safe(ctrl, tmp, &nvme_rdma_ctrl_list, list) {
+		int delete_ctrl;
+
+		if (ctrl->device->dev != ib_device)
+			continue;

Also not important for now, but we probably should move to a per-device
list here if we move forward with this approach.

quoted

+		mutex_unlock(&nvme_rdma_ctrl_mutex);

After unlocking nvme_rdma_ctrl_mutex we will need to restart the
list walk, as others might have changed the list (e.g. for a different
device).  Maybe we should do the per-device list from the start to
simplify this loop.

How would changing the list by adding/removing controllers for another device
cause this logic here to miss a controller associated with the device being
removed? 

quoted

@@ -2052,6 +2060,7 @@ static void __exit nvme_rdma_cleanup_module(void)
 		__nvme_rdma_del_ctrl(ctrl);
 	mutex_unlock(&nvme_rdma_ctrl_mutex);

+	ib_unregister_client(&nvme_rdma_ib_client);

We shouldn't need the deletion loop above because now ib_unregister_client
takes care of that, right?

Correct.