Messages in this thread | | | Date | Thu, 24 Jun 2021 10:23:01 +0800 | From | Can Guo <> | Subject | Re: [PATCH v4 10/10] scsi: ufs: Apply more limitations to user access |
| |
Hi Bart,
On 2021-06-24 05:51, Bart Van Assche wrote: > On 6/23/21 12:35 AM, Can Guo wrote: >> +int ufshcd_get_user_access(struct ufs_hba *hba) >> +__acquires(&hba->host_sem) >> +{ >> + down(&hba->host_sem); >> + if (!ufshcd_is_user_access_allowed(hba)) { >> + up(&hba->host_sem); >> + return -EBUSY; >> + } >> + if (ufshcd_rpm_get_sync(hba)) { >> + ufshcd_rpm_put_sync(hba); >> + up(&hba->host_sem); >> + return -EBUSY; >> + } >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(ufshcd_get_user_access); >> + >> +void ufshcd_put_user_access(struct ufs_hba *hba) >> +__releases(&hba->host_sem) >> +{ >> + ufshcd_rpm_put_sync(hba); >> + up(&hba->host_sem); >> +} >> +EXPORT_SYMBOL_GPL(ufshcd_put_user_access); > > Please indent __acquires() and __releases() annotations by one tab as > is > done elsewhere in the kernel.
OK.
> >> static inline bool ufshcd_is_user_access_allowed(struct ufs_hba *hba) >> { >> - return !hba->shutting_down; >> + return !hba->shutting_down && !hba->is_sys_suspended && >> + !hba->is_wlu_sys_suspended && >> + hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL; >> } > > Is my understanding of the following correct? > - ufshcd_is_user_access_allowed() is not in the hot path and hence > should not be inline.
OK.
> - The hba->shutting_down member variable is set from inside a shutdown > callback. Hence, the hba->shutting_down test can be left out since > no UFS sysfs attributes are accessed after shutdown has started.
We see that user can still access UFS sysfs during shutdown if shutdown is invoked by: reboot -f, hence the check.
> - During system suspend, user space software is paused before the > device > driver freeze callbacks are invoked. Hence, the hba->is_sys_suspended > check can be left out.
is_sys_suspended indicates that system resume failed (power/clk is OFF).
> - If a LUN is runtime suspended, it should be resumed if accessed from > user space instead of failing user space accesses. In other words, > the > hba->is_wlu_sys_suspended check seems inappropriate to me.
hba->is_wlu_sys_suspended indicates that wl system resume failed, device is not operational.
> - If the HBA is not in an operational state, user space accesses > should be blocked until error handling has finished. After error > handling has finished, the user space access should fail if and only > if error handling failed. >
Yes, which is why ufshcd_get_user_access() acquires host_sem first and checks the OPERATOINAL flag here. host_sem shall make sure that user space accesses should be blocked until error handling has finished.
Thanks,
Can Guo.
> Thanks, > > Bart.
| |