On Tue, Jun 01, 2021 at 04:07:05PM +0100, John Garry wrote: > On 01/06/2021 14:11, Ming Lei wrote: > > > 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
