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

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

On 9/13/2025 12:22 AM, Bart Van Assche wrote:
> On 9/2/25 12:48 AM, Zhongqiu Han wrote:
>> -    return sysfs_emit(buf, "%d\n", hba->pm_qos_enabled);
>> +    return sysfs_emit(buf, "%d\n", READ_ONCE(hba->pm_qos_enabled));
> 
> Using READ_ONCE() here is inconsistent since none of the modifications
> of hba->pm_qos_enabled use WRITE_ONCE(). Protecting hba->pm_qos_enabled
> modifications with a mutex is not sufficient since the above read of
> hba->pm_qos_enabled is not protected by the same mutex.
> 
> Has it been considered to leave out the READ_ONCE() from the above code
> and instead to add the following above the sysfs_emit() call?
> 
> guard(mutex)(&hba->pm_qos_mutex);
> 

Hi Bart,
Thanks for your review~

Yes, in this case pm_qos_enabled is a status bit indicating whether PM
QoS is enabled, rather than a constantly changing statistical value
(such as IRQ count) that users can tolerate. Users may rely on it to
determine whether PM QoS is enabled, and thereby assess whether current
performance meets expectations or decide to enable/disable PM QoS.
Protection is indeed necessary here.

I will use guard(mutex)(&hba->pm_qos_mutex);
in the V3 to make the modification.

>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index 926650412eaa..98b9ce583386 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -1047,14 +1047,19 @@ 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)
>> +    if (hba->pm_qos_enabled) {
>> +        mutex_unlock(&hba->pm_qos_mutex);
>>           return;
>> +    }
>>       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);
>>   }
> 
> Please make the above code easier to review by using
> guard(mutex)(&hba->pm_qos_mutex) instead of explicit mutex_lock() and
> mutex_unlock() calls.

Sure, I’ll make the change.

> 
>> @@ -1063,11 +1068,16 @@ void ufshcd_pm_qos_init(struct ufs_hba *hba)
>>    */
>>   void ufshcd_pm_qos_exit(struct ufs_hba *hba)
>>   {
>> -    if (!hba->pm_qos_enabled)
>> +    mutex_lock(&hba->pm_qos_mutex);
>> +
>> +    if (!hba->pm_qos_enabled) {
>> +        mutex_unlock(&hba->pm_qos_mutex);
>>           return;
>> +    }
>>       cpu_latency_qos_remove_request(&hba->pm_qos_req);
>>       hba->pm_qos_enabled = false;
>> +    mutex_unlock(&hba->pm_qos_mutex);
>>   }
> 
> Same comment here: please make the above code easier to review by using
> guard(mutex)(&hba->pm_qos_mutex) instead of explicit mutex_lock() and
> mutex_unlock() calls.

Acked, I’ll make the change.


> 
>> @@ -1077,10 +1087,15 @@ void ufshcd_pm_qos_exit(struct ufs_hba *hba)
>>    */
>>   static void ufshcd_pm_qos_update(struct ufs_hba *hba, bool on)
>>   {
>> -    if (!hba->pm_qos_enabled)
>> +    mutex_lock(&hba->pm_qos_mutex);
>> +
>> +    if (!hba->pm_qos_enabled) {
>> +        mutex_unlock(&hba->pm_qos_mutex);
>>           return;
>> +    }
>>       cpu_latency_qos_update_request(&hba->pm_qos_req, on ? 0 : 
>> PM_QOS_DEFAULT_VALUE);
>> +    mutex_unlock(&hba->pm_qos_mutex);
>>   }
> 
> Also in the above code, please use the guard()() macro instead of
> explicit mutex_lock() and mutex_unlock() calls.

Acked, will make the change as well.

> 
> Thanks,
> 
> Bart.


-- 
Thx and BRs,
Zhongqiu Han