* [PATCH v1 0/1] Set SDEV_OFFLINE when ufs shutdown.
[not found] <CGME20240829093920epcas1p1cf45ac0cd7d4ed8cf39ff5f1d1b4fe00@epcas1p1.samsung.com>
@ 2024-08-29 9:39 ` Seunghwan Baek
[not found] ` <CGME20240829093921epcas1p35d28696b0f79e2ae39d8e3690f088e64@epcas1p3.samsung.com>
2024-10-16 2:39 ` [PATCH v1 0/1] Set " Martin K. Petersen
0 siblings, 2 replies; 12+ messages in thread
From: Seunghwan Baek @ 2024-08-29 9:39 UTC (permalink / raw)
To: linux-kernel, linux-scsi, martin.petersen, James.Bottomley,
bvanassche, avri.altman, alim.akhtar
Cc: grant.jung, jt77.jang, junwoo80.lee, dh0421.hwang, jangsub.yi,
sh043.lee, cw9316.lee, sh8267.baek, wkon.kim
When ufs shutdown, set SDEV_OFFLINE instead of SDEV_QUIESCE for all lus
except device wlun.
Seunghwan Baek (1):
ufs: core: set SDEV_OFFLINE when ufs shutdown.
drivers/ufs/core/ufshcd.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
--
2.17.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v1 1/1] ufs: core: set SDEV_OFFLINE when ufs shutdown.
[not found] ` <CGME20240829093921epcas1p35d28696b0f79e2ae39d8e3690f088e64@epcas1p3.samsung.com>
@ 2024-08-29 9:39 ` Seunghwan Baek
2024-09-23 7:54 ` Seunghwan Baek
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Seunghwan Baek @ 2024-08-29 9:39 UTC (permalink / raw)
To: linux-kernel, linux-scsi, martin.petersen, James.Bottomley,
bvanassche, avri.altman, alim.akhtar
Cc: grant.jung, jt77.jang, junwoo80.lee, dh0421.hwang, jangsub.yi,
sh043.lee, cw9316.lee, sh8267.baek, wkon.kim, stable
There is a history of dead lock as reboot is performed at the beginning of
booting. SDEV_QUIESCE was set for all lu's scsi_devices by ufs shutdown,
and at that time the audio driver was waiting on blk_mq_submit_bio holding
a mutex_lock while reading the fw binary. After that, a deadlock issue
occurred while audio driver shutdown was waiting for mutex_unlock of
blk_mq_submit_bio. To solve this, set SDEV_OFFLINE for all lus except wlun,
so that any i/o that comes down after a ufs shutdown will return an error.
[ 31.907781]I[0: swapper/0: 0] 1 130705007 1651079834 11289729804 0 D( 2) 3 ffffff882e208000 * init [device_shutdown]
[ 31.907793]I[0: swapper/0: 0] Mutex: 0xffffff8849a2b8b0: owner[0xffffff882e28cb00 kworker/6:0 :49]
[ 31.907806]I[0: swapper/0: 0] Call trace:
[ 31.907810]I[0: swapper/0: 0] __switch_to+0x174/0x338
[ 31.907819]I[0: swapper/0: 0] __schedule+0x5ec/0x9cc
[ 31.907826]I[0: swapper/0: 0] schedule+0x7c/0xe8
[ 31.907834]I[0: swapper/0: 0] schedule_preempt_disabled+0x24/0x40
[ 31.907842]I[0: swapper/0: 0] __mutex_lock+0x408/0xdac
[ 31.907849]I[0: swapper/0: 0] __mutex_lock_slowpath+0x14/0x24
[ 31.907858]I[0: swapper/0: 0] mutex_lock+0x40/0xec
[ 31.907866]I[0: swapper/0: 0] device_shutdown+0x108/0x280
[ 31.907875]I[0: swapper/0: 0] kernel_restart+0x4c/0x11c
[ 31.907883]I[0: swapper/0: 0] __arm64_sys_reboot+0x15c/0x280
[ 31.907890]I[0: swapper/0: 0] invoke_syscall+0x70/0x158
[ 31.907899]I[0: swapper/0: 0] el0_svc_common+0xb4/0xf4
[ 31.907909]I[0: swapper/0: 0] do_el0_svc+0x2c/0xb0
[ 31.907918]I[0: swapper/0: 0] el0_svc+0x34/0xe0
[ 31.907928]I[0: swapper/0: 0] el0t_64_sync_handler+0x68/0xb4
[ 31.907937]I[0: swapper/0: 0] el0t_64_sync+0x1a0/0x1a4
[ 31.908774]I[0: swapper/0: 0] 49 0 11960702 11236868007 0 D( 2) 6 ffffff882e28cb00 * kworker/6:0 [__bio_queue_enter]
[ 31.908783]I[0: swapper/0: 0] Call trace:
[ 31.908788]I[0: swapper/0: 0] __switch_to+0x174/0x338
[ 31.908796]I[0: swapper/0: 0] __schedule+0x5ec/0x9cc
[ 31.908803]I[0: swapper/0: 0] schedule+0x7c/0xe8
[ 31.908811]I[0: swapper/0: 0] __bio_queue_enter+0xb8/0x178
[ 31.908818]I[0: swapper/0: 0] blk_mq_submit_bio+0x194/0x67c
[ 31.908827]I[0: swapper/0: 0] __submit_bio+0xb8/0x19c
Fixes: b294ff3e3449 ("scsi: ufs: core: Enable power management for wlun")
Cc: stable@vger.kernel.org
Signed-off-by: Seunghwan Baek <sh8267.baek@samsung.com>
---
drivers/ufs/core/ufshcd.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
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);
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* RE: [PATCH v1 1/1] ufs: core: set SDEV_OFFLINE when ufs shutdown.
2024-08-29 9:39 ` [PATCH v1 1/1] ufs: core: set " Seunghwan Baek
@ 2024-09-23 7:54 ` Seunghwan Baek
2024-09-23 17:31 ` Bart Van Assche
2024-09-23 17:41 ` Bart Van Assche
2024-10-08 17:58 ` Bart Van Assche
2 siblings, 1 reply; 12+ messages in thread
From: Seunghwan Baek @ 2024-09-23 7:54 UTC (permalink / raw)
To: linux-kernel, linux-scsi, martin.petersen, James.Bottomley,
bvanassche, avri.altman, alim.akhtar
Cc: grant.jung, jt77.jang, junwoo80.lee, dh0421.hwang, jangsub.yi,
sh043.lee, cw9316.lee, wkon.kim, stable
> There is a history of dead lock as reboot is performed at the beginning of
> booting. SDEV_QUIESCE was set for all lu's scsi_devices by ufs shutdown,
> and at that time the audio driver was waiting on blk_mq_submit_bio holding
> a mutex_lock while reading the fw binary. After that, a deadlock issue
> occurred while audio driver shutdown was waiting for mutex_unlock of
> blk_mq_submit_bio. To solve this, set SDEV_OFFLINE for all lus except wlun,
> so that any i/o that comes down after a ufs shutdown will return an error.
>
> [ 31.907781]I[0: swapper/0: 0] 1 130705007 1651079834
> 11289729804 0 D( 2) 3 ffffff882e208000 * init
> [device_shutdown]
> [ 31.907793]I[0: swapper/0: 0] Mutex: 0xffffff8849a2b8b0:
> owner[0xffffff882e28cb00 kworker/6:0 :49]
> [ 31.907806]I[0: swapper/0: 0] Call trace:
> [ 31.907810]I[0: swapper/0: 0] __switch_to+0x174/0x338
> [ 31.907819]I[0: swapper/0: 0] __schedule+0x5ec/0x9cc
> [ 31.907826]I[0: swapper/0: 0] schedule+0x7c/0xe8
> [ 31.907834]I[0: swapper/0: 0] schedule_preempt_disabled+0x24/0x40
> [ 31.907842]I[0: swapper/0: 0] __mutex_lock+0x408/0xdac
> [ 31.907849]I[0: swapper/0: 0] __mutex_lock_slowpath+0x14/0x24
> [ 31.907858]I[0: swapper/0: 0] mutex_lock+0x40/0xec
> [ 31.907866]I[0: swapper/0: 0] device_shutdown+0x108/0x280
> [ 31.907875]I[0: swapper/0: 0] kernel_restart+0x4c/0x11c
> [ 31.907883]I[0: swapper/0: 0] __arm64_sys_reboot+0x15c/0x280
> [ 31.907890]I[0: swapper/0: 0] invoke_syscall+0x70/0x158
> [ 31.907899]I[0: swapper/0: 0] el0_svc_common+0xb4/0xf4
> [ 31.907909]I[0: swapper/0: 0] do_el0_svc+0x2c/0xb0
> [ 31.907918]I[0: swapper/0: 0] el0_svc+0x34/0xe0
> [ 31.907928]I[0: swapper/0: 0] el0t_64_sync_handler+0x68/0xb4
> [ 31.907937]I[0: swapper/0: 0] el0t_64_sync+0x1a0/0x1a4
>
> [ 31.908774]I[0: swapper/0: 0] 49 0 11960702
> 11236868007 0 D( 2) 6 ffffff882e28cb00 * kworker/6:0
> [__bio_queue_enter]
> [ 31.908783]I[0: swapper/0: 0] Call trace:
> [ 31.908788]I[0: swapper/0: 0] __switch_to+0x174/0x338
> [ 31.908796]I[0: swapper/0: 0] __schedule+0x5ec/0x9cc
> [ 31.908803]I[0: swapper/0: 0] schedule+0x7c/0xe8
> [ 31.908811]I[0: swapper/0: 0] __bio_queue_enter+0xb8/0x178
> [ 31.908818]I[0: swapper/0: 0] blk_mq_submit_bio+0x194/0x67c
> [ 31.908827]I[0: swapper/0: 0] __submit_bio+0xb8/0x19c
>
> Fixes: b294ff3e3449 ("scsi: ufs: core: Enable power management for wlun")
> Cc: stable@vger.kernel.org
> Signed-off-by: Seunghwan Baek <sh8267.baek@samsung.com>
> ---
> drivers/ufs/core/ufshcd.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> 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);
>
> --
> 2.17.1
>
Dear all.
Could you please review this patch? It's been almost a month.
If you have any opinions about this patch, share and comment it.
Thanks.
BRs.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/1] ufs: core: set SDEV_OFFLINE when ufs shutdown.
2024-09-23 7:54 ` Seunghwan Baek
@ 2024-09-23 17:31 ` Bart Van Assche
0 siblings, 0 replies; 12+ messages in thread
From: Bart Van Assche @ 2024-09-23 17:31 UTC (permalink / raw)
To: Seunghwan Baek, linux-kernel, linux-scsi, martin.petersen,
James.Bottomley, avri.altman, alim.akhtar
Cc: grant.jung, jt77.jang, junwoo80.lee, dh0421.hwang, jangsub.yi,
sh043.lee, cw9316.lee, wkon.kim, stable
On 9/23/24 12:54 AM, Seunghwan Baek wrote:
> Could you please review this patch? It's been almost a month.
> If you have any opinions about this patch, share and comment it.
Thanks for the reminder. I'm not sure why this patch got overlooked but
I will take a look.
Bart.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/1] ufs: core: set SDEV_OFFLINE when ufs shutdown.
2024-08-29 9:39 ` [PATCH v1 1/1] ufs: core: set " Seunghwan Baek
2024-09-23 7:54 ` Seunghwan Baek
@ 2024-09-23 17:41 ` Bart Van Assche
2024-09-24 2:17 ` Seunghwan Baek
2024-10-08 17:58 ` Bart Van Assche
2 siblings, 1 reply; 12+ messages in thread
From: Bart Van Assche @ 2024-09-23 17:41 UTC (permalink / raw)
To: Seunghwan Baek, linux-kernel, linux-scsi, martin.petersen,
James.Bottomley, avri.altman, alim.akhtar
Cc: grant.jung, jt77.jang, junwoo80.lee, dh0421.hwang, jangsub.yi,
sh043.lee, cw9316.lee, wkon.kim, stable
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.
^ permalink raw reply related [flat|nested] 12+ messages in thread
* RE: [PATCH v1 1/1] ufs: core: set SDEV_OFFLINE when ufs shutdown.
2024-09-23 17:41 ` Bart Van Assche
@ 2024-09-24 2:17 ` Seunghwan Baek
2024-09-24 18:24 ` Bart Van Assche
0 siblings, 1 reply; 12+ messages in thread
From: Seunghwan Baek @ 2024-09-24 2:17 UTC (permalink / raw)
To: 'Bart Van Assche', linux-kernel, linux-scsi,
martin.petersen, James.Bottomley, avri.altman, alim.akhtar
Cc: grant.jung, jt77.jang, junwoo80.lee, dh0421.hwang, jangsub.yi,
sh043.lee, cw9316.lee, wkon.kim, stable
> 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;
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/1] ufs: core: set SDEV_OFFLINE when ufs shutdown.
2024-09-24 2:17 ` Seunghwan Baek
@ 2024-09-24 18:24 ` Bart Van Assche
2024-09-25 6:54 ` Seunghwan Baek
2024-10-08 5:27 ` Seunghwan Baek
0 siblings, 2 replies; 12+ messages in thread
From: Bart Van Assche @ 2024-09-24 18:24 UTC (permalink / raw)
To: Seunghwan Baek, linux-kernel, linux-scsi, martin.petersen,
James.Bottomley, avri.altman, alim.akhtar
Cc: grant.jung, jt77.jang, junwoo80.lee, dh0421.hwang, jangsub.yi,
sh043.lee, cw9316.lee, wkon.kim, stable
On 9/23/24 7:17 PM, Seunghwan Baek wrote:> 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.
Right. Since ufshcd_wl_shutdown() is expected to stop all DMA related to
the UFS host, shouldn't there be a scsi_device_quiesce(sdev) call after
the __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM) call?
Thanks,
Bart.
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v1 1/1] ufs: core: set SDEV_OFFLINE when ufs shutdown.
2024-09-24 18:24 ` Bart Van Assche
@ 2024-09-25 6:54 ` Seunghwan Baek
2024-10-08 5:27 ` Seunghwan Baek
1 sibling, 0 replies; 12+ messages in thread
From: Seunghwan Baek @ 2024-09-25 6:54 UTC (permalink / raw)
To: 'Bart Van Assche', linux-kernel, linux-scsi,
martin.petersen, James.Bottomley, avri.altman, alim.akhtar
Cc: grant.jung, jt77.jang, junwoo80.lee, dh0421.hwang, jangsub.yi,
sh043.lee, cw9316.lee, wkon.kim, stable
> On 9/23/24 7:17 PM, Seunghwan Baek wrote:> 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.
> Right. Since ufshcd_wl_shutdown() is expected to stop all DMA related to
> the UFS host, shouldn't there be a scsi_device_quiesce(sdev) call after
> the __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM) call?
>
> Thanks,
>
> Bart.
Yes. __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM) should be called after
scsi_device_quiesce(sdev). Generally, the SSU command is the last command
before UFS power off. Therefore, if __ufshcd_wl_suspend is performed
before scsi_device_quiesce, other commands may be performed after the SSU
command and UFS may not guarantee the operation of the SSU command, which
may cause other problems. This order must be guaranteed.
And with SDEV_QUIESCE, deadlock issue cannot be avoided due to requeue.
We need to return the i/o error with SDEV_OFFLINE to avoid the mentioned
deadlock problem.
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v1 1/1] ufs: core: set SDEV_OFFLINE when ufs shutdown.
2024-09-24 18:24 ` Bart Van Assche
2024-09-25 6:54 ` Seunghwan Baek
@ 2024-10-08 5:27 ` Seunghwan Baek
2024-10-10 5:47 ` Kiwoong Kim
1 sibling, 1 reply; 12+ messages in thread
From: Seunghwan Baek @ 2024-10-08 5:27 UTC (permalink / raw)
To: 'Bart Van Assche', linux-kernel, linux-scsi,
martin.petersen, James.Bottomley, avri.altman, alim.akhtar
Cc: grant.jung, jt77.jang, junwoo80.lee, dh0421.hwang, jangsub.yi,
sh043.lee, cw9316.lee, wkon.kim, stable, kwmad.kim, sh425.lee,
hy50.seo, kwangwon.min, h10.kim
> > On 9/23/24 7:17 PM, Seunghwan Baek wrote:> 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.
> > Right. Since ufshcd_wl_shutdown() is expected to stop all DMA related
> > to the UFS host, shouldn't there be a scsi_device_quiesce(sdev) call
> > after the __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM) call?
> >
> > Thanks,
> >
> > Bart.
>
> Yes. __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM) should be called after
> scsi_device_quiesce(sdev). Generally, the SSU command is the last command
> before UFS power off. Therefore, if __ufshcd_wl_suspend is performed
> before scsi_device_quiesce, other commands may be performed after the SSU
> command and UFS may not guarantee the operation of the SSU command, which
> may cause other problems. This order must be guaranteed.
>
> And with SDEV_QUIESCE, deadlock issue cannot be avoided due to requeue.
> We need to return the i/o error with SDEV_OFFLINE to avoid the mentioned
> deadlock problem.
(+ more CC added.)
Dear All.
Could you please update for this patch?
If you have any opinions about this patch, share and comment it.
Thanks.
BRs.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/1] ufs: core: set SDEV_OFFLINE when ufs shutdown.
2024-08-29 9:39 ` [PATCH v1 1/1] ufs: core: set " Seunghwan Baek
2024-09-23 7:54 ` Seunghwan Baek
2024-09-23 17:41 ` Bart Van Assche
@ 2024-10-08 17:58 ` Bart Van Assche
2 siblings, 0 replies; 12+ messages in thread
From: Bart Van Assche @ 2024-10-08 17:58 UTC (permalink / raw)
To: Seunghwan Baek, linux-kernel, linux-scsi, martin.petersen,
James.Bottomley, avri.altman, alim.akhtar
Cc: grant.jung, jt77.jang, junwoo80.lee, dh0421.hwang, jangsub.yi,
sh043.lee, cw9316.lee, wkon.kim, stable
On 8/29/24 2:39 AM, Seunghwan Baek wrote:
> There is a history of dead lock as reboot is performed at the beginning of
> booting. SDEV_QUIESCE was set for all lu's scsi_devices by ufs shutdown,
> and at that time the audio driver was waiting on blk_mq_submit_bio holding
> a mutex_lock while reading the fw binary. After that, a deadlock issue
> occurred while audio driver shutdown was waiting for mutex_unlock of
> blk_mq_submit_bio. To solve this, set SDEV_OFFLINE for all lus except wlun,
> so that any i/o that comes down after a ufs shutdown will return an error.
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v1 1/1] ufs: core: set SDEV_OFFLINE when ufs shutdown.
2024-10-08 5:27 ` Seunghwan Baek
@ 2024-10-10 5:47 ` Kiwoong Kim
0 siblings, 0 replies; 12+ messages in thread
From: Kiwoong Kim @ 2024-10-10 5:47 UTC (permalink / raw)
To: 'Seunghwan Baek', 'Bart Van Assche', linux-kernel,
linux-scsi, martin.petersen, James.Bottomley, avri.altman,
alim.akhtar
Cc: grant.jung, jt77.jang, junwoo80.lee, dh0421.hwang, jangsub.yi,
sh043.lee, cw9316.lee, wkon.kim, stable, sh425.lee, hy50.seo,
kwangwon.min, h10.kim
> > > On 9/23/24 7:17 PM, Seunghwan Baek wrote:> 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.
> > > Right. Since ufshcd_wl_shutdown() is expected to stop all DMA
> > > related to the UFS host, shouldn't there be a
> > > scsi_device_quiesce(sdev) call after the __ufshcd_wl_suspend(hba,
> UFS_SHUTDOWN_PM) call?
> > >
> > > Thanks,
> > >
> > > Bart.
> >
> > Yes. __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM) should be called after
> > scsi_device_quiesce(sdev). Generally, the SSU command is the last
> > command before UFS power off. Therefore, if __ufshcd_wl_suspend is
> > performed before scsi_device_quiesce, other commands may be performed
> > after the SSU command and UFS may not guarantee the operation of the
> > SSU command, which may cause other problems. This order must be
> guaranteed.
> >
> > And with SDEV_QUIESCE, deadlock issue cannot be avoided due to requeue.
> > We need to return the i/o error with SDEV_OFFLINE to avoid the
> > mentioned deadlock problem.
>
> (+ more CC added.)
> Dear All.
>
> Could you please update for this patch?
> If you have any opinions about this patch, share and comment it.
>
> Thanks.
> BRs.
Looks good to me.
Thanks.
Kiwoong Kim.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 0/1] Set SDEV_OFFLINE when ufs shutdown.
2024-08-29 9:39 ` [PATCH v1 0/1] Set SDEV_OFFLINE when ufs shutdown Seunghwan Baek
[not found] ` <CGME20240829093921epcas1p35d28696b0f79e2ae39d8e3690f088e64@epcas1p3.samsung.com>
@ 2024-10-16 2:39 ` Martin K. Petersen
1 sibling, 0 replies; 12+ messages in thread
From: Martin K. Petersen @ 2024-10-16 2:39 UTC (permalink / raw)
To: linux-kernel, linux-scsi, James.Bottomley, bvanassche,
avri.altman, alim.akhtar, Seunghwan Baek
Cc: Martin K . Petersen, grant.jung, jt77.jang, junwoo80.lee,
dh0421.hwang, jangsub.yi, sh043.lee, cw9316.lee, wkon.kim
On Thu, 29 Aug 2024 18:39:12 +0900, Seunghwan Baek wrote:
> When ufs shutdown, set SDEV_OFFLINE instead of SDEV_QUIESCE for all lus
> except device wlun.
>
> Seunghwan Baek (1):
> ufs: core: set SDEV_OFFLINE when ufs shutdown.
>
> drivers/ufs/core/ufshcd.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> [...]
Applied to 6.12/scsi-fixes, thanks!
[1/1] ufs: core: set SDEV_OFFLINE when ufs shutdown.
https://git.kernel.org/mkp/scsi/c/19a198b67767
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-10-16 2:39 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20240829093920epcas1p1cf45ac0cd7d4ed8cf39ff5f1d1b4fe00@epcas1p1.samsung.com>
2024-08-29 9:39 ` [PATCH v1 0/1] Set SDEV_OFFLINE when ufs shutdown Seunghwan Baek
[not found] ` <CGME20240829093921epcas1p35d28696b0f79e2ae39d8e3690f088e64@epcas1p3.samsung.com>
2024-08-29 9:39 ` [PATCH v1 1/1] ufs: core: set " Seunghwan Baek
2024-09-23 7:54 ` Seunghwan Baek
2024-09-23 17:31 ` Bart Van Assche
2024-09-23 17:41 ` Bart Van Assche
2024-09-24 2:17 ` Seunghwan Baek
2024-09-24 18:24 ` Bart Van Assche
2024-09-25 6:54 ` Seunghwan Baek
2024-10-08 5:27 ` Seunghwan Baek
2024-10-10 5:47 ` Kiwoong Kim
2024-10-08 17:58 ` Bart Van Assche
2024-10-16 2:39 ` [PATCH v1 0/1] Set " Martin K. Petersen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).