On Tue, Jun 01, 2021 at 04:07:05PM +0100, John Garry wrote:

On 01/06/2021 14:11, Ming Lei wrote:

quoted

quoted

We don't call scsi_host_cls_release() either, so I guess a ref count is
leaked for shost_dev - I see its refcount is 1 at exit in
scsi_add_host_with_dma(). We have the device_initialize(), device_add(),
device_del() in the alloc and add host functions, but I don't know who is
responsible for the final "device put".

Hammm, we still need to put ->shost_dev before returning the error, and the
following delta patch can fix the issue, and it should have been wrapped
into the 1st one.

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 22a58e453a0c..532165462a42 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c

@@ -306,6 +306,8 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
  	pm_runtime_set_suspended(&shost->shost_gendev);
  	pm_runtime_put_noidle(&shost->shost_gendev);
   fail:
+	/* drop ref of ->shost_dev so that caller can release this host */
+	put_device(&shost->shost_dev);
  	return error;
  }
  EXPORT_SYMBOL(scsi_add_host_with_dma);

That looks better now.

And we can see the equivalent on the normal removal path in
scsi_remove_host() -> device_unregister(&shost->shost_dev), which does a
device_del()+put_device().

So could we actually just have:
out_del_dev:
	unregister_dev(&shost->shost_dev)

No, we still have to call put_device(&shost->shost_dev) only in case of
failure before adding &shost->shost_dev.

I am not sure if we are required to keep that shost_dev reference all the
way until the exit, as you do.

That has been done in this way, the problem is that both .shost_dev and
.shost_gendev share same lifetime and memory(same struct Scsi_Host instance),
this kind of pattern isn't one usual driver core use case, and we have to
handle it carefully.


Thanks,
Ming