RE: [RESEND PATCH v3] scsi: ufshcd: fix possible unclocked register access

[Kiwoong Kim]

Reviewed-by: Kiwoong Kim <kwmad.kim@xxxxxxxxxxx>

> Vendor specific setup_clocks callback may require the clocks managed
> by ufshcd driver to be ON. So if the vendor specific setup_clocks callback
> is called while the required clocks are turned off, it could result into
> unclocked register access.
> 
> To prevent possible unclock register access, this change adds one more
> argument to setup_clocks callback to let it know whether it is called
> pre/post the clock changes by core driver.
> 
> Signed-off-by: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>
> ---
> Changes from v2:
> * Added one more argument to setup_clocks callback, this should address
>   Kiwoong Kim's comments on v2.
> 
> Changes from v1:
> * Don't call ufshcd_vops_setup_clocks() again for clock off
> ---
>  drivers/scsi/ufs/ufs-qcom.c | 10 ++++++----
>  drivers/scsi/ufs/ufshcd.c   | 17 ++++++++---------
>  drivers/scsi/ufs/ufshcd.h   |  8 +++++---
>  3 files changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index 3aedf73..3c4f602 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -1094,10 +1094,12 @@ static void ufs_qcom_set_caps(struct ufs_hba *hba)
>   * ufs_qcom_setup_clocks - enables/disable clocks
>   * @hba: host controller instance
>   * @on: If true, enable clocks else disable them.
> + * @status: PRE_CHANGE or POST_CHANGE notify
>   *
>   * Returns 0 on success, non-zero on failure.
>   */
> -static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on)
> +static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
> +				 enum ufs_notify_change_status status)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>  	int err;
> @@ -1111,7 +1113,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba
*hba,
> bool on)
>  	if (!host)
>  		return 0;
> 
> -	if (on) {
> +	if (on && (status == POST_CHANGE)) {
>  		err = ufs_qcom_phy_enable_iface_clk(host->generic_phy);
>  		if (err)
>  			goto out;
> @@ -1130,7 +1132,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba
*hba,
> bool on)
>  		if (vote == host->bus_vote.min_bw_vote)
>  			ufs_qcom_update_bus_bw_vote(host);
> 
> -	} else {
> +	} else if (!on && (status == PRE_CHANGE)) {
> 
>  		/* M-PHY RMMI interface clocks can be turned off */
>  		ufs_qcom_phy_disable_iface_clk(host->generic_phy);
> @@ -1254,7 +1256,7 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>  	ufs_qcom_set_caps(hba);
>  	ufs_qcom_advertise_quirks(hba);
> 
> -	ufs_qcom_setup_clocks(hba, true);
> +	ufs_qcom_setup_clocks(hba, true, POST_CHANGE);
> 
>  	if (hba->dev->id < MAX_UFS_QCOM_HOSTS)
>  		ufs_qcom_hosts[hba->dev->id] = host;
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 05c7456..571a2f6 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5389,6 +5389,10 @@ static int __ufshcd_setup_clocks(struct ufs_hba
> *hba, bool on,
>  	if (!head || list_empty(head))
>  		goto out;
> 
> +	ret = ufshcd_vops_setup_clocks(hba, on, PRE_CHANGE);
> +	if (ret)
> +		return ret;
> +
>  	list_for_each_entry(clki, head, list) {
>  		if (!IS_ERR_OR_NULL(clki->clk)) {
>  			if (skip_ref_clk && !strcmp(clki->name, "ref_clk"))
> @@ -5410,7 +5414,10 @@ static int __ufshcd_setup_clocks(struct ufs_hba
> *hba, bool on,
>  		}
>  	}
> 
> -	ret = ufshcd_vops_setup_clocks(hba, on);
> +	ret = ufshcd_vops_setup_clocks(hba, on, POST_CHANGE);
> +	if (ret)
> +		return ret;
> +
>  out:
>  	if (ret) {
>  		list_for_each_entry(clki, head, list) {
> @@ -5500,8 +5507,6 @@ static void ufshcd_variant_hba_exit(struct ufs_hba
> *hba)
>  	if (!hba->vops)
>  		return;
> 
> -	ufshcd_vops_setup_clocks(hba, false);
> -
>  	ufshcd_vops_setup_regulators(hba, false);
> 
>  	ufshcd_vops_exit(hba);
> @@ -5905,10 +5910,6 @@ disable_clks:
>  	if (ret)
>  		goto set_link_active;
> 
> -	ret = ufshcd_vops_setup_clocks(hba, false);
> -	if (ret)
> -		goto vops_resume;
> -
>  	if (!ufshcd_is_link_active(hba))
>  		ufshcd_setup_clocks(hba, false);
>  	else
> @@ -5925,8 +5926,6 @@ disable_clks:
>  	ufshcd_hba_vreg_set_lpm(hba);
>  	goto out;
> 
> -vops_resume:
> -	ufshcd_vops_resume(hba, pm_op);
>  set_link_active:
>  	ufshcd_vreg_set_hpm(hba);
>  	if (ufshcd_is_link_hibern8(hba) && !ufshcd_uic_hibern8_exit(hba))
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 430bef1..afff7f4 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -273,7 +273,8 @@ struct ufs_hba_variant_ops {
>  	u32	(*get_ufs_hci_version)(struct ufs_hba *);
>  	int	(*clk_scale_notify)(struct ufs_hba *, bool,
>  				    enum ufs_notify_change_status);
> -	int	(*setup_clocks)(struct ufs_hba *, bool);
> +	int	(*setup_clocks)(struct ufs_hba *, bool,
> +				enum ufs_notify_change_status);
>  	int     (*setup_regulators)(struct ufs_hba *, bool);
>  	int	(*hce_enable_notify)(struct ufs_hba *,
>  				     enum ufs_notify_change_status);
> @@ -755,10 +756,11 @@ static inline int
> ufshcd_vops_clk_scale_notify(struct ufs_hba *hba,
>  	return 0;
>  }
> 
> -static inline int ufshcd_vops_setup_clocks(struct ufs_hba *hba, bool on)
> +static inline int ufshcd_vops_setup_clocks(struct ufs_hba *hba, bool on,
> +					enum ufs_notify_change_status
status)
>  {
>  	if (hba->vops && hba->vops->setup_clocks)
> -		return hba->vops->setup_clocks(hba, on);
> +		return hba->vops->setup_clocks(hba, on, status);
>  	return 0;
>  }
> 
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html