linux-kernel - RE: [PATCH v1 1/1] ufs: core: set SDEV

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <003201db0e27$df93f250$9ebbd6f0$@samsung.com>
Date: Tue, 24 Sep 2024 11:17:15 +0900
From: "Seunghwan Baek" <sh8267.baek@...sung.com>
To: "'Bart Van Assche'" <bvanassche@....org>,
	<linux-kernel@...r.kernel.org>, <linux-scsi@...r.kernel.org>,
	<martin.petersen@...cle.com>, <James.Bottomley@...senPartnership.com>,
	<avri.altman@....com>, <alim.akhtar@...sung.com>
Cc: <grant.jung@...sung.com>, <jt77.jang@...sung.com>,
	<junwoo80.lee@...sung.com>, <dh0421.hwang@...sung.com>,
	<jangsub.yi@...sung.com>, <sh043.lee@...sung.com>, <cw9316.lee@...sung.com>,
	<wkon.kim@...sung.com>, <stable@...r.kernel.org>
Subject: RE: [PATCH v1 1/1] ufs: core: set SDEV_OFFLINE when ufs shutdown.

> On 8/29/24 2:39 AM, Seunghwan Baek wrote:
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index a6f818cdef0e..4ac1492787c2 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -10215,7 +10215,9 @@ static void ufshcd_wl_shutdown(struct device
> *dev)
> >   	shost_for_each_device(sdev, hba->host) {
> >   		if (sdev == hba->ufs_device_wlun)
> >   			continue;
> > -		scsi_device_quiesce(sdev);
> > +		mutex_lock(&sdev->state_mutex);
> > +		scsi_device_set_state(sdev, SDEV_OFFLINE);
> > +		mutex_unlock(&sdev->state_mutex);
> >   	}
> >   	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
> 
> Why to keep one scsi_device_quiesce() call and convert the other call?
> Please consider something like this change:
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
> e808350c6774..914770dff18f 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -10134,11 +10134,10 @@ static void ufshcd_wl_shutdown(struct device
> *dev)
> 
>   	/* Turn on everything while shutting down */
>   	ufshcd_rpm_get_sync(hba);
> -	scsi_device_quiesce(sdev);
>   	shost_for_each_device(sdev, hba->host) {
> -		if (sdev == hba->ufs_device_wlun)
> -			continue;
> -		scsi_device_quiesce(sdev);
> +		mutex_lock(&sdev->state_mutex);
> +		scsi_device_set_state(sdev, SDEV_OFFLINE);
> +		mutex_unlock(&sdev->state_mutex);
>   	}
>   	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
> 
> Thanks,
> 
> Bart.

That's because SSU (Start Stop Unit) command must be sent during shutdown process.
If SDEV_OFFLINE is set for wlun, SSU command cannot be sent because it is rejected
by the scsi layer. Therefore, we consider to set SDEV_QUIESCE for wlun, and set
SDEV_OFFLINE for other lus.

static int ufshcd_execute_start_stop(struct scsi_device *sdev,
				     enum ufs_dev_pwr_mode pwr_mode,
				     struct scsi_sense_hdr *sshdr)
{
	const unsigned char cdb[6] = { START_STOP, 0, 0, 0, pwr_mode << 4, 0 };
	const struct scsi_exec_args args = {
		.sshdr = sshdr,
		.req_flags = BLK_MQ_REQ_PM,            <<< set REQ_PM flag
		.scmd_flags = SCMD_FAIL_IF_RECOVERING,
	};

	return scsi_execute_cmd(sdev, cdb, REQ_OP_DRV_IN, /*buffer=*/NULL,
			/*bufflen=*/0, /*timeout=*/10 * HZ, /*retries=*/0,
			&args);
}

static blk_status_t
scsi_device_state_check(struct scsi_device *sdev, struct request *req)
{
	case SDEV_OFFLINE:
	case SDEV_TRANSPORT_OFFLINE:             <<< Refuse all commands
		/*
		 * If the device is offline we refuse to process any
		 * commands.  The device must be brought online
		 * before trying any recovery commands.
		 */
		if (!sdev->offline_already) {
			sdev->offline_already = true;
			sdev_printk(KERN_ERR, sdev,
				    "rejecting I/O to offline device\n");
		}
		return BLK_STS_IOERR;
	case SDEV_QUIESCE:                       <<< Refuse all commands except REQ_PM flag
		/*
		 * If the device is blocked we only accept power management
		 * commands.
		 */
		if (req && WARN_ON_ONCE(!(req->rq_flags & RQF_PM)))
			return BLK_STS_RESOURCE;
		return BLK_STS_OK;