Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
IronPort-SDR: 6qaYWzJ/ptvYmYJ56VjZ1b4iEGPSyDmexPm+OVEbQToxmv9EtGWitjl9QBviquv++7/THRnwTU
 6XsCRNWTt9BiiC7zNHnpqgVuxfBJGY8KUfrC52EUKD5QvruM8Nf1njB4LNv+ptSKFEqv0S5gvu
 HY63rAkx7WwmoQTEjZN6Pp36fc/n1c+Z3cHmgp8OvMNl8gPbpumMIyql6bnXkFf0TL+8/AqDC6
 FgT2Q5CnH0/KFg08p3m9nincFhAP4L4uP3g14j+H4uxL7Ufx5eiBNcIyr1Giqeo4qJUW4LW8V3
 OrY=
From:   Avri Altman <Avri.Altman@wdc.com>
To:     Can Guo <cang@codeaurora.org>,
        "asutoshd@codeaurora.org" <asutoshd@codeaurora.org>,
        "nguyenb@codeaurora.org" <nguyenb@codeaurora.org>,
        "hongwus@codeaurora.org" <hongwus@codeaurora.org>,
        "rnayak@codeaurora.org" <rnayak@codeaurora.org>,
        "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "kernel-team@android.com" <kernel-team@android.com>,
        "saravanak@google.com" <saravanak@google.com>,
        "salyzyn@google.com" <salyzyn@google.com>
CC:     Subhash Jadavani <subhashj@codeaurora.org>,
        Alim Akhtar <alim.akhtar@samsung.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Stanley Chu <stanley.chu@mediatek.com>,
        Bean Huo <beanhuo@micron.com>,
        Bart Van Assche <bvanassche@acm.org>,
        Venkat Gopalakrishnan <venkatg@codeaurora.org>,
        Tomas Winkler <tomas.winkler@intel.com>,
        open list <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 1/2] scsi: ufs: Clean up ufshcd_scale_clks() and clock
 scaling error out path
Thread-Topic: [PATCH 1/2] scsi: ufs: Clean up ufshcd_scale_clks() and clock
 scaling error out path
Thread-Index: AQHV+2FxCSs4Pin03USk3FSY0RKVv6hOAA7w
Date:   Wed, 18 Mar 2020 08:19:51 +0000
Message-ID: <SN6PR04MB4640E8EF26802A44B1F5D38AFCF70@SN6PR04MB4640.namprd04.prod.outlook.com>
References: <1584342373-10282-1-git-send-email-cang@codeaurora.org>
 <1584342373-10282-2-git-send-email-cang@codeaurora.org>
In-Reply-To: <1584342373-10282-2-git-send-email-cang@codeaurora.org>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: b5a4f43e-93f0-4df1-2259-08d7cb1521e0
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Mar 2020 08:19:51.7784
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: SPwjba8dGm5JF/ZSV8IqoT66T09Mrc/xK9K6TXKQcrW1o3jRhW1c2ph409s/v72lqzyRssJgdcENkoImjYNUXg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR04MB4350
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi,

>=20
> From: Subhash Jadavani <subhashj@codeaurora.org>
>=20
> 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 scalin=
g
> error out path.
>=20
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
>=20
> 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;
>  }
>=20
> -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 =3D 0;
>         struct ufs_clk_info *clki;
>         struct list_head *head =3D &hba->clk_list_head;
> -       ktime_t start =3D ktime_get();
> -       bool clk_state_changed =3D false;
>=20
>         if (list_empty(head))
>                 goto out;
>=20
> -       ret =3D 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 =3D=3D clki->max_freq=
)
>                                         continue;
>=20
> -                               clk_state_changed =3D true;
>                                 ret =3D 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 =3D=3D clki->min_freq=
)
>                                         continue;
>=20
> -                               clk_state_changed =3D true;
>                                 ret =3D 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));
>         }
>=20
> +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 =3D 0;
> +
> +       ret =3D ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
> +       if (ret)
> +               return ret;
> +
> +       ret =3D ufshcd_set_clk_freq(hba, scale_up);
> +       if (ret)
> +               return ret;
> +
>         ret =3D ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE);
> +       if (ret) {
> +               ufshcd_set_clk_freq(hba, !scale_up);
> +               return ret;
> +       }
>=20
> -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?


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

>=20
>         /* scale down the gear before scaling down clocks */
>         if (!scale_up) {
>                 ret =3D ufshcd_scale_gear(hba, false);
>                 if (ret)
> -                       goto out;
> +                       goto clk_scaling_unprepare;
>         }
>=20
>         ret =3D ufshcd_scale_clks(hba, scale_up);
> -       if (ret) {
> -               if (!scale_up)
> -                       ufshcd_scale_gear(hba, true);
> -               goto out;
> -       }
> +       if (ret)
> +               goto scale_up_gear;
>=20
>         /* scale up the gear after scaling up clocks */
>         if (scale_up) {
>                 ret =3D ufshcd_scale_gear(hba, true);
>                 if (ret) {
>                         ufshcd_scale_clks(hba, false);
> -                       goto out;
> +                       goto clk_scaling_unprepare;
>                 }
>         }
>=20
> -       ret =3D 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.=20

Thanks,
Avri

>=20
> -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);
>=20
>         /* scale up clocks to max frequency before full reinitialization =
*/
> -       ufshcd_scale_clks(hba, true);
> +       ufshcd_set_clk_freq(hba, true);
>=20
>         err =3D ufshcd_hba_enable(hba);
>         if (err)
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> Linux Foundation Collaborative Project.