Re: [PATCH 1/2] scsi: ufs: Clean up ufshcd_scale_clks() and clock scaling error out path

[Can Guo]

On 2020-03-18 16:19, Avri Altman wrote:
> Hi,
>
> > From: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>
> >
> > This change introduces a func ufshcd_set_clk_freq() to explicitly
> > set clock frequency so that it can be used in reset_and_resotre path 
> > and
> > in ufshcd_scale_clks(). Meanwhile, this change cleans up the clock 
> > scaling
> > error out path.
> >
> > Signed-off-by: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>
> > Signed-off-by: Can Guo <cang@xxxxxxxxxxxxxx>
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 2a2a63b..63aaa88f 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -855,28 +855,29 @@ static bool
> > ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
> >                 return false;
> >  }
> >
> > -static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
> > +/**
> > + * ufshcd_set_clk_freq - set UFS controller clock frequencies
> > + * @hba: per adapter instance
> > + * @scale_up: If True, set max possible frequency othewise set low
> > frequency
> > + *
> > + * Returns 0 if successful
> > + * Returns < 0 for any other errors
> > + */
> > +static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
> Personally I prefer using the convention of "__scale_clks" to describe
> the privet core of scale_clks,
> But I know that many people are against it.
>
> >  {
> >         int ret = 0;
> >         struct ufs_clk_info *clki;
> >         struct list_head *head = &hba->clk_list_head;
> > -       ktime_t start = ktime_get();
> > -       bool clk_state_changed = false;
> >
> >         if (list_empty(head))
> >                 goto out;
> >
> > -       ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
> > -       if (ret)
> > -               return ret;
> > -
> >         list_for_each_entry(clki, head, list) {
> >                 if (!IS_ERR_OR_NULL(clki->clk)) {
> >                         if (scale_up && clki->max_freq) {
> >                                 if (clki->curr_freq == clki->max_freq)
> >                                         continue;
> >
> > -                               clk_state_changed = true;
> >                                 ret = clk_set_rate(clki->clk, 
> > clki->max_freq);
> >                                 if (ret) {
> >                                         dev_err(hba->dev, "%s: %s clk 
> > set rate(%dHz) failed,
> > %d\n",
> > @@ -895,7 +896,6 @@ static int ufshcd_scale_clks(struct ufs_hba *hba,
> > bool scale_up)
> >                                 if (clki->curr_freq == clki->min_freq)
> >                                         continue;
> >
> > -                               clk_state_changed = true;
> >                                 ret = clk_set_rate(clki->clk, 
> > clki->min_freq);
> >                                 if (ret) {
> >                                         dev_err(hba->dev, "%s: %s clk 
> > set rate(%dHz) failed,
> > %d\n",
> > @@ -914,13 +914,36 @@ static int ufshcd_scale_clks(struct ufs_hba 
> > *hba,
> > bool scale_up)
> >                                 clki->name, clk_get_rate(clki->clk));
> >         }
> >
> > +out:
> > +       return ret;
> > +}
> > +
> > +/**
> > + * ufshcd_scale_clks - scale up or scale down UFS controller clocks
> > + * @hba: per adapter instance
> > + * @scale_up: True if scaling up and false if scaling down
> > + *
> > + * Returns 0 if successful
> > + * Returns < 0 for any other errors
> > + */
> > +static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
> > +{
> > +       int ret = 0;
> > +
> > +       ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = ufshcd_set_clk_freq(hba, scale_up);
> > +       if (ret)
> > +               return ret;
> > +
> >         ret = ufshcd_vops_clk_scale_notify(hba, scale_up, 
> > POST_CHANGE);
> > +       if (ret) {
> > +               ufshcd_set_clk_freq(hba, !scale_up);
> > +               return ret;
> > +       }
> >
> > -out:
> > -       if (clk_state_changed)
> > -               trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
> > -                       (scale_up ? "up" : "down"),
> > -                       ktime_to_us(ktime_sub(ktime_get(), start)), 
> > ret);
>
> Why remove the ufshcd_profile_clk_scaling trace?

Shall add it back, this is just a collection of Subhash's changes.

> >         return ret;
> >  }
> >
> > @@ -1106,35 +1129,36 @@ static int ufshcd_devfreq_scale(struct ufs_hba
> > *hba, bool scale_up)
> >
> >         ret = ufshcd_clock_scaling_prepare(hba);
> >         if (ret)
> > -               return ret;
> > +               goto out;
> Are you fixing here a hold without release?
> Should make note of that.

Yes, true

> >         /* scale down the gear before scaling down clocks */
> >         if (!scale_up) {
> >                 ret = ufshcd_scale_gear(hba, false);
> >                 if (ret)
> > -                       goto out;
> > +                       goto clk_scaling_unprepare;
> >         }
> >
> >         ret = ufshcd_scale_clks(hba, scale_up);
> > -       if (ret) {
> > -               if (!scale_up)
> > -                       ufshcd_scale_gear(hba, true);
> > -               goto out;
> > -       }
> > +       if (ret)
> > +               goto scale_up_gear;
> >
> >         /* scale up the gear after scaling up clocks */
> >         if (scale_up) {
> >                 ret = ufshcd_scale_gear(hba, true);
> >                 if (ret) {
> >                         ufshcd_scale_clks(hba, false);
> > -                       goto out;
> > +                       goto clk_scaling_unprepare;
> >                 }
> >         }
> >
> > -       ret = ufshcd_vops_clk_scale_notify(hba, scale_up, 
> > POST_CHANGE);
> > +       goto clk_scaling_unprepare;
> I think you should find a way to make this function more readable.
> Adding all those "spaghetti" gotos making it even harder to read.
>
> Thanks,
> Avri

This is just a collection of Subhash's changes. I think the 2
clk_scaling_unprepare and out gotos make sense, but the
scale_up_gear goto is a bit vague. I will remove the
scale_up_gear goto.

Thanks,

Can Guo.

> > -out:
> > +scale_up_gear:
> > +       if (!scale_up)
> > +               ufshcd_scale_gear(hba, true);
> > +clk_scaling_unprepare:
> >         ufshcd_clock_scaling_unprepare(hba);
> > +out:
> >         ufshcd_release(hba);
> >         return ret;
> >  }
> > @@ -6251,7 +6275,7 @@ static int ufshcd_host_reset_and_restore(struct
> > ufs_hba *hba)
> >         spin_unlock_irqrestore(hba->host->host_lock, flags);
> >
> >         /* scale up clocks to max frequency before full 
> > reinitialization */
> > -       ufshcd_scale_clks(hba, true);
> > +       ufshcd_set_clk_freq(hba, true);
> >
> >         err = ufshcd_hba_enable(hba);
> >         if (err)
> > --
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> > Linux Foundation Collaborative Project.