[PATCH v2 0/3] Allow ib_clients to nullify .add but still get .remove events reliably

[PATCH v2 0/3] Allow ib_clients to nullify .add but still get .remove events reliably

[Sagi Grimberg]

Some ULPs are just interested in device removal notification with no specific
association because they work over RDMA_CM so no need for detecting appearance
of the devices. That wasn't the because the corresponding ib_client_data
was never added to the device clients list when the ib_client registers,
Patch 1 fixes it. Patches 2-3 just removes the empty .add callout nvme(t)-rdma set.

Changes from v1:
- reword patch 1 change log title
- collected review tags

Note that this will create a conflict with jens's for-4.13 which we added
ib_client registration to nvmet-rdma.

Sagi Grimberg (3):
  RDMA/core: make ib_device.add method optional
  nvme-rdma: remove redundant empty device add callout
  nvmet-rdma: remove redundant empty device add callout

 drivers/infiniband/core/device.c | 4 ++--
 drivers/nvme/host/rdma.c         | 5 -----
 drivers/nvme/target/rdma.c       | 5 -----
 3 files changed, 2 insertions(+), 12 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] RDMA/core: make ib_device.add method optional

[Sagi Grimberg]

ib_clients can indeed fill .add to NULL, but then they will not see
any device removal notifications. The reason is that that
ib_register_client and ib_register_device checked existence of .add
before adding the creating a corresponding client_data and adding
it to the list. Simple condition reverse fixes the issue.

Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
Reviewed-by: Christoph Hellwig <hch at lst.de>
Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/infiniband/core/device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 81d447da0048..26eb428af473 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -414,7 +414,7 @@ int ib_register_device(struct ib_device *device,
 	device->reg_state = IB_DEV_REGISTERED;
 
 	list_for_each_entry(client, &client_list, list)
-		if (client->add && !add_client_context(device, client))
+		if (!add_client_context(device, client) && client->add)
 			client->add(device);
 
 	down_write(&lists_rwsem);
@@ -499,7 +499,7 @@ int ib_register_client(struct ib_client *client)
 	mutex_lock(&device_mutex);
 
 	list_for_each_entry(device, &device_list, core_list)
-		if (client->add && !add_client_context(device, client))
+		if (!add_client_context(device, client) && client->add)
 			client->add(device);
 
 	down_write(&lists_rwsem);
-- 
2.7.4

[PATCH v2 1/3] RDMA/core: make ib_device.add method optional

[Sagi Grimberg]

> ib_clients can indeed fill .add to NULL, but then they will not see
> any device removal notifications. The reason is that that
> ib_register_client and ib_register_device checked existence of .add
> before adding the creating a corresponding client_data and adding
> it to the list. Simple condition reverse fixes the issue.
> 
> Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
> Reviewed-by: Christoph Hellwig <hch at lst.de>
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>

Any RDMA folks?

[PATCH v2 1/3] RDMA/core: make ib_device.add method optional

[Leon Romanovsky]

On Thu, Jul 06, 2017@10:51:36AM +0300, Sagi Grimberg wrote:
> > ib_clients can indeed fill .add to NULL, but then they will not see
> > any device removal notifications. The reason is that that
> > ib_register_client and ib_register_device checked existence of .add
> > before adding the creating a corresponding client_data and adding
> > it to the list. Simple condition reverse fixes the issue.
> >
> > Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
> > Reviewed-by: Christoph Hellwig <hch at lst.de>
> > Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
>
> Any RDMA folks?

It looks right and two gentlemen above already reviewed it.

Thanks,
Reviewed-by: Leon Romanovsky <leonro at mellanox.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20170706/eeb4cec0/attachment.sig>

[PATCH v2 1/3] RDMA/core: make ib_device.add method optional

[Sagi Grimberg]

>> Any RDMA folks?
> 
> It looks right and two gentlemen above already reviewed it.

None of them is focused on the RDMA core on a day-to-day basis.

> Thanks,
> Reviewed-by: Leon Romanovsky <leonro at mellanox.com>

Awesome.

[PATCH v2 2/3] nvme-rdma: remove redundant empty device add callout

[Sagi Grimberg]

Now that its not needed, we can simply not assign it.

Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
Reviewed-by: Christoph Hellwig <hch at lst.de>
Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 6d4119dfbdaa..934183f80fff 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1956,10 +1956,6 @@ static struct nvmf_transport_ops nvme_rdma_transport = {
 	.create_ctrl	= nvme_rdma_create_ctrl,
 };
 
-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)
 {
 	struct nvme_rdma_ctrl *ctrl;
@@ -1981,7 +1977,6 @@ static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
 
 static struct ib_client nvme_rdma_ib_client = {
 	.name   = "nvme_rdma",
-	.add = nvme_rdma_add_one,
 	.remove = nvme_rdma_remove_one
 };
 
-- 
2.7.4

[PATCH v2 3/3] nvmet-rdma: remove redundant empty device add callout

[Sagi Grimberg]

Now that its not needed, we can simply not assign it.

Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
Reviewed-by: Christoph Hellwig <hch at lst.de>
Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/rdma.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 56a4cba690b5..76d2bb793afe 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1510,10 +1510,6 @@ static struct nvmet_fabrics_ops nvmet_rdma_ops = {
 	.delete_ctrl		= nvmet_rdma_delete_ctrl,
 };
 
-static void nvmet_rdma_add_one(struct ib_device *ib_device)
-{
-}
-
 static void nvmet_rdma_remove_one(struct ib_device *ib_device, void *client_data)
 {
 	struct nvmet_rdma_queue *queue;
@@ -1534,7 +1530,6 @@ static void nvmet_rdma_remove_one(struct ib_device *ib_device, void *client_data
 
 static struct ib_client nvmet_rdma_ib_client = {
 	.name   = "nvmet_rdma",
-	.add = nvmet_rdma_add_one,
 	.remove = nvmet_rdma_remove_one
 };
 
-- 
2.7.4

[PATCH v2 0/3] Allow ib_clients to nullify .add but still get .remove events reliably

[Max Gurtovoy]

On 7/2/2017 11:20 AM, Sagi Grimberg wrote:
> Some ULPs are just interested in device removal notification with no specific
> association because they work over RDMA_CM so no need for detecting appearance
> of the devices. That wasn't the because the corresponding ib_client_data
> was never added to the device clients list when the ib_client registers,
> Patch 1 fixes it. Patches 2-3 just removes the empty .add callout nvme(t)-rdma set.
>
> Changes from v1:
> - reword patch 1 change log title
> - collected review tags
>
> Note that this will create a conflict with jens's for-4.13 which we added
> ib_client registration to nvmet-rdma.
>
> Sagi Grimberg (3):
>   RDMA/core: make ib_device.add method optional
>   nvme-rdma: remove redundant empty device add callout
>   nvmet-rdma: remove redundant empty device add callout

All these patches looks fine to me,

Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

[PATCH v2 0/3] Allow ib_clients to nullify .add but still get .remove events reliably

[Doug Ledford]

On Sun, 2017-07-02@12:14 +0300, Max Gurtovoy wrote:
> 
> On 7/2/2017 11:20 AM, Sagi Grimberg wrote:
> > Some ULPs are just interested in device removal notification with
> > no specific
> > association because they work over RDMA_CM so no need for detecting
> > appearance
> > of the devices. That wasn't the because the corresponding
> > ib_client_data
> > was never added to the device clients list when the ib_client
> > registers,
> > Patch 1 fixes it. Patches 2-3 just removes the empty .add callout
> > nvme(t)-rdma set.
> > 
> > Changes from v1:
> > - reword patch 1 change log title
> > - collected review tags
> > 
> > Note that this will create a conflict with jens's for-4.13 which we
> > added
> > ib_client registration to nvmet-rdma.
> > 
> > Sagi Grimberg (3):
> >   RDMA/core: make ib_device.add method optional
> >   nvme-rdma: remove redundant empty device add callout
> >   nvmet-rdma: remove redundant empty device add callout
> 
> All these patches looks fine to me,
> 
> Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

Thanks, series applied.

-- 
Doug Ledford <dledford at redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD