Re: [PATCH] scsi: ufs: core: Fix data race in CPU latency PM QoS request handling

[Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>]

On 9/2/2025 2:43 PM, Ziqi Chen wrote:
>
> On 9/1/2025 4:51 PM, Zhongqiu Han wrote:
>> The cpu_latency_qos_add/remove/update_request interfaces lack internal
>> synchronization by design, requiring the caller to ensure thread safety.
>> The current implementation relies on the `pm_qos_enabled` flag, which is
>> insufficient to prevent concurrent access and cannot serve as a proper
>> synchronization mechanism. This has led to data races and list 
>> corruption
>> issues.
>>
>> A typical race condition call trace is:
>>
>> [Thread A]
>> ufshcd_pm_qos_exit()
>>    --> cpu_latency_qos_remove_request()
>>      --> cpu_latency_qos_apply();
>>        --> pm_qos_update_target()
>>          --> plist_del              <--(1) delete plist node
>>      --> memset(req, 0, sizeof(*req));
>>    --> hba->pm_qos_enabled = false;
>>
>> [Thread B]
>> ufshcd_devfreq_target
>>    --> ufshcd_devfreq_scale
>>      --> ufshcd_scale_clks
>>        --> ufshcd_pm_qos_update     <--(2) pm_qos_enabled is true
>>          --> cpu_latency_qos_update_request
>>            --> pm_qos_update_target
>>              --> plist_del          <--(3) plist node use-after-free
>>
>> This patch introduces a dedicated mutex to serialize PM QoS operations,
>> preventing data races and ensuring safe access to PM QoS resources.
>> Additionally, READ_ONCE is used in the sysfs interface to ensure atomic
>> read access to pm_qos_enabled flag.
>>
>> Fixes: 2777e73fc154 ("scsi: ufs: core: Add CPU latency QoS support 
>> for UFS driver")
>> Signed-off-by: Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>
>> ---
>>   drivers/ufs/core/ufs-sysfs.c |  2 +-
>>   drivers/ufs/core/ufshcd.c    | 16 ++++++++++++++++
>>   include/ufs/ufshcd.h         |  2 ++
>>   3 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
>> index 4bd7d491e3c5..8f7975010513 100644
>> --- a/drivers/ufs/core/ufs-sysfs.c
>> +++ b/drivers/ufs/core/ufs-sysfs.c
>> @@ -512,7 +512,7 @@ static ssize_t pm_qos_enable_show(struct device 
>> *dev,
>>   {
>>       struct ufs_hba *hba = dev_get_drvdata(dev);
>>   -    return sysfs_emit(buf, "%d\n", hba->pm_qos_enabled);
>> +    return sysfs_emit(buf, "%d\n", READ_ONCE(hba->pm_qos_enabled));
>>   }
>>     /**
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index 926650412eaa..f259fb1790fa 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -1047,14 +1047,18 @@ EXPORT_SYMBOL_GPL(ufshcd_is_hba_active);
>>    */
>>   void ufshcd_pm_qos_init(struct ufs_hba *hba)
>>   {
>> +    mutex_lock(&hba->pm_qos_mutex);
>>         if (hba->pm_qos_enabled)
>> +        mutex_unlock(&hba->pm_qos_mutex);
>>           return;
> Missing the curly braces for this If statement.

Hi Ziqi,
Thanks for the review, yes, i will fix it on v2
https://lore.kernel.org/all/20250902074829.657343-1-zhongqiu.han@oss.qualcomm.com/
  
The internal test version does not contain this bug; in fact,
the internal test version is correct.

>> cpu_latency_qos_add_request(&hba->pm_qos_req, PM_QOS_DEFAULT_VALUE);
>>         if (cpu_latency_qos_request_active(&hba->pm_qos_req))
>>           hba->pm_qos_enabled = true;
>> +
>> +    mutex_unlock(&hba->pm_qos_mutex);
>>   }
>>     /**
>> @@ -1063,11 +1067,15 @@ void ufshcd_pm_qos_init(struct ufs_hba *hba)
>>    */
>>   void ufshcd_pm_qos_exit(struct ufs_hba *hba)
>>   {
>> +    mutex_lock(&hba->pm_qos_mutex);
>> +
>>       if (!hba->pm_qos_enabled)
>> +        mutex_unlock(&hba->pm_qos_mutex);
>>           return;
> Same here.

Acked.

>> cpu_latency_qos_remove_request(&hba->pm_qos_req);
>>       hba->pm_qos_enabled = false;
>> +    mutex_unlock(&hba->pm_qos_mutex);
>>   }
>>     /**
>> @@ -1077,10 +1085,14 @@ void ufshcd_pm_qos_exit(struct ufs_hba *hba)
>>    */
>>   static void ufshcd_pm_qos_update(struct ufs_hba *hba, bool on)
>>   {
>> +    mutex_lock(&hba->pm_qos_mutex);
>> +
>>       if (!hba->pm_qos_enabled)
>> +        mutex_unlock(&hba->pm_qos_mutex);
>>           return;
> Same here.

Acked.

>> cpu_latency_qos_update_request(&hba->pm_qos_req, on ? 0 : 
>> PM_QOS_DEFAULT_VALUE);
>> +    mutex_unlock(&hba->pm_qos_mutex);
>>   }
>>     /**
>> @@ -10764,6 +10776,10 @@ int ufshcd_init(struct ufs_hba *hba, void 
>> __iomem *mmio_base, unsigned int irq)
>>       mutex_init(&hba->ee_ctrl_mutex);
>>         mutex_init(&hba->wb_mutex);
>> +
>> +    /* Initialize mutex for PM QoS request synchronization */
>> +    mutex_init(&hba->pm_qos_mutex);
>> +
>>       init_rwsem(&hba->clk_scaling_lock);
>>         ufshcd_init_clk_gating(hba);
>> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
>> index 30ff169878dc..e81f4346f168 100644
>> --- a/include/ufs/ufshcd.h
>> +++ b/include/ufs/ufshcd.h
>> @@ -962,6 +962,7 @@ enum ufshcd_mcq_opr {
>>    * @ufs_rtc_update_work: A work for UFS RTC periodic update
>>    * @pm_qos_req: PM QoS request handle
>>    * @pm_qos_enabled: flag to check if pm qos is enabled
>> + * @pm_qos_mutex: synchronizes PM QoS request and status updates
>>    * @critical_health_count: count of critical health exceptions
>>    * @dev_lvl_exception_count: count of device level exceptions since 
>> last reset
>>    * @dev_lvl_exception_id: vendor specific information about the
>> @@ -1135,6 +1136,7 @@ struct ufs_hba {
>>       struct delayed_work ufs_rtc_update_work;
>>       struct pm_qos_request pm_qos_req;
>>       bool pm_qos_enabled;
>> +    struct mutex pm_qos_mutex;
>>         int critical_health_count;
>>       atomic_t dev_lvl_exception_count;


-- 
Thx and BRs,
Zhongqiu Han