On Thu, Jan 05, 2023 at 10:38:58AM -0800, Bart Van Assche wrote:
> On 12/22/22 02:21, Johan Hovold wrote:
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index bda61be5f035..5c3821b2fcf8 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -1234,12 +1234,14 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
> > * clock scaling is in progress
> > */
> > ufshcd_scsi_block_requests(hba);
> > + mutex_lock(&hba->wb_mutex);
> > down_write(&hba->clk_scaling_lock);
> >
> > if (!hba->clk_scaling.is_allowed ||
> > ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
> > ret = -EBUSY;
> > up_write(&hba->clk_scaling_lock);
> > + mutex_unlock(&hba->wb_mutex);
> > ufshcd_scsi_unblock_requests(hba);
> > goto out;
> > }
> > @@ -1251,12 +1253,16 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
> > return ret;
> > }
>
> Please add an __acquires(&hba->wb_mutex) annotation for sparse.
>
> > -static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
> > +static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool scale_up)
> > {
> > - if (writelock)
> > - up_write(&hba->clk_scaling_lock);
> > - else
> > - up_read(&hba->clk_scaling_lock);
> > + up_write(&hba->clk_scaling_lock);
> > +
> > + /* Enable Write Booster if we have scaled up else disable it */
> > + if (ufshcd_enable_wb_if_scaling_up(hba))
> > + ufshcd_wb_toggle(hba, scale_up);
> > +
> > + mutex_unlock(&hba->wb_mutex);
> > +
> > ufshcd_scsi_unblock_requests(hba);
> > ufshcd_release(hba);
> > }
>
> Please add a __releases(&hba->wb_mutex) annotation for sparse.
This would actually introduce new sparse warnings as mutex_lock/unlock()
are not sparse annotated:
drivers/ufs/core/ufshcd.c:1254:9: warning: context imbalance in 'ufshcd_clock_scaling_prepare' - wrong count at exit
drivers/ufs/core/ufshcd.c:1266:9: warning: context imbalance in 'ufshcd_clock_scaling_unprepare' - wrong count at exit
drivers/ufs/core/ufshcd.c:1281:12: warning: context imbalance in 'ufshcd_devfreq_scale' - wrong count at exit
I guess it's not worth adding explicit __acquire()/__release() to these
helpers either (e.g. as they are only used in one function).
> > @@ -1273,7 +1279,6 @@ static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
> > static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
> > {
> > int ret = 0;
> > - bool is_writelock = true;
> >
> > ret = ufshcd_clock_scaling_prepare(hba);
> > if (ret)
> > @@ -1302,15 +1307,8 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
> > }
> > }
> >
> > - /* Enable Write Booster if we have scaled up else disable it */
> > - if (ufshcd_enable_wb_if_scaling_up(hba)) {
> > - downgrade_write(&hba->clk_scaling_lock);
> > - is_writelock = false;
> > - ufshcd_wb_toggle(hba, scale_up);
> > - }
> > -
> > out_unprepare:
> > - ufshcd_clock_scaling_unprepare(hba, is_writelock);
> > + ufshcd_clock_scaling_unprepare(hba, scale_up);
> > return ret;
> > }
>
> This patch moves the ufshcd_wb_toggle() from before the out_unprepare
> label to after the out_unprepare label (into
> ufshcd_clock_scaling_unprepare()). Does this change perhaps introduce a
> new call to ufshcd_wb_toggle() in error paths?
Thanks for spotting that. I'll leave the setting unchanged on errors in
v2.
Johan
