Synchronize user layer access with system PM ops and error handling

During system resume/suspend, hba could be NULL. In this case, do not touch eh_sem.

Fixes 88a92d6ae4fe ("scsi: ufs: Serialize eh_work with system PM events and async scan")

Signed-off-by Can Guo <[email protected]>

drivers/scsi/ufs/ufshcd.c +9 -1

index e221add..9829c8d 100644
@@ -94,6 +94,8 @@
 		       16, 4, buf, __len, false);                        \
 } while (0)
 
+static bool early_suspend;
+
 int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
 		     const char *prefix)
 {

@@ -8896,8 +8898,14 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
 	int ret = 0;
 	ktime_t start = ktime_get();
 
+	if (!hba) {
+		early_suspend = true;
+		return 0;
+	}
+
 	down(&hba->eh_sem);
-	if (!hba || !hba->is_powered)
+
+	if (!hba->is_powered)
 		return 0;
 
 	if ((ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl) ==

@@ -8945,9 +8953,12 @@ int ufshcd_system_resume(struct ufs_hba *hba)
 	int ret = 0;
 	ktime_t start = ktime_get();

drivers/scsi/ufs/ufshcd.c +0 -0

 	if (!hba->is_powered || pm_runtime_suspended(hba->dev))

Hi Can,

Quoted 40 lines · Can Guo

During system resume/suspend, hba could be NULL. In this case, do not touch eh_sem.

Fixes 88a92d6ae4fe ("scsi: ufs: Serialize eh_work with system PM events and async scan")

Signed-off-by Can Guo <[email protected]>

drivers/scsi/ufs/ufshcd.c +9 -1

index e221add..9829c8d 100644
@@ -94,6 +94,8 @@
 		       16, 4, buf, __len, false);                        \
 } while (0)
 
+static bool early_suspend;
+
 int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
 		     const char *prefix)
 {

@@ -8896,8 +8898,14 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
 	int ret = 0;
 	ktime_t start = ktime_get();
 
+	if (!hba) {
+		early_suspend = true;
+		return 0;
+	}
+
 	down(&hba->eh_sem);
-	if (!hba || !hba->is_powered)
+
+	if (!hba->is_powered)
 		return 0;
 
 	if ((ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl) ==

@@ -8945,9 +8953,12 @@ int ufshcd_system_resume(struct ufs_hba *hba)
 	int ret = 0;
 	ktime_t start = ktime_get();
 

drivers/scsi/ufs/ufshcd.c int ufshcd_system_resume(struct ufs_hba *hba) · line 8962

Quoted 9 lines

drivers/scsi/ufs/ufshcd.c +5 -2

-	if (!hba) {
-		up(&hba->eh_sem);
+	if (!hba)
 		return -EINVAL;
+
+	if (unlikely(early_suspend)) {
+		early_suspend = false;
+		down(&hba->eh_sem);
 	}

I guess early_suspend here is to handle the case that hba is null during ufshcd_system_suspend() but !null during ufshcd_system_resume(). If yes, would it be possible? If no, may I know what is the purpose?

Thanks a lot.
Stanley Chu

Quoted 2 lines

drivers/scsi/ufs/ufshcd.c +0 -0

 	if (!hba->is_powered || pm_runtime_suspended(hba->dev))

Quoted 45 lines · Stanley Chu

Hi Can,

Quoted 42 lines · Can Guo

During system resume/suspend, hba could be NULL. In this case, do not touch
eh_sem.

Fixes 88a92d6ae4fe ("scsi: ufs: Serialize eh_work with system PM

events and async scan")

Signed-off-by Can Guo <[email protected]>

drivers/scsi/ufs/ufshcd.c +9 -1

index e221add..9829c8d 100644
@@ -94,6 +94,8 @@
 		       16, 4, buf, __len, false);                        \
 } while (0)
 
+static bool early_suspend;
+
 int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
 		     const char *prefix)
 {

@@ -8896,8 +8898,14 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
 	int ret = 0;
 	ktime_t start = ktime_get();
 
+	if (!hba) {
+		early_suspend = true;
+		return 0;
+	}
+
 	down(&hba->eh_sem);
-	if (!hba || !hba->is_powered)
+
+	if (!hba->is_powered)
 		return 0;
 
 	if ((ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl) ==

@@ -8945,9 +8953,12 @@ int ufshcd_system_resume(struct ufs_hba *hba)
 	int ret = 0;
 	ktime_t start = ktime_get();
 

Quoted 16 lines

drivers/scsi/ufs/ufshcd.c int ufshcd_system_resume(struct ufs_hba *hba) · line 8962

Quoted 9 lines

drivers/scsi/ufs/ufshcd.c +5 -2

-	if (!hba) {
-		up(&hba->eh_sem);
+	if (!hba)
 		return -EINVAL;
+
+	if (unlikely(early_suspend)) {
+		early_suspend = false;
+		down(&hba->eh_sem);
 	}

I guess early_suspend here is to handle the case that hba is null during
ufshcd_system_suspend() but !null during ufshcd_system_resume(). If yes,
would it be possible? If no, may I know what is the purpose?

Yes, you are right. I think it is possible. platform_set_drvdata() is called in ufshcd_pltfrm_init(). Say suspend happens before platform_set_drvdata() is called, but resume comes back after platform_set_drvdata() is called. What do you think?

+ * @shutting_down: flag to check if shutdown has been invoked

+ * @host_sem: semaphore used to serialize concurrent contexts
* @eh_wq: Workqueue that eh_work works on
* @eh_work: Worker to handle UFS errors that require s/w attention
* @eeh_work: Worker to handle exception events

drivers/scsi/ufs/ufshcd.h +2 -1

@@ -751,7 +753,8 @@ struct ufs_hba {
        u32 intr_mask;
        u16 ee_ctrl_mask;
        bool is_powered;
-       struct semaphore eh_sem;
+       bool shutting_down;
+       struct semaphore host_sem;

Quoted 1 line · Can Guo

+ * @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.

Hi Rafael
would you please help us confirm this?

thanks,
Bean

Quoted 11 lines

+ * @host_sem: semaphore used to serialize concurrent contexts
* @eh_wq: Workqueue that eh_work works on
* @eh_work: Worker to handle UFS errors that require s/w attention
* @eeh_work: Worker to handle exception events

drivers/scsi/ufs/ufshcd.h +2 -1

@@ -751,7 +753,8 @@ struct ufs_hba {
        u32 intr_mask;
        u16 ee_ctrl_mask;
        bool is_powered;
-       struct semaphore eh_sem;
+       bool shutting_down;
+       struct semaphore host_sem;

Quoted 8 lines · Bean Huo

Quoted 1 line · Can Guo

+ * @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.

Quoted 6 lines

Hi Rafael
would you please help us confirm this?

thanks,
Bean

Quoted 18 lines · Can Guo

Quoted 8 lines · Bean Huo

Quoted 1 line · Can Guo

+ * @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/

Quoted 1 line

Can Guo.

Can,

Thanks for pointing out that.

Based on my understanding, that patch is redundent. maybe I misundestood Linux shutdown sequence.

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

3. device_shutdown()
So, userspace thread can not start a request to trigger runtime resume(pm_runtime_get_sync) after step 2.

also, no need to add that flag to checkup if shutdwon is running, maybe it is better to check variable system_state:

if (system_state == SYSTEM_POWER_OFF || system_state == SYSTEM_HALT || system_state == SYSTEM_RESTART)

	//shutdown start

I still hope Rafael or someone else can confirm that if pm_runtime_get_sync() will really return ok in shutdown flow.

thanks,
Bean

Quoted 6 lines

Quoted 6 lines

Hi Rafael
would you please help us confirm this?

thanks,
Bean

Hi Bean,

Quoted 22 lines · Bean Huo

Quoted 20 lines · Can Guo

Quoted 9 lines · Bean Huo

Quoted 2 lines · Can Guo

+ * @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.

Quoted 6 lines

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?

Quoted 8 lines

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.

kernel_poweroff/kernel_halt()->device_shutdown()->platform_shutdown()- ufshcd_platform_shutdown()->ufshcd_shutdown().

264     system_state = state;
265     usermodehelper_disable();
266     device_shutdown();

Thanks,
Can Guo.

Quoted 25 lines · Can Guo

Hi Bean,

Quoted 22 lines · Bean Huo

Quoted 20 lines · Can Guo

Quoted 9 lines · Bean Huo

Quoted 2 lines · Can Guo

+ * @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.

Quoted 8 lines

Quoted 6 lines

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.

Quoted 9 lines

Quoted 8 lines

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

Quoted 5 lines

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()-

Quoted 2 lines

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!!!!!

Thanks,
Bean

Quoted 7 lines

264     system_state = state;
265     usermodehelper_disable();
266     device_shutdown();

Thanks,
Can Guo.

Quoted 27 lines · Can Guo

Hi Bean,

Quoted 24 lines · Bean Huo

Quoted 16 lines · Can Guo

Quoted 9 lines · Bean Huo

Quoted 2 lines · Can Guo

+ * @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.

Quoted 4 lines

Check Stanley's change -
https://lore.kernel.org/patchwork/patch/1341389/

Can Guo.

Can,

Quoted 6 lines

Quoted 4 lines

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.

Quoted 9 lines

Quoted 8 lines

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

Quoted 5 lines

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()-

Quoted 2 lines

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!!!!!

Thanks,
Bean

Quoted 7 lines

264     system_state = state;
265     usermodehelper_disable();
266     device_shutdown();

Thanks,
Can Guo.

Quoted 28 lines · Bean Huo

Quoted 27 lines · Can Guo

Hi Bean,

Quoted 24 lines · Bean Huo

Quoted 16 lines · Can Guo

Quoted 9 lines · Bean Huo

Quoted 2 lines · Can Guo

+ * @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.

Quoted 4 lines

Check Stanley's change -
https://lore.kernel.org/patchwork/patch/1341389/

Can Guo.

Can,

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

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.

Quoted 49 lines

Quoted 9 lines

Quoted 8 lines

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

Quoted 5 lines

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()-

Quoted 2 lines

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)

Quoted 5 lines

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.

Quoted 13 lines

Thanks,
Bean

Quoted 7 lines

264     system_state = state;
265     usermodehelper_disable();
266     device_shutdown();

Thanks,
Can Guo.

Quoted 185 lines · Can Guo

Quoted 31 lines · Bean Huo

Quoted 30 lines · Can Guo

Hi Bean,

Quoted 27 lines · Bean Huo

Quoted 19 lines · Can Guo

Quoted 4 lines · Bean Huo

Quoted 2 lines · Can Guo

+ * @shutting_down: flag to check if shutdown has been invoked

Quoted 8 lines

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.

Quoted 4 lines

Check Stanley's change -
https://lore.kernel.org/patchwork/patch/1341389/

Can Guo.

Can,

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

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.

Quoted 65 lines

Quoted 10 lines

Quoted 9 lines

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

Quoted 6 lines

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()-

Quoted 3 lines

kernel_poweroff/kernel_halt()->device_shutdown()-

Quoted 1 line

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)

Quoted 9 lines

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

Quoted 6 lines

# while true; do cat /sys/devices/platform/soc@0/*ufshc*/rpm_lvl;
done 
&
# reboot -f

Can Guo.

Oops... forgot the logs:

#

# while true; do cat /sys/devices/platform/soc@0/*ufshc*/rpm_lvl;
done &
3
3
3
3
....
# reboot -f
3
3
3
....
[   17.959206] sd 0:0:0:5: [sdf] Synchronizing SCSI cache
3
[   17.964833] sd 0:0:0:4: [sde] Synchronizing SCSI cache
[   17.970224] sd 0:0:0:3: [sdd] Synchronizing SCSI cache
[   17.975574] sd 0:0:0:2: [sdc] Synchronizing SCSI cache
3
[   17.981034] sd 0:0:0:1: [sdb] Synchronizing SCSI cache
[   17.986493] sd 0:0:0:0: [sda] Synchronizing SCSI cache
3
[   17.991870] [DEBUG]ufshcd_shutdown: UFS SHUTDOWN START
[   17.998902] ------------[ cut here ]------------
[   18.003648] kernel BUG at drivers/scsi/ufs/ufs-sysfs.c:62!
[   18.009286] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[   18.034249] pstate: 40c00005 (nZcv daif +PAN +UAO)
[   18.039185] pc : rpm_lvl_show+0x38/0x40
[   18.043137] lr : dev_attr_show+0x1c/0x58
[   18.132552] Call trace:

[   18.135076]  rpm_lvl_show+0x38/0x40
[   18.138672]  sysfs_kf_seq_show+0xa8/0x140
[   18.142802]  kernfs_seq_show+0x28/0x30
[   18.146665]  seq_read+0x1d8/0x4b0
[   18.150072]  kernfs_fop_read+0x12c/0x1f0
[   18.154109]  do_iter_read+0x184/0x1c0
[   18.157882]  vfs_readv+0x68/0xb0
....

Quoted 225 lines · Can Guo

Quoted 185 lines · Can Guo

Quoted 31 lines · Bean Huo

Quoted 30 lines · Can Guo

Hi Bean,

Quoted 27 lines · Bean Huo

Quoted 19 lines · Can Guo

Quoted 4 lines · Bean Huo

Quoted 2 lines · Can Guo

+ * @shutting_down: flag to check if shutdown has been invoked

Quoted 8 lines

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.

Quoted 4 lines

Check Stanley's change -
https://lore.kernel.org/patchwork/patch/1341389/

Can Guo.

Can,

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

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.

Quoted 65 lines

Quoted 10 lines

Quoted 9 lines

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

Quoted 6 lines

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()-

Quoted 3 lines

kernel_poweroff/kernel_halt()->device_shutdown()-

Quoted 1 line

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)

Quoted 9 lines

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

Quoted 6 lines

# while true; do cat /sys/devices/platform/soc@0/*ufshc*/rpm_lvl;
done
&
# reboot -f

Can Guo.

Oops... forgot the logs:

#

# while true; do cat /sys/devices/platform/soc@0/*ufshc*/rpm_lvl;
done &
3
3
3
3
....
# reboot -f
3
3
3
....
[   17.959206] sd 0:0:0:5: [sdf] Synchronizing SCSI cache
3
[   17.964833] sd 0:0:0:4: [sde] Synchronizing SCSI cache
[   17.970224] sd 0:0:0:3: [sdd] Synchronizing SCSI cache
[   17.975574] sd 0:0:0:2: [sdc] Synchronizing SCSI cache
3
[   17.981034] sd 0:0:0:1: [sdb] Synchronizing SCSI cache
[   17.986493] sd 0:0:0:0: [sda] Synchronizing SCSI cache
3
[   17.991870] [DEBUG]ufshcd_shutdown: UFS SHUTDOWN START
[   17.998902] ------------[ cut here ]------------
[   18.003648] kernel BUG at drivers/scsi/ufs/ufs-sysfs.c:62!
[   18.009286] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[   18.034249] pstate: 40c00005 (nZcv daif +PAN +UAO)
[   18.039185] pc : rpm_lvl_show+0x38/0x40
[   18.043137] lr : dev_attr_show+0x1c/0x58
[   18.132552] Call trace:

Quoted 8 lines

[   18.135076]  rpm_lvl_show+0x38/0x40
[   18.138672]  sysfs_kf_seq_show+0xa8/0x140
[   18.142802]  kernfs_seq_show+0x28/0x30
[   18.146665]  seq_read+0x1d8/0x4b0
[   18.150072]  kernfs_fop_read+0x12c/0x1f0
[   18.154109]  do_iter_read+0x184/0x1c0
[   18.157882]  vfs_readv+0x68/0xb0
....

Hi Can
Please forgive me for being verbose.

The above BUG log is from your BUG_ON() called, not becuase of the race betwen shutdown flow and sysfs node access(if I misunderstood, correct me). But it tells that the user can still access UFS by sysfs node in the "reboot -f" flow since it skips the actual shutdown process, " --force" is not a safe way anyway.

If accessing sysfs nodes, which triggers a UFS UPIU request to read/write UFS device descriptors during shutdown flow, there is only one issue that sysfs node access failure since UFS device and LINK has been shutdown. Strictly speaking, the failure comes after ufshcd_set_dev_pwr_mode().

   __ufshcd_query_descriptor: opcode 0x01 for idn 0 failed, index 0,

err = -11

Since the shutdown is oneway process, this failure is not big issue. If you meant to avoid this failure for unsafe shutdown, I agree with you, But for the race issue, I don't know.

Bean

Quoted 9 lines

If accessing sysfs nodes, which triggers a UFS UPIU request to read/write UFS device descriptors during shutdown flow, there is only
one issue that sysfs node access failure since UFS device and LINK has
been shutdown. Strictly speaking, the failure comes after ufshcd_set_dev_pwr_mode().

Quoted 3 lines

    __ufshcd_query_descriptor: opcode 0x01 for idn 0 failed, index

0,
err = -11

You misunderstood it again. You are expecting a simple query cmd error.
But what really matters are NoC issues[1] and OCP[2]. And while/after
UFS
shutting down, either of them may happen.

[1] When a un-clocked register access issue happens, we call it a NoC
issue,
meaning you are tring to access a register when clocks are disabled. This
leads to system CRASH.

[2] OCP is over current protection. While UFS shutting down, you may have put UFS regulators to LPM. After that, if you are still trying to
talk to UFS, OCP can happen on VCCQ/VCCQ2. This leads to system CRASH
too.

Quoted 8 lines

Since the shutdown is oneway process, this failure is not big issue. If
you meant to avoid this failure for unsafe shutdown, I agree with you,
But for the race issue, I don't know.

Easy for you to say. System crash is a big issue to any SoC vendors I
belive.

Can Guo.

Quoted 20 lines · Can Guo

Quoted 9 lines

If accessing sysfs nodes, which triggers a UFS UPIU request to read/write UFS device descriptors during shutdown flow, there is only
one issue that sysfs node access failure since UFS device and LINK has
been shutdown. Strictly speaking, the failure comes after ufshcd_set_dev_pwr_mode().

Quoted 3 lines

    __ufshcd_query_descriptor: opcode 0x01 for idn 0 failed, index

0,
err = -11

You misunderstood it again. You are expecting a simple query cmd error.
But what really matters are NoC issues[1] and OCP[2]. And while/after
UFS
shutting down, either of them may happen.

Quoted 7 lines

[1] When a un-clocked register access issue happens, we call it a NoC
issue,
meaning you are tring to access a register when clocks are disabled. This
leads to system CRASH.

OK, let it simple, share this kind of crash log becuase of access sysfs node in the shutdown flow.

Quoted 6 lines

[2] OCP is over current protection. While UFS shutting down, you may have put UFS regulators to LPM. After that, if you are still trying to
talk to UFS, OCP can happen on VCCQ/VCCQ2. This leads to system CRASH
too.

the same as above, share the crash log.

Quoted 9 lines

Quoted 8 lines

Since the shutdown is oneway process, this failure is not big issue. If
you meant to avoid this failure for unsafe shutdown, I agree with you,
But for the race issue, I don't know.

Quoted 4 lines

Easy for you to say. System crash is a big issue to any SoC vendors I
belive.

indeed, crash is serious issue, share the log.

Thanks,
Bean

Quoted 1 line

Can Guo.

Quoted 20 lines

Quoted 5 lines

meaning you are tring to access a register when clocks are disabled.
This
leads to system CRASH.

OK, let it simple, share this kind of crash log becuase of access sysfs
node in the shutdown flow.

Quoted 8 lines

[2] OCP is over current protection. While UFS shutting down, you may
have put UFS regulators to LPM. After that, if you are still trying
to
talk to UFS, OCP can happen on VCCQ/VCCQ2. This leads to system CRASH
too.

Quoted 2 lines

the same as above, share the crash log.

If you have hand-on experiences on NoC and/or OCP issues, you won't ask
for the crash log. The tricky parts about critial NoC and OCP issues is

Quoted 23 lines · Can Guo

Quoted 22 lines

Quoted 5 lines

meaning you are tring to access a register when clocks are disabled.
This
leads to system CRASH.

OK, let it simple, share this kind of crash log becuase of access sysfs
node in the shutdown flow.

Quoted 8 lines

[2] OCP is over current protection. While UFS shutting down, you may
have put UFS regulators to LPM. After that, if you are still trying
to
talk to UFS, OCP can happen on VCCQ/VCCQ2. This leads to system CRASH
too.

the same as above, share the crash log.

Quoted 4 lines

If you have hand-on experiences on NoC and/or OCP issues, you won't ask
for the crash log. The tricky parts about critial NoC and OCP issues is

OK, interesting. would you tell me which register access node in ufs- sysfs.c can trigger this crash? let me verify your statement.

Bean

Quoted 1 line

Quoted 16 lines

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

to
talk to UFS, OCP can happen on VCCQ/VCCQ2. This leads to system
CRASH
too.

the same as above, share the crash log.

Quoted 6 lines

If you have hand-on experiences on NoC and/or OCP issues, you won't
ask
for the crash log. The tricky parts about critial NoC and OCP issues
is

Quoted 5 lines

OK, interesting. would you tell me which register access node in ufs-
sysfs.c can trigger this crash? let me verify your statement.

I believe I have explained enough to prove we need this change.

If you are really interested in NoC and OCP, feel free to ping me on teams, I will show you how to trigger one and what is it like on my setup.

Can Guo.

Quoted 225 lines · Can Guo

Quoted 185 lines · Can Guo

Quoted 31 lines · Bean Huo

Quoted 30 lines · Can Guo

Hi Bean,

Quoted 27 lines · Bean Huo

Quoted 19 lines · Can Guo

Quoted 4 lines · Bean Huo

Quoted 2 lines · Can Guo

+ * @shutting_down: flag to check if shutdown has been invoked

Quoted 8 lines

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.

Quoted 4 lines

Check Stanley's change -
https://lore.kernel.org/patchwork/patch/1341389/

Can Guo.

Can,

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

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.

Quoted 65 lines

Quoted 10 lines

Quoted 9 lines

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

Quoted 6 lines

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()-

Quoted 3 lines

kernel_poweroff/kernel_halt()->device_shutdown()-

Quoted 1 line

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)

Quoted 9 lines

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

Quoted 6 lines

# while true; do cat /sys/devices/platform/soc@0/*ufshc*/rpm_lvl;
done
&
# reboot -f

Can Guo.

Oops... forgot the logs:

#

# while true; do cat /sys/devices/platform/soc@0/*ufshc*/rpm_lvl;
done &
3
3
3
3
....
# reboot -f
3
3
3
....
[   17.959206] sd 0:0:0:5: [sdf] Synchronizing SCSI cache
3
[   17.964833] sd 0:0:0:4: [sde] Synchronizing SCSI cache
[   17.970224] sd 0:0:0:3: [sdd] Synchronizing SCSI cache
[   17.975574] sd 0:0:0:2: [sdc] Synchronizing SCSI cache
3
[   17.981034] sd 0:0:0:1: [sdb] Synchronizing SCSI cache
[   17.986493] sd 0:0:0:0: [sda] Synchronizing SCSI cache
3
[   17.991870] [DEBUG]ufshcd_shutdown: UFS SHUTDOWN START
[   17.998902] ------------[ cut here ]------------
[   18.003648] kernel BUG at drivers/scsi/ufs/ufs-sysfs.c:62!
[   18.009286] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[   18.034249] pstate: 40c00005 (nZcv daif +PAN +UAO)
[   18.039185] pc : rpm_lvl_show+0x38/0x40
[   18.043137] lr : dev_attr_show+0x1c/0x58
[   18.132552] Call trace:

Quoted 8 lines

[   18.135076]  rpm_lvl_show+0x38/0x40
[   18.138672]  sysfs_kf_seq_show+0xa8/0x140
[   18.142802]  kernfs_seq_show+0x28/0x30
[   18.146665]  seq_read+0x1d8/0x4b0
[   18.150072]  kernfs_fop_read+0x12c/0x1f0
[   18.154109]  do_iter_read+0x184/0x1c0
[   18.157882]  vfs_readv+0x68/0xb0
....

Hi Can
Please forgive me for being verbose.

The above BUG log is from your BUG_ON() called, not becuase of the race betwen shutdown flow and sysfs node access(if I misunderstood, correct me). But it tells that the user can still access UFS by sysfs node in the "reboot -f" flow since it skips the actual shutdown process, " --force" is not a safe way anyway.

If accessing sysfs nodes, which triggers a UFS UPIU request to read/write UFS device descriptors during shutdown flow, there is only one issue that sysfs node access failure since UFS device and LINK has been shutdown. Strictly speaking, the failure comes after ufshcd_set_dev_pwr_mode().

   __ufshcd_query_descriptor: opcode 0x01 for idn 0 failed, index 0,

err = -11

Since the shutdown is oneway process, this failure is not big issue. If you meant to avoid this failure for unsafe shutdown, I agree with you, But for the race issue, I don't know.

Bean

+ * @shutting_down: flag to check if shutdown has been invoked
+ * @host_sem: semaphore used to serialize concurrent contexts
* @eh_wq: Workqueue that eh_work works on
* @eh_work: Worker to handle UFS errors that require s/w attention
* @eeh_work: Worker to handle exception events

drivers/scsi/ufs/ufshcd.h +2 -1

@@ -751,7 +753,8 @@ struct ufs_hba {
        u32 intr_mask;
        u16 ee_ctrl_mask;
        bool is_powered;
-       struct semaphore eh_sem;
+       bool shutting_down;
+       struct semaphore host_sem;
 
        /* Work Queues */
        struct workqueue_struct *eh_wq;

Quoted 16 lines · Can Guo

+ * @shutting_down: flag to check if shutdown has been invoked
+ * @host_sem: semaphore used to serialize concurrent contexts
* @eh_wq: Workqueue that eh_work works on
* @eh_work: Worker to handle UFS errors that require s/w attention
* @eeh_work: Worker to handle exception events

drivers/scsi/ufs/ufshcd.h +2 -1

@@ -751,7 +753,8 @@ struct ufs_hba {
        u32 intr_mask;
        u16 ee_ctrl_mask;
        bool is_powered;
-       struct semaphore eh_sem;
+       bool shutting_down;
+       struct semaphore host_sem;
 
        /* Work Queues */
        struct workqueue_struct *eh_wq;

drivers/scsi/ufs/ufshcd.h static inline bool ufshcd_is_wb_allowed(struct · line 886

Quoted 9 lines

drivers/scsi/ufs/ufshcd.h +5 -0

@@ -875,6 +878,11 @@ static inline bool ufshcd_is_wb_allowed(struct
 ufs_hba *hba)
        return hba->caps & UFSHCD_CAP_WB_EN;
 }
 
+static inline bool ufshcd_is_sysfs_allowed(struct ufs_hba *hba)
+{
+       return !hba->shutting_down;
+}
+

Can,

Instead adding new shutting_down flag, can we use availible variable system_state?

Thanks,
Bean

Quoted 9 lines

drivers/scsi/ufs/ufshcd.h static inline bool ufshcd_is_wb_allowed(struct ufs_hba *hba) · line 885

Quoted 5 lines

drivers/scsi/ufs/ufshcd.h +5 -0

+static inline bool ufshcd_is_sysfs_allowed(struct ufs_hba *hba)
+{
+       return !hba->shutting_down;
+}
+

Can,

Quoted 6 lines

Instead adding new shutting_down flag, can we use availible variable
system_state?

Thanks,
Bean

Hi Bean,

I prefer the flag shutting_down, it tells us whether ufshcd_shutdown()
has been invoked or not. It comes handy when debug some system crash issues caused by UFS during reboot/shutdown tests. system_state is too
wide in this case.

Thanks,
Can Guo.

Quoted 16 lines

+ * @shutting_down: flag to check if shutdown has been invoked
+ * @host_sem: semaphore used to serialize concurrent contexts
* @eh_wq: Workqueue that eh_work works on
* @eh_work: Worker to handle UFS errors that require s/w attention
* @eeh_work: Worker to handle exception events

drivers/scsi/ufs/ufshcd.h +2 -1

@@ -751,7 +753,8 @@ struct ufs_hba {
        u32 intr_mask;
        u16 ee_ctrl_mask;
        bool is_powered;
-       struct semaphore eh_sem;
+       bool shutting_down;
+       struct semaphore host_sem;
 
        /* Work Queues */
        struct workqueue_struct *eh_wq;

drivers/scsi/ufs/ufshcd.h static inline bool ufshcd_is_wb_allowed(struct · line 886

Quoted 9 lines

drivers/scsi/ufs/ufshcd.h +5 -0

@@ -875,6 +878,11 @@ static inline bool ufshcd_is_wb_allowed(struct
 ufs_hba *hba)
        return hba->caps & UFSHCD_CAP_WB_EN;
 }
 
+static inline bool ufshcd_is_sysfs_allowed(struct ufs_hba *hba)
+{
+       return !hba->shutting_down;
+}
+

Can,

Instead adding new shutting_down flag, can we use availible variable system_state?

User layer may access sysfs nodes when system PM ops or error handling is running, which can cause various problems. Rename eh_sem to host_sem and use it to protect PM ops and error handling from user layer intervene.

Signed-off-by Can Guo <[email protected]>

This series contains two changes and it is based on 5.11/scsi-queue

Can,

Quoted 1 line

This series contains two changes and it is based on 5.11/scsi-queue

Please rebase against 5.12/scsi-queue.

Thanks!