Re: [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma
From: John Garry <hidden>
Date: 2021-06-01 15:08:00
On 01/06/2021 14:11, Ming Lei wrote:
quoted hunk ↗ jump to hunk
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) I am not sure if we are required to keep that shost_dev reference all the way until the exit, as you do. Thanks, John