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

From: John Garry <john.garry@huawei.com>
To: Ming Lei <ming.lei@redhat.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: Tue, 1 Jun 2021 16:07:05 +0100	[thread overview]
Message-ID: <17786916-5e1f-8387-344c-55bb1020b09e@huawei.com> (raw)
In-Reply-To: <YLYx//I7J2kcnrtN@T590>

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)

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