Synchronize user layer access with system PM ops and error handling
Subsystems (2)
v4 cover Synchronize user layer access with system PM ops and error handling
This series contains two changes and it is based on 5.11/scsi-queue
The 1st change is just a minor fix.
The 2nd change is to synchronize user layer access through UFS sysfs nodes, so that system PM ops (suspend, resume and shutdown), error handling and async probe won't be disturbed by user layer access. The protection is only added to some sysfs nodes, not all of them.
Change since v3:
- Rebased to 5.12/scsi-queue
Change since v2:
- Updated the 1st change, added a global boolean flag to tell if system suspend is invoked when hba is NULL, it is used during resume in case of hba becomes not NULL.
Change since v1:
- Slightly updated the 2nd change, added a dedicated inline func to check hba->shutting_down in ufshcd.h. This inline func can be updated to add more rules for sysfs passage in future.
Can Guo (2):
scsi: ufs: Fix a possible NULL pointer issue
scsi: ufs: Protect PM ops and err_handler from user access through
sysfs
drivers/scsi/ufs/ufs-sysfs.c | 106 ++++++++++++++++++++++++++++++++++++------- drivers/scsi/ufs/ufshcd.c | 57 +++++++++++++++-------- drivers/scsi/ufs/ufshcd.h | 10 +++- 3 files changed, 138 insertions(+), 35 deletions(-)
Signature · 1 line
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v4 1/2 scsi: ufs: Fix a possible NULL pointer issue
During system resume/suspend, hba could be NULL. In this case, do not touch eh_sem.
--- drivers/scsi/ufs/ufshcd.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
drivers/scsi/ufs/ufshcd.c +14 -3
@@ -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) {
@@ -8953,8 +8955,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) ==
@@ -9002,9 +9010,12 @@ int ufshcd_system_resume(struct ufs_hba *hba)int ret =0 ;ktime_t start =ktime_get (); -if (!hba) { -up (&hba->eh_sem); +if (!hba)return -EINVAL ; + +if (unlikely (early_suspend)) { + early_suspend = false; +down (&hba->eh_sem); }if (!hba->is_powered ||pm_runtime_suspended (hba->dev))
Signature · 7 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. _______________________________________________ Linux-mediatek mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-mediatek
PATCH v4 2/2 scsi: ufs: Protect PM ops and err_handler from user access through sysfs
In reply to Can GuoUser 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.
--- drivers/scsi/ufs/ufs-sysfs.c | 106 ++++++++++++++++++++++++++++++++++++------- drivers/scsi/ufs/ufshcd.c | 42 ++++++++++------- drivers/scsi/ufs/ufshcd.h | 10 +++- 3 files changed, 125 insertions(+), 33 deletions(-)
drivers/scsi/ufs/ufs-sysfs.c +90 -16
@@ -154,18 +154,29 @@ static ssize_t auto_hibern8_show(struct device *dev, struct device_attribute *attr, char *buf) { u32 ahit; +int ret;struct ufs_hba *hba =dev_get_drvdata (dev);if (!ufshcd_is_auto_hibern8_supported (hba))return -EOPNOTSUPP ; +down (&hba->host_sem); +if (!ufshcd_is_sysfs_allowed (hba)) { + ret = -EBUSY ; + goto out; + } +pm_runtime_get_sync (hba->dev);ufshcd_hold (hba, false); ahit =ufshcd_readl (hba,REG_AUTO_HIBERNATE_IDLE_TIMER );ufshcd_release (hba);pm_runtime_put_sync (hba->dev); -return sysfs_emit (buf,"%d\n" ,ufshcd_ahit_to_us (ahit)); + ret =sysfs_emit (buf,"%d\n" ,ufshcd_ahit_to_us (ahit)); + +out: +up (&hba->host_sem); +return ret; } static ssize_t auto_hibern8_store(struct device *dev,
@@ -174,6 +185,7 @@ static ssize_t auto_hibern8_store(struct device *dev, {struct ufs_hba *hba =dev_get_drvdata (dev);unsigned int timer; +int ret =0 ;if (!ufshcd_is_auto_hibern8_supported (hba))return -EOPNOTSUPP ;
@@ -184,9 +196,17 @@ static ssize_t auto_hibern8_store(struct device *dev,if (timer >UFSHCI_AHIBERN8_MAX )return -EINVAL ; +down (&hba->host_sem); +if (!ufshcd_is_sysfs_allowed (hba)) { + ret = -EBUSY ; + goto out; + } +ufshcd_auto_hibern8_update (hba,ufshcd_us_to_ahit (timer)); -return count; +out: +up (&hba->host_sem); +return ret ? ret : count; }static DEVICE_ATTR_RW (rpm_lvl);
@@ -225,12 +245,21 @@ static ssize_t ufs_sysfs_read_desc_param(struct ufs_hba *hba,if (param_size >8 )return -EINVAL ; +down (&hba->host_sem); +if (!ufshcd_is_sysfs_allowed (hba)) { + ret = -EBUSY ; + goto out; + } +pm_runtime_get_sync (hba->dev); ret =ufshcd_read_desc_param (hba, desc_id, desc_index, param_offset, desc_buf, param_size);pm_runtime_put_sync (hba->dev); -if (ret) -return -EINVAL ; +if (ret) { + ret = -EINVAL ; + goto out; + } +switch (param_size) {case 1 : ret =sysfs_emit (sysfs_buf,"0x%02X\n" , *desc_buf);
@@ -249,6 +278,8 @@ static ssize_t ufs_sysfs_read_desc_param(struct ufs_hba *hba,break ; } +out: +up (&hba->host_sem);return ret; }
@@ -591,9 +622,16 @@ static ssize_t _name##_show(struct device *dev, \int desc_len =QUERY_DESC_MAX_SIZE ; \u8 *desc_buf; \ \ +down (&hba->host_sem); \ +if (!ufshcd_is_sysfs_allowed (hba)) { \ +up (&hba->host_sem); \ +return -EBUSY ; \ + } \ desc_buf =kzalloc (QUERY_DESC_MAX_SIZE ,GFP_ATOMIC ); \ -if (!desc_buf) \ -return -ENOMEM ; \ +if (!desc_buf) { \ +up (&hba->host_sem); \ +return -ENOMEM ; \ + } \pm_runtime_get_sync (hba->dev); \ ret = ufshcd_query_descriptor_retry(hba, \ UPIU_QUERY_OPCODE_READ_DESC, QUERY_DESC_IDN_DEVICE, \
@@ -609,10 +647,11 @@ static ssize_t _name##_show(struct device *dev, \ SD_ASCII_STD); \if (ret <0 ) \ goto out; \ - ret =sysfs_emit (buf,"%s\n" , desc_buf); \ + ret =sysfs_emit (buf,"%s\n" , desc_buf); \ out: \pm_runtime_put_sync (hba->dev); \kfree (desc_buf); \ +up (&hba->host_sem); \return ret; \ } \static DEVICE_ATTR_RO (_name)
@@ -651,15 +690,26 @@ static ssize_t _name##_show(struct device *dev, \u8 index =0 ; \int ret; \struct ufs_hba *hba =dev_get_drvdata (dev); \ + \ +down (&hba->host_sem); \ +if (!ufshcd_is_sysfs_allowed (hba)) { \ +up (&hba->host_sem); \ +return -EBUSY ; \ + } \ if (ufshcd_is_wb_flags(QUERY_FLAG_IDN##_uname)) \ index =ufshcd_wb_get_query_index (hba); \pm_runtime_get_sync (hba->dev); \ ret =ufshcd_query_flag (hba,UPIU_QUERY_OPCODE_READ_FLAG , \ QUERY_FLAG_IDN##_uname, index, &flag); \pm_runtime_put_sync (hba->dev); \ -if (ret) \ -return -EINVAL ; \ -return sysfs_emit (buf,"%s\n" , flag ?"true" :"false" ); \ +if (ret) { \ + ret = -EINVAL ; \ + goto out; \ + } \ + ret =sysfs_emit (buf,"%s\n" , flag ?"true" :"false" ); \ +out: \ +up (&hba->host_sem); \ +return ret; \ } \static DEVICE_ATTR_RO (_name)
@@ -709,15 +759,26 @@ static ssize_t _name##_show(struct device *dev, \u32 value; \int ret; \u8 index =0 ; \ + \ +down (&hba->host_sem); \ +if (!ufshcd_is_sysfs_allowed (hba)) { \ +up (&hba->host_sem); \ +return -EBUSY ; \ + } \ if (ufshcd_is_wb_attrs(QUERY_ATTR_IDN##_uname)) \ index =ufshcd_wb_get_query_index (hba); \pm_runtime_get_sync (hba->dev); \ ret =ufshcd_query_attr (hba,UPIU_QUERY_OPCODE_READ_ATTR , \ QUERY_ATTR_IDN##_uname, index, 0, &value); \pm_runtime_put_sync (hba->dev); \ -if (ret) \ -return -EINVAL ; \ -return sysfs_emit (buf,"0x%08X\n" , value); \ +if (ret) { \ + ret = -EINVAL ; \ + goto out; \ + } \ + ret =sysfs_emit (buf,"0x%08X\n" , value); \ +out: \ +up (&hba->host_sem); \ +return ret; \ } \static DEVICE_ATTR_RO (_name)
@@ -850,13 +911,26 @@ static ssize_t dyn_cap_needed_attribute_show(struct device *dev,u8 lun =ufshcd_scsi_to_upiu_lun (sdev->lun);int ret; +down (&hba->host_sem); +if (!ufshcd_is_sysfs_allowed (hba)) { + ret = -EBUSY ; + goto out; + } +pm_runtime_get_sync (hba->dev); ret =ufshcd_query_attr (hba,UPIU_QUERY_OPCODE_READ_ATTR ,QUERY_ATTR_IDN_DYN_CAP_NEEDED , lun,0 , &value);pm_runtime_put_sync (hba->dev); -if (ret) -return -EINVAL ; -return sysfs_emit (buf,"0x%08X\n" , value); +if (ret) { + ret = -EINVAL ; + goto out; + } + + ret =sysfs_emit (buf,"0x%08X\n" , value); + +out: +up (&hba->host_sem); +return ret; }static DEVICE_ATTR_RO (dyn_cap_needed_attribute);
drivers/scsi/ufs/ufshcd.c +26 -16
132 lines not on this page · raw message
drivers/scsi/ufs/ufshcd.h +9 -1
@@ -663,6 +663,8 @@ struct ufs_hba_variant_params {* @intr_mask: Interrupt Mask Bits * @ee_ctrl_mask: Exception event control mask * @is_powered: flag to check if HBA is powered +* @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
@@ -759,7 +761,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;
@@ -883,6 +886,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; +} +#define ufshcd_writel (hba, val, reg) \ writel((val), (hba)->mmio_base + (reg))#define ufshcd_readl (hba, reg) \
Signature · 7 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. _______________________________________________ Linux-mediatek mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-mediatek
Re: [PATCH v4 2/2] scsi: ufs: Protect PM ops and err_handler from user access through sysfs
Quoted 19 lines · Can Guo
User layer may access sysfs nodes when system PM ops or error handling is running, which can cause various problems. Rename
eh_semtohost_semand use it to protect PM ops and error handling from user layer intervene.Acked-by Avri Altman <[email protected]>Reviewed-by Stanley Chu <[email protected]>Signed-off-by Can Guo <[email protected]>--- drivers/scsi/ufs/ufs-sysfs.c | 106 ++++++++++++++++++++++++++++++++++++------- drivers/scsi/ufs/ufshcd.c | 42 ++++++++++------- drivers/scsi/ufs/ufshcd.h | 10 +++- 3 files changed, 125 insertions(+), 33 deletions(-)drivers/scsi/ufs/ufs-sysfs.c +0
-0@@ -154,18 +154,29 @@ static ssize_t auto_hibern8_show(struct device *dev, struct device_attribute *attr, char *buf)
drivers/scsi/ufs/ufs-sysfs.c static ssize_t auto_hibern8_show(struct device *dev, · line 164
Quoted 10 lines
drivers/scsi/ufs/ufs-sysfs.c +3
-0{u32 ahit; +int ret;struct ufs_hba *hba =dev_get_drvdata (dev);if (!ufshcd_is_auto_hibern8_supported (hba))return -EOPNOTSUPP ; +down (&hba->host_sem); +if (!ufshcd_is_sysfs_allowed (hba)) {
I expect debugfs has the same potential problem, so maybe ufshcd_is_sysfs_allowed() is not quite the right name.
drivers/scsi/ufs/ufs-sysfs.c static ssize_t auto_hibern8_show(struct device *dev, · line 168
Quoted 4 lines
drivers/scsi/ufs/ufs-sysfs.c +4
-0+ ret = -EBUSY ; + goto out; + } +
_______________________________________________
Linux-mediatek mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-mediatek
Re: [PATCH v4 2/2] scsi: ufs: Protect PM ops and err_handler from user access through sysfs
Quoted 27 lines · Adrian Hunter
Quoted 26 lines · Can Guo
User layer may access sysfs nodes when system PM ops or error handling is running, which can cause various problems. Rename
eh_semtohost_semand use it to protect PM ops and error handling from user layer intervene.Acked-by Avri Altman <[email protected]>Reviewed-by Stanley Chu <[email protected]>Signed-off-by Can Guo <[email protected]>--- drivers/scsi/ufs/ufs-sysfs.c | 106 ++++++++++++++++++++++++++++++++++++------- drivers/scsi/ufs/ufshcd.c | 42 ++++++++++------- drivers/scsi/ufs/ufshcd.h | 10 +++- 3 files changed, 125 insertions(+), 33 deletions(-)drivers/scsi/ufs/ufs-sysfs.c +1
-0@@ -154,18 +154,29 @@ static ssize_t auto_hibern8_show(struct device *dev, struct device_attribute *attr, char *buf) {u32 ahit; +int ret;struct ufs_hba *hba =dev_get_drvdata (dev);
Quoted 8 lines
drivers/scsi/ufs/ufs-sysfs.c static ssize_t auto_hibern8_show(struct device *dev, · line 164
Quoted 5 lines
drivers/scsi/ufs/ufs-sysfs.c +2
-0if (!ufshcd_is_auto_hibern8_supported (hba))return -EOPNOTSUPP ; +down (&hba->host_sem); +if (!ufshcd_is_sysfs_allowed (hba)) {I expect debugfs has the same potential problem, so maybe
ufshcd_is_sysfs_allowed()is not quite the right name.
I noticed your debugfs change - currently it is only printing error counts recorded in hba struct, which is fine.
Even in this patch, the check is only added for those entries which need to talk with HW. Sysfs nodes like show/store rpm/spm_lvl does not need this check.
Thanks,
Can Guo.
Quoted 5 lines
drivers/scsi/ufs/ufs-sysfs.c static ssize_t auto_hibern8_show(struct device *dev, · line 168
Quoted 5 lines
drivers/scsi/ufs/ufs-sysfs.c +4
-0+ ret = -EBUSY ; + goto out; + } +
_______________________________________________
Linux-mediatek mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-mediatek