Re: [PATCH v1] ufs: core: fix deadlock when rtc update

[Bean Huo <huobean@xxxxxxxxx>]

On Fri, 2024-07-12 at 10:34 -0700, Bart Van Assche wrote:
> > -        /* Update RTC only when there are no requests in progress
> > and UFSHCI is operational */
> > -       if (!ufshcd_is_ufs_dev_busy(hba) && hba->ufshcd_state ==
> > UFSHCD_STATE_OPERATIONAL)
> > +        /*
> > +         * Update RTC only when
> > +         * 1. there are no requests in progress
> > +         * 2. UFSHCI is operational
> > +         * 3. pm operation is not in progress
> > +         */
> > +       if (!ufshcd_is_ufs_dev_busy(hba) &&
> > +           hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL &&
> > +           !hba->pm_op_in_progress)
> >                 ufshcd_update_rtc(hba);
> >    
> >         if (ufshcd_is_ufs_dev_active(hba) && hba-
> > >dev_info.rtc_update_period)
> 
> The above seems racy to me. I don't think there is any mechanism that
> prevents that hba->pm_op_in_progress is set after it has been checked
> and before ufshcd_update_rtc() is called. Has it been considered to
> add
> an ufshcd_rpm_get_sync_nowait() call before the hba-
> >pm_op_in_progress
> check and a ufshcd_rpm_put_sync() call after the ufshcd_update_rtc()
> call?
> 
> Thanks,
> 
> Bart.

Bart,

do you want this:

diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-
priv.h
index ce36154ce963..2b74d6329b9d 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -311,6 +311,25 @@ static inline int ufshcd_update_ee_usr_mask(struct
ufs_hba *hba,
                                        &hba->ee_drv_mask, set, clr);
 }
 
+static inline int ufshcd_rpm_get_sync_nowait(struct ufs_hba *hba)
+{
+       int ret = 0;
+       struct device *dev = &hba->ufs_device_wlun->sdev_gendev;
+
+       pm_runtime_get_noresume(dev);
+
+       /* Check if the device is already active */
+       if (pm_runtime_active(dev))
+               return 0;
+
+       /* Attempt to resume the device without blocking */
+       ret = pm_request_resume(dev);
+       if (ret < 0)
+               pm_runtime_put_noidle(dev);
+
+       return ret;
+}
+
 static inline int ufshcd_rpm_get_sync(struct ufs_hba *hba)
 {
        return pm_runtime_get_sync(&hba->ufs_device_wlun->sdev_gendev);
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index bea00e069e9a..1b7fc4ce9e5c 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8209,10 +8209,8 @@ static void ufshcd_update_rtc(struct ufs_hba
*hba)
         */
        val = ts64.tv_sec - hba->dev_info.rtc_time_baseline;
 
-       ufshcd_rpm_get_sync(hba);
        err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
QUERY_ATTR_IDN_SECONDS_PASSED,
                                0, 0, &val);
-       ufshcd_rpm_put_sync(hba);
 
        if (err)
                dev_err(hba->dev, "%s: Failed to update rtc %d\n",
__func__, err);
@@ -8226,10 +8224,14 @@ static void ufshcd_rtc_work(struct work_struct
*work)
 
        hba = container_of(to_delayed_work(work), struct ufs_hba,
ufs_rtc_update_work);
 
+       if (ufshcd_rpm_get_sync_nowait(hba))
+               goto out;
+
         /* Update RTC only when there are no requests in progress and
UFSHCI is operational */
        if (!ufshcd_is_ufs_dev_busy(hba) && hba->ufshcd_state ==
UFSHCD_STATE_OPERATIONAL)
                ufshcd_update_rtc(hba);
-
+       ufshcd_rpm_put_sync(hba);
+out:
        if (ufshcd_is_ufs_dev_active(hba) && hba-
>dev_info.rtc_update_period)
                schedule_delayed_work(&hba->ufs_rtc_update_work,
                                      msecs_to_jiffies(hba-
>dev_info.rtc_update_period));
(END)




or can we change cancel_delayed_work_sync(&hba->ufs_rtc_update_work);
to cancel_delayed_work(&hba->ufs_rtc_update_work);  ??



kind regards,
Bean