Re: [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma

From: Ming Lei <ming.lei@redhat.com>
To: John Garry <john.garry@huawei.com>
Cc: "Martin K . Petersen" <martin.petersen@oracle.com>,
	linux-scsi@vger.kernel.org, Bart Van Assche <bvanassche@acm.org>,
	Hannes Reinecke <hare@suse.de>
Subject: Re: [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma
Date: Wed, 2 Jun 2021 09:55:31 +0800	[thread overview]
Message-ID: <YLblEz2xDZfWZ6jA@T590> (raw)
In-Reply-To: <17786916-5e1f-8387-344c-55bb1020b09e@huawei.com>

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