On 2021-01-08 19:29, Bean Huo wrote:
On Wed, 2021-01-06 at 09:20 +0800, Can Guo wrote:
Hi Bean,
On 2021-01-06 02:38, Bean Huo wrote:
> On Tue, 2021-01-05 at 09:07 +0800, Can Guo wrote:
> > On 2021-01-05 04:05, Bean Huo wrote:
> > > On Sat, 2021-01-02 at 05:59 -0800, Can Guo wrote:
> > > > + * @shutting_down: flag to check if shutdown has been
> > > > invoked
> > >
> > > I am not much sure if this flag is need, since once PM going in
> > > shutdown path, what will be returnded by pm_runtime_get_sync()?
> > >
> > > If pm_runtime_get_sync() will fail, just check its return.
> > >
> >
> > That depends. During/after shutdown, for UFS's case only,
> > pm_runtime_get_sync(hba->dev) will most likely return 0,
> > because it is already RUNTIME_ACTIVE, pm_runtime_get_sync()
> > will directly return 0... meaning you cannot count on it.
> >
> > Check Stanley's change -
> > https://lore.kernel.org/patchwork/patch/1341389/
> >
> > Can Guo.
>
> Can,
>
> Thanks for pointing out that.
>
> Based on my understanding, that patch is redundent. maybe I
> misundestood Linux shutdown sequence.
Sorry, do you mean Stanley's change is redundant?
yes.
No, it is definitely needed. As Stanley replied you in another
thread, it is not protecting I/Os from user layer, but from
other subsystems during shutdown.
>
> I checked the shutdown flow:
>
> 1. Set the "system_state" variable
> 2. Disable usermod to ensure that no user from userspace can start
> a
> request
I hope it is like what you interpreted, but step #2 only stops
UMH(#265)
but not all user space activities. Whereas, UMH is for kernel space
calling
user space.
Can,
I did further study and homework on the Linux shutdown in the last few
days. Yes, you are right, usermodehelper_disable() is to prevent
executing the process from the kernel space.
But I didn't reproduce this "maybe" race issue while shutdown. no
matter how I torment my system, once Linux shutdown/halt/reboot starts,
nobody can access the sysfs node. I create 10 processes in the user
space and constantly access UFS sysfs node, also, fio is running in the
background for the normal data read/write. there is a shutdown thread
that will randomly trigger shutdown/halt/reboot. but no race issue
appears.
I don't know if this is a hypothetical issue(the race between shutdown
flow and sysfs node access), it may not really exist in the Linux
envriroment. everytime, the shutdonw flow will be:
e10_sync_handler()->e10_svc()->do_e10_svc()->__do_sys_reboot()-
kernel_poweroff/kernel_halt()->device_shutdown()->platform_shutdown()-
ufshcd_platform_shutdown()->ufshcd_shutdown().
I think before going into the kernel shutdown, the userspace cannot
issue new requests anymore. otherwise, this would be a big issue.
pm_runtime_get_sync() will return 0 or failure while shutdown? the
answer is not important now, maybe as you said, it is always 0. But in
my testing, it didn't get there the system has been shutdown. Which
means once shutdonw starts, sysfs node access path cannot reach
pm_runtime_get_sync(). (note, I don't know if sysfs node access thread
has been disabled or not)
Responsibly say, I didn't reproduce this issue on my system (ubuntu),
maybe you are using Android. I am not an expert on this topic, if you
have the best idea on how to reproduce this issue. please please let me
try. appreciate it!!!!!
When you do a reboot/shutdown/poweroff, how your system behaves highly
depends on how the reboot cmd is implemented in C code under /sbin/.
On Ubuntu, reboot looks like:
$ reboot --help
reboot [OPTIONS...] [ARG]
Reboot the system.
--help Show this help
--halt Halt the machine
-p --poweroff Switch off the machine
--reboot Reboot the machine
-f --force Force immediate halt/power-off/reboot
-w --wtmp-only Don't halt/power-off/reboot, just write wtmp record
-d --no-wtmp Don't write wtmp record
--no-wall Don't send wall message before halt/power-off/reboot
On a pure Linux with a initrd RAM FS built from busybox, reboot looks
like:
# reboot --help
BusyBox v1.30.1 (2019-05-24 12:53:36 IST) multi-call binary.
Usage: reboot [-d DELAY] [-n] [-f]
Reboot the system
-d SEC Delay interval
-n Do not sync
-f Force (don't go through init)
For example, when you work on a pure Linux with a filesystem built from
busybox, when you hit reboot cmd, halt_main() will be called. And based
on the reboot options passed to reboot cmd, halt_main() behaves
differently.
A plain reboot cmd does things like sync filesystem, send SIGKILL to all
processes (except for init), remount all filesytem as read-only and so
on
before invoking linux kernel reboot syscall. In this case, we are safe.
However, if you do a "reboot -f", halt_main() directly invokes reboot().
And with "reboot -f", I can easily reproduce the race condition we are
talking about here - it is not based on imagination.
Find the patch I used for replication in the attachment, fix conflicts
if any. After boot up, the cmd lines I used are
# while true; do cat /sys/devices/platform/soc@0/*ufshc*/rpm_lvl; done &
# reboot -f
Can Guo.
Thanks,
Bean
264 system_state = state;
265 usermodehelper_disable();
266 device_shutdown();
Thanks,
Can Guo.
From 639358dd6e2f7e5e9f7e66b828bf043fa3c7bbf2 Mon Sep 17 00:00:00 2001
From: Can Guo <cang@xxxxxxxxxxxxxx>
Date: Sat, 9 Jan 2021 12:37:40 +0800
Subject: [PATCH] scsi: ufs: Reproduce race condition btw sysfs access and
shutdown
Reproduce race condition btw sysfs access and shutdown
Usage:
Signed-off-by: Can Guo <cang@xxxxxxxxxxxxxx>
diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 92a63ee..a29a490 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -59,6 +59,8 @@ static ssize_t rpm_lvl_show(struct device *dev,
{
struct ufs_hba *hba = dev_get_drvdata(dev);
+ BUG_ON(hba->shutting_down);
+
return sprintf(buf, "%d\n", hba->rpm_lvl);
}
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 698e8d2..210fa5c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8277,6 +8277,11 @@ int ufshcd_shutdown(struct ufs_hba *hba)
{
int ret = 0;
+ hba->shutting_down = true;
+ pr_err("[DEBUG]%s: UFS SHUTDOWN START\n", __func__);
+
+ usleep_range(10000000, 11000000);
+
if (!hba->is_powered)
goto out;
@@ -8294,6 +8299,7 @@ int ufshcd_shutdown(struct ufs_hba *hba)
if (ret)
dev_err(hba->dev, "%s failed, err %d\n", __func__, ret);
/* allow force shutdown even in case of errors */
+ pr_err("[DEBUG]%s: UFS SHUTDOWN END\n", __func__);
return 0;
}
EXPORT_SYMBOL(ufshcd_shutdown);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 6ffc08a..3a40d94 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -677,6 +677,7 @@ struct ufs_hba {
u16 ee_ctrl_mask;
u16 hba_enable_delay_us;
bool is_powered;
+ bool shutting_down;
/* Work Queues */
struct work_struct eh_work;
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
