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

Bean Huo
2024-07-14T22:37:49Z

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