[PATCH v2 0/2] Synchronize user layer access with system PM ops and error handling

[PATCH v2 0/2] Synchronize user layer access with system PM ops and error handling

[Can Guo]

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 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 | 104 ++++++++++++++++++++++++++++++++++++-------
 drivers/scsi/ufs/ufshcd.c    |  49 ++++++++++++--------
 drivers/scsi/ufs/ufshcd.h    |  10 ++++-
 3 files changed, 128 insertions(+), 35 deletions(-)

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 1/2] scsi: ufs: Fix a possible NULL pointer issue

[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 <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e221add..34e2541 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8896,8 +8896,11 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
 	int ret = 0;
 	ktime_t start = ktime_get();
 
+	if (!hba)
+		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,10 +8948,8 @@ 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 (!hba->is_powered || pm_runtime_suspended(hba->dev))
 		/*
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 2/2] scsi: ufs: Protect PM ops and err_handler from user access through sysfs

[Can Guo]

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 <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufs-sysfs.c | 104 ++++++++++++++++++++++++++++++++++++-------
 drivers/scsi/ufs/ufshcd.c    |  40 ++++++++++-------
 drivers/scsi/ufs/ufshcd.h    |  10 ++++-
 3 files changed, 123 insertions(+), 31 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 08e72b7..98a9447 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -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 scnprintf(buf, PAGE_SIZE, "%d\n", ufshcd_ahit_to_us(ahit));
+	ret = scnprintf(buf, PAGE_SIZE, "%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 = sprintf(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,	\
@@ -613,6 +651,7 @@ static ssize_t _name##_show(struct device *dev,				\
 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 sprintf(buf, "%s\n", flag ? "true" : "false"); \
+	if (ret) {							\
+		ret = -EINVAL;						\
+		goto out;						\
+	}								\
+	ret = sprintf(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 sprintf(buf, "0x%08X\n", value);				\
+	if (ret) {							\
+		ret = -EINVAL;						\
+		goto out;						\
+	}								\
+	ret = sprintf(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 sprintf(buf, "0x%08X\n", value);
+	if (ret) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = sprintf(buf, "0x%08X\n", value);
+
+out:
+	up(&hba->host_sem);
+	return ret;
 }
 static DEVICE_ATTR_RO(dyn_cap_needed_attribute);
 
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 34e2541..6c3006a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1522,11 +1522,17 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);
 	u32 value;
-	int err;
+	int err = 0;
 
 	if (kstrtou32(buf, 0, &value))
 		return -EINVAL;
 
+	down(&hba->host_sem);
+	if (!ufshcd_is_sysfs_allowed(hba)) {
+		err = -EBUSY;
+		goto out;
+	}
+
 	value = !!value;
 	if (value == hba->clk_scaling.is_allowed)
 		goto out;
@@ -1552,7 +1558,8 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
 	ufshcd_release(hba);
 	pm_runtime_put_sync(hba->dev);
 out:
-	return count;
+	up(&hba->host_sem);
+	return err ? err : count;
 }
 
 static void ufshcd_clkscaling_init_sysfs(struct ufs_hba *hba)
@@ -5720,9 +5727,10 @@ static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
 
 static inline bool ufshcd_err_handling_should_stop(struct ufs_hba *hba)
 {
-	return (!hba->is_powered || hba->ufshcd_state == UFSHCD_STATE_ERROR ||
+	return (!hba->is_powered || hba->shutting_down ||
+		hba->ufshcd_state == UFSHCD_STATE_ERROR ||
 		(!(hba->saved_err || hba->saved_uic_err || hba->force_reset ||
-			ufshcd_is_link_broken(hba))));
+		   ufshcd_is_link_broken(hba))));
 }
 
 #ifdef CONFIG_PM
@@ -5792,13 +5800,13 @@ static void ufshcd_err_handler(struct work_struct *work)
 
 	hba = container_of(work, struct ufs_hba, eh_work);
 
-	down(&hba->eh_sem);
+	down(&hba->host_sem);
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	if (ufshcd_err_handling_should_stop(hba)) {
 		if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
 			hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
 		spin_unlock_irqrestore(hba->host->host_lock, flags);
-		up(&hba->eh_sem);
+		up(&hba->host_sem);
 		return;
 	}
 	ufshcd_set_eh_in_progress(hba);
@@ -5967,7 +5975,7 @@ static void ufshcd_err_handler(struct work_struct *work)
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 	ufshcd_scsi_unblock_requests(hba);
 	ufshcd_err_handling_unprepare(hba);
-	up(&hba->eh_sem);
+	up(&hba->host_sem);
 }
 
 /**
@@ -7869,10 +7877,10 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 	struct ufs_hba *hba = (struct ufs_hba *)data;
 	int ret;
 
-	down(&hba->eh_sem);
+	down(&hba->host_sem);
 	/* Initialize hba, detect and initialize UFS device */
 	ret = ufshcd_probe_hba(hba, true);
-	up(&hba->eh_sem);
+	up(&hba->host_sem);
 	if (ret)
 		goto out;
 
@@ -8899,7 +8907,7 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
 	if (!hba)
 		return 0;
 
-	down(&hba->eh_sem);
+	down(&hba->host_sem);
 	if (!hba->is_powered)
 		return 0;
 
@@ -8931,7 +8939,7 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
 	if (!ret)
 		hba->is_sys_suspended = true;
 	else
-		up(&hba->eh_sem);
+		up(&hba->host_sem);
 	return ret;
 }
 EXPORT_SYMBOL(ufshcd_system_suspend);
@@ -8965,7 +8973,7 @@ int ufshcd_system_resume(struct ufs_hba *hba)
 		hba->curr_dev_pwr_mode, hba->uic_link_state);
 	if (!ret)
 		hba->is_sys_suspended = false;
-	up(&hba->eh_sem);
+	up(&hba->host_sem);
 	return ret;
 }
 EXPORT_SYMBOL(ufshcd_system_resume);
@@ -9057,7 +9065,10 @@ int ufshcd_shutdown(struct ufs_hba *hba)
 {
 	int ret = 0;
 
-	down(&hba->eh_sem);
+	down(&hba->host_sem);
+	hba->shutting_down = true;
+	up(&hba->host_sem);
+
 	if (!hba->is_powered)
 		goto out;
 
@@ -9075,7 +9086,6 @@ int ufshcd_shutdown(struct ufs_hba *hba)
 	if (ret)
 		dev_err(hba->dev, "%s failed, err %d\n", __func__, ret);
 	hba->is_powered = false;
-	up(&hba->eh_sem);
 	/* allow force shutdown even in case of errors */
 	return 0;
 }
@@ -9270,7 +9280,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	INIT_WORK(&hba->eh_work, ufshcd_err_handler);
 	INIT_WORK(&hba->eeh_work, ufshcd_exception_event_handler);
 
-	sema_init(&hba->eh_sem, 1);
+	sema_init(&hba->host_sem, 1);
 
 	/* Initialize UIC command mutex */
 	mutex_init(&hba->uic_cmd_mutex);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 9bb5f0e..3e91951 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -655,6 +655,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
@@ -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;
@@ -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;
+}
+
 #define ufshcd_writel(hba, val, reg)	\
 	writel((val), (hba)->mmio_base + (reg))
 #define ufshcd_readl(hba, reg)	\
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2 1/2] scsi: ufs: Fix a possible NULL pointer issue

[Bart Van Assche]

On 12/31/20 9:44 PM, Can Guo wrote:
> 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 <cang@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index e221add..34e2541 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8896,8 +8896,11 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
>  	int ret = 0;
>  	ktime_t start = ktime_get();
>  
> +	if (!hba)
> +		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,10 +8948,8 @@ 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 (!hba->is_powered || pm_runtime_suspended(hba->dev))
>  		/*

Hi Can,

How can ufshcd_system_suspend() or ufshcd_system_resume() be called with a
NULL argument? In ufshcd_pci_probe() I see that pci_set_drvdata() is called
before pm_runtime_allow(). ufshcd_pci_remove() calls pm_runtime_forbid().

Thanks,

Bart.

Re: [PATCH v2 1/2] scsi: ufs: Fix a possible NULL pointer issue

[Can Guo]

On 2021-01-02 00:05, Bart Van Assche wrote:
> On 12/31/20 9:44 PM, Can Guo wrote:
>> 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 <cang@codeaurora.org>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index e221add..34e2541 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -8896,8 +8896,11 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
>>  	int ret = 0;
>>  	ktime_t start = ktime_get();
>> 
>> +	if (!hba)
>> +		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,10 +8948,8 @@ 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 (!hba->is_powered || pm_runtime_suspended(hba->dev))
>>  		/*
> 
> Hi Can,
> 
> How can ufshcd_system_suspend() or ufshcd_system_resume() be called 
> with a
> NULL argument? In ufshcd_pci_probe() I see that pci_set_drvdata() is 
> called
> before pm_runtime_allow(). ufshcd_pci_remove() calls 
> pm_runtime_forbid().
> 
> Thanks,
> 
> Bart.

Hi Bart,

You are right about ufshcd_RUNTIME_suspend/resume() - 
platform_set_drvdata()
is called before pm_runtime_enable(), so runtime suspend/resume cannot 
happen
before pm_runtime_enable() is called. We can remove the sanity checks of
!hba there, they are outdated.

But for ufshcd_SYSTEM_suspend/resume() callbacks (not runtime ones), my
understanding is that system suspend/resume may happen after probe 
(vendor
driver probe calls ufshcd_pltfrm_init()) starts but before 
platform_set_drvdata()
is called, in this case hba is NULL.

int ufshcd_pltfrm_init(struct platform_device *pdev,
		       const struct ufs_hba_variant_ops *vops)
{
...
  	platform_set_drvdata(pdev, hba);

	pm_runtime_set_active(&pdev->dev);
	pm_runtime_enable(&pdev->dev);
}

Thanks,

Can Guo.

Re: [PATCH v2 1/2] scsi: ufs: Fix a possible NULL pointer issue

[Can Guo]

On 2021-01-02 20:29, Can Guo wrote:
> On 2021-01-02 00:05, Bart Van Assche wrote:
>> On 12/31/20 9:44 PM, Can Guo wrote:
>>> 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 <cang@codeaurora.org>
>>> ---
>>>  drivers/scsi/ufs/ufshcd.c | 9 +++++----
>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index e221add..34e2541 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -8896,8 +8896,11 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
>>>  	int ret = 0;
>>>  	ktime_t start = ktime_get();
>>> 
>>> +	if (!hba)
>>> +		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,10 +8948,8 @@ 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 (!hba->is_powered || pm_runtime_suspended(hba->dev))
>>>  		/*
>> 
>> Hi Can,
>> 
>> How can ufshcd_system_suspend() or ufshcd_system_resume() be called 
>> with a
>> NULL argument? In ufshcd_pci_probe() I see that pci_set_drvdata() is 
>> called
>> before pm_runtime_allow(). ufshcd_pci_remove() calls 
>> pm_runtime_forbid().
>> 
>> Thanks,
>> 
>> Bart.
> 
> Hi Bart,
> 
> You are right about ufshcd_RUNTIME_suspend/resume() - 
> platform_set_drvdata()
> is called before pm_runtime_enable(), so runtime suspend/resume cannot 
> happen
> before pm_runtime_enable() is called. We can remove the sanity checks 
> of
> !hba there, they are outdated.

Add more history here - before Stanley's change (see below), 
platform_set_drvdata()
is called AFTER pm_runtime_enable(), which was why we needed sanity 
checks of !hba.
But now the sanity checks are unnecessary in 
ufshcd_RUNTIME_suspend/resume(), so
feel free to remove them.

But still, things are a bit different for 
ufshcd_SYSTEM_suspend/resume(), we need
the sanity checks of !hba there if my understanding is correct.

commit 24e2e7a19f7e4b83d0d5189040d997bce3596473
Author: Stanley Chu <stanley.chu@mediatek.com>
Date:   Wed Jun 12 23:19:05 2019 +0800

     scsi: ufs: Avoid runtime suspend possibly being blocked forever

Thanks,
Can Guo.

> 
> But for ufshcd_SYSTEM_suspend/resume() callbacks (not runtime ones), my
> understanding is that system suspend/resume may happen after probe 
> (vendor
> driver probe calls ufshcd_pltfrm_init()) starts but before
> platform_set_drvdata()
> is called, in this case hba is NULL.
> 
> int ufshcd_pltfrm_init(struct platform_device *pdev,
> 		       const struct ufs_hba_variant_ops *vops)
> {
> ...
>  	platform_set_drvdata(pdev, hba);
> 
> 	pm_runtime_set_active(&pdev->dev);
> 	pm_runtime_enable(&pdev->dev);
> }
> 
> Thanks,
> 
> Can Guo.

Re: [PATCH v2 1/2] scsi: ufs: Fix a possible NULL pointer issue

[Adrian Hunter]

On 2/01/21 3:10 pm, Can Guo wrote:
> On 2021-01-02 20:29, Can Guo wrote:
>> On 2021-01-02 00:05, Bart Van Assche wrote:
>>> On 12/31/20 9:44 PM, Can Guo wrote:
>>>> 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 <cang@codeaurora.org>
>>>> ---
>>>>  drivers/scsi/ufs/ufshcd.c | 9 +++++----
>>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>> index e221add..34e2541 100644
>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>> @@ -8896,8 +8896,11 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
>>>>      int ret = 0;
>>>>      ktime_t start = ktime_get();
>>>>
>>>> +    if (!hba)
>>>> +        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,10 +8948,8 @@ 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 (!hba->is_powered || pm_runtime_suspended(hba->dev))
>>>>          /*
>>>
>>> Hi Can,
>>>
>>> How can ufshcd_system_suspend() or ufshcd_system_resume() be called with a
>>> NULL argument? In ufshcd_pci_probe() I see that pci_set_drvdata() is called
>>> before pm_runtime_allow(). ufshcd_pci_remove() calls pm_runtime_forbid().
>>>
>>> Thanks,
>>>
>>> Bart.
>>
>> Hi Bart,
>>
>> You are right about ufshcd_RUNTIME_suspend/resume() - platform_set_drvdata()
>> is called before pm_runtime_enable(), so runtime suspend/resume cannot happen
>> before pm_runtime_enable() is called. We can remove the sanity checks of
>> !hba there, they are outdated.
> 
> Add more history here - before Stanley's change (see below),
> platform_set_drvdata()
> is called AFTER pm_runtime_enable(), which was why we needed sanity checks
> of !hba.
> But now the sanity checks are unnecessary in
> ufshcd_RUNTIME_suspend/resume(), so
> feel free to remove them.
> 
> But still, things are a bit different for ufshcd_SYSTEM_suspend/resume(), we
> need
> the sanity checks of !hba there if my understanding is correct.
> 
> commit 24e2e7a19f7e4b83d0d5189040d997bce3596473
> Author: Stanley Chu <stanley.chu@mediatek.com>
> Date:   Wed Jun 12 23:19:05 2019 +0800
> 
>     scsi: ufs: Avoid runtime suspend possibly being blocked forever
> 
> Thanks,
> Can Guo.
> 
>>
>> But for ufshcd_SYSTEM_suspend/resume() callbacks (not runtime ones), my
>> understanding is that system suspend/resume may happen after probe (vendor
>> driver probe calls ufshcd_pltfrm_init()) starts but before
>> platform_set_drvdata()
>> is called, in this case hba is NULL.
>>
>> int ufshcd_pltfrm_init(struct platform_device *pdev,
>>                const struct ufs_hba_variant_ops *vops)
>> {
>> ...
>>      platform_set_drvdata(pdev, hba);
>>
>>     pm_runtime_set_active(&pdev->dev);
>>     pm_runtime_enable(&pdev->dev);
>> }

Hi Can

I expect probe and system suspend are synchronized e.g. by device_lock(), so
hba would not be NULL.  Is there any example of it being NULL in system suspend?

Regards
Adrian

Re: [PATCH v2 1/2] scsi: ufs: Fix a possible NULL pointer issue

[Can Guo]

On 2021-01-15 21:07, Adrian Hunter wrote:
> On 2/01/21 3:10 pm, Can Guo wrote:
>> On 2021-01-02 20:29, Can Guo wrote:
>>> On 2021-01-02 00:05, Bart Van Assche wrote:
>>>> On 12/31/20 9:44 PM, Can Guo wrote:
>>>>> 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 <cang@codeaurora.org>
>>>>> ---
>>>>>  drivers/scsi/ufs/ufshcd.c | 9 +++++----
>>>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>>> 
>>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>>> index e221add..34e2541 100644
>>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>>> @@ -8896,8 +8896,11 @@ int ufshcd_system_suspend(struct ufs_hba 
>>>>> *hba)
>>>>>      int ret = 0;
>>>>>      ktime_t start = ktime_get();
>>>>> 
>>>>> +    if (!hba)
>>>>> +        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,10 +8948,8 @@ 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 (!hba->is_powered || pm_runtime_suspended(hba->dev))
>>>>>          /*
>>>> 
>>>> Hi Can,
>>>> 
>>>> How can ufshcd_system_suspend() or ufshcd_system_resume() be called 
>>>> with a
>>>> NULL argument? In ufshcd_pci_probe() I see that pci_set_drvdata() is 
>>>> called
>>>> before pm_runtime_allow(). ufshcd_pci_remove() calls 
>>>> pm_runtime_forbid().
>>>> 
>>>> Thanks,
>>>> 
>>>> Bart.
>>> 
>>> Hi Bart,
>>> 
>>> You are right about ufshcd_RUNTIME_suspend/resume() - 
>>> platform_set_drvdata()
>>> is called before pm_runtime_enable(), so runtime suspend/resume 
>>> cannot happen
>>> before pm_runtime_enable() is called. We can remove the sanity checks 
>>> of
>>> !hba there, they are outdated.
>> 
>> Add more history here - before Stanley's change (see below),
>> platform_set_drvdata()
>> is called AFTER pm_runtime_enable(), which was why we needed sanity 
>> checks
>> of !hba.
>> But now the sanity checks are unnecessary in
>> ufshcd_RUNTIME_suspend/resume(), so
>> feel free to remove them.
>> 
>> But still, things are a bit different for 
>> ufshcd_SYSTEM_suspend/resume(), we
>> need
>> the sanity checks of !hba there if my understanding is correct.
>> 
>> commit 24e2e7a19f7e4b83d0d5189040d997bce3596473
>> Author: Stanley Chu <stanley.chu@mediatek.com>
>> Date:   Wed Jun 12 23:19:05 2019 +0800
>> 
>>     scsi: ufs: Avoid runtime suspend possibly being blocked forever
>> 
>> Thanks,
>> Can Guo.
>> 
>>> 
>>> But for ufshcd_SYSTEM_suspend/resume() callbacks (not runtime ones), 
>>> my
>>> understanding is that system suspend/resume may happen after probe 
>>> (vendor
>>> driver probe calls ufshcd_pltfrm_init()) starts but before
>>> platform_set_drvdata()
>>> is called, in this case hba is NULL.
>>> 
>>> int ufshcd_pltfrm_init(struct platform_device *pdev,
>>>                const struct ufs_hba_variant_ops *vops)
>>> {
>>> ...
>>>      platform_set_drvdata(pdev, hba);
>>> 
>>>     pm_runtime_set_active(&pdev->dev);
>>>     pm_runtime_enable(&pdev->dev);
>>> }
> 
> Hi Can
> 
> I expect probe and system suspend are synchronized e.g. by 
> device_lock(), so
> hba would not be NULL.  Is there any example of it being NULL in system 
> suspend?
> 
> Regards
> Adrian

Hi Adrian,

Thanks for the remind - I didn't notice they are protected by 
device_lock().
You are right, hba cannot be NULL in current code... Maybe if (!hba) was
there just for a sanity check. I will make a change to remove these 
checks.

Thanks,
Can Guo.