Re: [PATCH v5 5/6] scsi: core: Protect host state changes with the host lock

[John Garry <john.g.garry@oracle.com>]

On 05/08/2026 22:36, Bart Van Assche wrote:
> ---
>    drivers/scsi/hosts.c                      | 18 ++++++++++--------
>    drivers/scsi/megaraid/megaraid_sas_base.c |  2 +-
>    drivers/scsi/mpi3mr/mpi3mr_os.c           |  2 +-
>    drivers/scsi/mpt3sas/mpt3sas_scsih.c      |  2 +-
>    drivers/scsi/qla4xxx/ql4_os.c             |  6 ++----
>    drivers/scsi/scsi_lib.c                   |  3 +--
>    drivers/scsi/scsi_sysfs.c                 |  7 ++++---
>    include/scsi/scsi_host.h                  | 23 ++++++++++++++++-------
>    8 files changed, 36 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index d512080268af..d036accb4903 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -74,7 +74,7 @@ static struct class shost_class = {
>     **/
>    int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state)
>    {
> -	enum scsi_host_state oldstate = shost->shost_state;
> +	enum scsi_host_state oldstate = READ_ONCE(shost->shost_state);
 >    >    	if (state == oldstate)
>    		return 0;
> @@ -145,7 +145,7 @@ int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state)
>    		}
>    		break;
>    	}
> -	shost->shost_state = state;
> +	WRITE_ONCE(shost->shost_state, state);
>    	return 0;
>    
>     illegal:
> @@ -276,7 +276,8 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>    	if (error)
>    		goto out_disable_runtime_pm;
>    
> -	scsi_host_set_state(shost, SHOST_RUNNING);
> +	scoped_guard(spinlock_irq, shost->host_lock)
> +		scsi_host_set_state(shost, SHOST_RUNNING);
>    	get_device(shost->shost_gendev.parent);
>    
>    	device_enable_async_suspend(&shost->shost_dev);
> @@ -350,6 +351,7 @@ EXPORT_SYMBOL(scsi_add_host_with_dma);
>    static void scsi_host_dev_release(struct device *dev)
>    {
>    	struct Scsi_Host *shost = dev_to_shost(dev);
> +	enum scsi_host_state host_state = scsi_get_host_state(shost);

nit: it would be nice to use consistent variable names throughout the 
code, either state or host_state , if possible.

>    	struct device *parent = dev->parent;
>    
>    	/* Wait for functions invoked through call_rcu(&scmd->rcu, ...) */
> @@ -362,7 +364,7 @@ static void scsi_host_dev_release(struct device *dev)
>    	if (shost->work_q)
>    		destroy_workqueue(shost->work_q);
>    
> -	if (shost->shost_state == SHOST_CREATED) {
> +	if (host_state == SHOST_CREATED) {
>    		/*
>    		 * Free the shost_dev device name and remove the proc host dir
>    		 * here if scsi_host_{alloc,put}() have been called but neither
> @@ -378,7 +380,7 @@ static void scsi_host_dev_release(struct device *dev)
>    
>    	ida_free(&host_index_ida, shost->host_no);
>    
> -	if (shost->shost_state != SHOST_CREATED)
> +	if (host_state != SHOST_CREATED)
>    		put_device(parent);
>    	kfree(shost);
>    }
> @@ -411,8 +413,8 @@ struct Scsi_Host *scsi_host_alloc(const struct scsi_host_template *sht, int priv
>    		return NULL;
>    
>    	shost->host_lock = &shost->default_lock;
> -	spin_lock_init(shost->host_lock);
> -	shost->shost_state = SHOST_CREATED;
> +	scoped_guard(spinlock_init, shost->host_lock)
> +		shost->shost_state = SHOST_CREATED;
>    	INIT_LIST_HEAD(&shost->__devices);
>    	INIT_LIST_HEAD(&shost->__targets);
>    	INIT_LIST_HEAD(&shost->eh_abort_list);
> @@ -598,7 +600,7 @@ EXPORT_SYMBOL(scsi_host_lookup);
>     **/
>    struct Scsi_Host *scsi_host_get(struct Scsi_Host *shost)
>    {
> -	if ((shost->shost_state == SHOST_DEL) ||
> +	if (scsi_get_host_state(shost) == SHOST_DEL ||
>    		!get_device(&shost->shost_gendev))
>    		return NULL;
>    	return shost;
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
> index ecd365d78ae3..f0152b043e18 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -3072,7 +3072,7 @@ static int megasas_reset_bus_host(struct scsi_cmnd *scmd)
>    
>    	scmd_printk(KERN_INFO, scmd,
>    		"SCSI host state: %d  SCSI host busy: %d  FW outstanding: %d\n",
> -		scmd->device->host->shost_state,
> +		scsi_get_host_state(scmd->device->host),
>    		scsi_host_busy(scmd->device->host),
>    		atomic_read(&instance->fw_outstanding));
>    	/*
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 402d1f35d214..f80a21ec161b 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -5172,7 +5172,7 @@ static enum scsi_qc_status mpi3mr_qcmd(struct Scsi_Host *shost,
>    
>    	/* Avoid error handling escalation when device is removed or blocked */
>    
> -	if (scmd->device->host->shost_state == SHOST_RECOVERY &&
> +	if (scsi_get_host_state(scmd->device->host) == SHOST_RECOVERY &&
>    		scmd->cmnd[0] == TEST_UNIT_READY &&
>    		(stgt_priv_data->dev_removed || (dev_handle == MPI3MR_INVALID_DEV_HANDLE))) {
>    		scsi_build_sense(scmd, 0, UNIT_ATTENTION, 0x29, 0x07);
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index dea78688cc9b..0e12009a87f6 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -5472,7 +5472,7 @@ static enum scsi_qc_status scsih_qcmd(struct Scsi_Host *shost,
>    	 * Avoid error handling escallation when device is disconnected
>    	 */
>    	if (handle == MPT3SAS_INVALID_DEVICE_HANDLE || sas_device_priv_data->block) {
> -		if (scmd->device->host->shost_state == SHOST_RECOVERY &&
> +		if (scsi_get_host_state(scmd->device->host) == SHOST_RECOVERY &&
>    		    scmd->cmnd[0] == TEST_UNIT_READY) {
>    			scsi_build_sense(scmd, 0, UNIT_ATTENTION, 0x29, 0x07);
>    			scsi_done(scmd);
> diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
> index d598ab4126f8..c9d9fc7c81fb 100644
> --- a/drivers/scsi/qla4xxx/ql4_os.c
> +++ b/drivers/scsi/qla4xxx/ql4_os.c
> @@ -9411,11 +9411,9 @@ static int qla4xxx_eh_target_reset(struct scsi_cmnd *cmd)
>     * This routine finds that if reset host is called in EH
>     * scenario or from some application like sg_reset
>     **/
> -static int qla4xxx_is_eh_active(struct Scsi_Host *shost)
> +static bool qla4xxx_is_eh_active(struct Scsi_Host *shost)
>    {
> -	if (shost->shost_state == SHOST_RECOVERY)
> -		return 1;
> -	return 0;
> +	return scsi_get_host_state(shost) == SHOST_RECOVERY;
>    }
>    
>    /**
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 22e2e3223440..89d2e5a70e9b 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1661,10 +1661,9 @@ static enum scsi_qc_status scsi_dispatch_cmd(struct scsi_cmnd *cmd)
>    		goto done;
>    	}
>    
> -	if (unlikely(host->shost_state == SHOST_DEL)) {
> +	if (unlikely(scsi_get_host_state(host) == SHOST_DEL)) {
>    		cmd->result = (DID_NO_CONNECT << 16);
>    		goto done;
> -
>    	}
>    
>    	trace_scsi_dispatch_cmd_start(cmd);
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index dfc3559e7e04..9480432f650b 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -214,8 +214,9 @@ store_shost_state(struct device *dev, struct device_attribute *attr,
>    	if (!state)
>    		return -EINVAL;
>    
> -	if (scsi_host_set_state(shost, state))
> -		return -EINVAL;
> +	scoped_guard(spinlock_irq, shost->host_lock)
> +		if (scsi_host_set_state(shost, state))
> +			return -EINVAL;
>    	return count;
>    }
>    
> @@ -223,7 +224,7 @@ static ssize_t
>    show_shost_state(struct device *dev, struct device_attribute *attr, char *buf)
>    {
>    	struct Scsi_Host *shost = class_to_shost(dev);
> -	const char *name = scsi_host_state_name(shost->shost_state);
> +	const char *name = scsi_host_state_name(scsi_get_host_state(shost));
>    
>    	if (!name)
>    		return -EINVAL;
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 7e2011830ba4..69d432fd8e32 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -727,7 +727,7 @@ struct Scsi_Host {
>    	unsigned int  irq;
>    	
>    
> -	enum scsi_host_state shost_state;
> +	enum scsi_host_state shost_state __guarded_by(host_lock);
>    
>    	/* ldm bits */
>    	struct device		shost_gendev, shost_dev;
> @@ -785,11 +785,18 @@ static inline struct Scsi_Host *dev_to_shost(struct device *dev)
>    	return container_of(dev, struct Scsi_Host, shost_gendev);
>    }
>    
> +static inline enum scsi_host_state scsi_get_host_state(struct Scsi_Host *shost)
> +{
> +	return context_unsafe(READ_ONCE(shost->shost_state));

I am wondering if it may be better to protect reading this with the 
spinlock as well. We could lose the READ_ONCE and WRITE_ONCE. And we 
would be more symmetrical with the set function.

I really don't feel strongly about this, though.

> +}
> +
>    static inline int scsi_host_in_recovery(struct Scsi_Host *shost)
>    {
> -	return shost->shost_state == SHOST_RECOVERY ||
> -		shost->shost_state == SHOST_CANCEL_RECOVERY ||
> -		shost->shost_state == SHOST_DEL_RECOVERY ||
> +	enum scsi_host_state state = scsi_get_host_state(shost);
> +
> +	return state == SHOST_RECOVERY ||
> +		state == SHOST_CANCEL_RECOVERY ||
> +		state == SHOST_DEL_RECOVERY ||
>    		shost->tmf_in_progress;
>    }
>    
> @@ -835,8 +842,9 @@ static inline struct device *scsi_get_device(struct Scsi_Host *shost)
>     **/
>    static inline int scsi_host_scan_allowed(struct Scsi_Host *shost)
>    {
> -	return shost->shost_state == SHOST_RUNNING ||
> -	       shost->shost_state == SHOST_RECOVERY;
> +	enum scsi_host_state state = scsi_get_host_state(shost);
> +
> +	return state == SHOST_RUNNING || state == SHOST_RECOVERY;
>    }
>    
>    extern void scsi_unblock_requests(struct Scsi_Host *);
> @@ -940,6 +948,7 @@ static inline unsigned char scsi_host_get_guard(struct Scsi_Host *shost)
>    	return shost->prot_guard_type;
>    }
>    
> -extern int scsi_host_set_state(struct Scsi_Host *, enum scsi_host_state);
> +int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state)
> +	__must_hold(shost->host_lock);

How come we have this in the prototype and not the actual function itself?

>    
>    #endif /* _SCSI_SCSI_HOST_H */