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

["Peter Wang (王信友)" <peter.wang@mediatek.com>]

On Tue, 2025-09-02 at 15:48 +0800, Zhongqiu Han wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> 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.


Hi Zhongqiu,

Introducing an additional mutex lock would impact the efficiency of
devfreq.
Wouldn’t it be better to simply adjust the sequence to avoid race
conditions?
For instance,
ufshcd_pm_qos_exit(hba);
ufshcd_exit_clk_scaling(hba);
could be changed to
ufshcd_exit_clk_scaling(hba);
ufshcd_pm_qos_exit(hba);
This ensures that clock scaling is stopped before pm_qos is removed.

Thanks.
Peter