diff mbox series

[v5,3/8] scsi: ufs: ufs-mediatek: Remove useless mediatek,ufs-boost-crypt property

Message ID 20240612074309.50278-4-angelogioacchino.delregno@collabora.com
State New
Headers show
Series [v5,1/8] scsi: ufs: ufs-mediatek: Remove useless mediatek,ufs-support-va09 property | expand

Commit Message

AngeloGioacchino Del Regno June 12, 2024, 7:43 a.m. UTC
There is no need to have a property that activates the inline crypto
boost feature, as this needs many things: a regulator, three clocks,
and the mediatek,boost-crypt-microvolt property to be set.

If any one of these is missing, the feature won't be activated,
hence, it is useless to have yet one more property to enable that.

While at it, also address another two issues:
1. Give back the return value to the caller and make sure to fail
   probing if we get an -EPROBE_DEFER or -ENOMEM; and
2. Free the ufs_mtk_crypt_cfg structure allocated in the crypto
   boost function if said functionality could not be enabled because
   it's not supported, as that'd be only wasted memory.

Last but not least, move the devm_kzalloc() call for ufs_mtk_crypt_cfg
to after getting the dvfsrc-vcore regulator and the boost microvolt
property, as if those fail there's no reason to even allocate that.

Fixes: ac8c2459091c ("scsi: ufs-mediatek: Decouple features from platform bindings")
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/ufs/host/ufs-mediatek.c | 55 ++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 25 deletions(-)

Comments

Peter Wang (王信友) June 14, 2024, 6:34 a.m. UTC | #1
On Wed, 2024-06-12 at 09:43 +0200, AngeloGioacchino Del Regno wrote:
> There is no need to have a property that activates the inline crypto
> boost feature, as this needs many things: a regulator, three clocks,
> and the mediatek,boost-crypt-microvolt property to be set.
> 
> If any one of these is missing, the feature won't be activated,
> hence, it is useless to have yet one more property to enable that.
> 
> While at it, also address another two issues:
> 1. Give back the return value to the caller and make sure to fail
>    probing if we get an -EPROBE_DEFER or -ENOMEM; and
> 2. Free the ufs_mtk_crypt_cfg structure allocated in the crypto
>    boost function if said functionality could not be enabled because
>    it's not supported, as that'd be only wasted memory.
> 
> Last but not least, move the devm_kzalloc() call for
> ufs_mtk_crypt_cfg
> to after getting the dvfsrc-vcore regulator and the boost microvolt
> property, as if those fail there's no reason to even allocate that.
> 
> Fixes: ac8c2459091c ("scsi: ufs-mediatek: Decouple features from
> platform bindings")
> Signed-off-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> ---
>  drivers/ufs/host/ufs-mediatek.c | 55 ++++++++++++++++++-------------
> --
>  1 file changed, 30 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-
> mediatek.c
> index 23271eb1a244..8d0e7ea52541 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
> @@ -576,51 +576,55 @@ static int ufs_mtk_init_host_clk(struct ufs_hba
> *hba, const char *name,
>  	return ret;
>  }
>  
> -static void ufs_mtk_init_boost_crypt(struct ufs_hba *hba)
> +static int ufs_mtk_init_boost_crypt(struct ufs_hba *hba)
>  {
>  	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
>  	struct ufs_mtk_crypt_cfg *cfg;
>  	struct device *dev = hba->dev;
>  	struct regulator *reg;
>  	u32 volt;
> -
> -	host->crypt = devm_kzalloc(dev, sizeof(*(host->crypt)),
> -				   GFP_KERNEL);
> -	if (!host->crypt)
> -		goto disable_caps;
> +	int ret;
>  
>  	reg = devm_regulator_get_optional(dev, "dvfsrc-vcore");
>  	if (IS_ERR(reg)) {
> -		dev_info(dev, "failed to get dvfsrc-vcore: %ld",
> -			 PTR_ERR(reg));
> -		goto disable_caps;
> +		ret = PTR_ERR(reg);
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +
> +		return 0;
>  	}
>  
> -	if (of_property_read_u32(dev->of_node, "mediatek,boost-crypt-
> microvolt",
> -				 &volt)) {
> +	ret = of_property_read_u32(dev->of_node, "mediatek,boost-crypt-
> microvolt", &volt);
> +	if (ret) {
>  		dev_info(dev, "failed to get mediatek,boost-crypt-
> microvolt");
> -		goto disable_caps;
> +		return 0;
>  	}
>  
> +	host->crypt = devm_kzalloc(dev, sizeof(*host->crypt),
> GFP_KERNEL);
> +	if (!host->crypt)
> +		return -ENOMEM;
> +
>  	cfg = host->crypt;
> -	if (ufs_mtk_init_host_clk(hba, "crypt_mux",
> -				  &cfg->clk_crypt_mux))
> -		goto disable_caps;
> +	ret = ufs_mtk_init_host_clk(hba, "crypt_mux", &cfg-
> >clk_crypt_mux);
> +	if (ret)
> +		goto out;
>  
> -	if (ufs_mtk_init_host_clk(hba, "crypt_lp",
> -				  &cfg->clk_crypt_lp))
> -		goto disable_caps;
> +	ret = ufs_mtk_init_host_clk(hba, "crypt_lp", &cfg-
> >clk_crypt_lp);
> +	if (ret)
> +		goto out;
>  
> -	if (ufs_mtk_init_host_clk(hba, "crypt_perf",
> -				  &cfg->clk_crypt_perf))
> -		goto disable_caps;
> +	ret = ufs_mtk_init_host_clk(hba, "crypt_perf", &cfg-
> >clk_crypt_perf);
> +	if (ret)
> +		goto out;
>  
>  	cfg->reg_vcore = reg;
>  	cfg->vcore_volt = volt;
>  	host->caps |= UFS_MTK_CAP_BOOST_CRYPT_ENGINE;
>  
> -disable_caps:
> -	return;
> +out:
> +	if (ret)
> +		devm_kfree(dev, host->crypt);
> +	return 0;
>  }
>  
>  static int ufs_mtk_init_va09_pwr_ctrl(struct ufs_hba *hba)
> @@ -649,8 +653,9 @@ static int ufs_mtk_init_host_caps(struct ufs_hba
> *hba)
>  	struct device_node *np = hba->dev->of_node;
>  	int ret;
>  
> -	if (of_property_read_bool(np, "mediatek,ufs-boost-crypt"))
> -		ufs_mtk_init_boost_crypt(hba);
> 

Hi AngeloGioacchino,

As previously explained, removing these DTS settings will make what was
originally a simple task 
more complicated. In addition, it will require MediaTek to put in extra
effort to migrate the kernel. 
We do not believe that such changes have any benefits.

Thanks.
Peter




> +	ret = ufs_mtk_init_boost_crypt(hba);
> +	if (ret)
> +		return ret;
>  
>  	ret = ufs_mtk_init_va09_pwr_ctrl(hba);
>  	if (ret)
diff mbox series

Patch

diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index 23271eb1a244..8d0e7ea52541 100644
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -576,51 +576,55 @@  static int ufs_mtk_init_host_clk(struct ufs_hba *hba, const char *name,
 	return ret;
 }
 
-static void ufs_mtk_init_boost_crypt(struct ufs_hba *hba)
+static int ufs_mtk_init_boost_crypt(struct ufs_hba *hba)
 {
 	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
 	struct ufs_mtk_crypt_cfg *cfg;
 	struct device *dev = hba->dev;
 	struct regulator *reg;
 	u32 volt;
-
-	host->crypt = devm_kzalloc(dev, sizeof(*(host->crypt)),
-				   GFP_KERNEL);
-	if (!host->crypt)
-		goto disable_caps;
+	int ret;
 
 	reg = devm_regulator_get_optional(dev, "dvfsrc-vcore");
 	if (IS_ERR(reg)) {
-		dev_info(dev, "failed to get dvfsrc-vcore: %ld",
-			 PTR_ERR(reg));
-		goto disable_caps;
+		ret = PTR_ERR(reg);
+		if (ret == -EPROBE_DEFER)
+			return ret;
+
+		return 0;
 	}
 
-	if (of_property_read_u32(dev->of_node, "mediatek,boost-crypt-microvolt",
-				 &volt)) {
+	ret = of_property_read_u32(dev->of_node, "mediatek,boost-crypt-microvolt", &volt);
+	if (ret) {
 		dev_info(dev, "failed to get mediatek,boost-crypt-microvolt");
-		goto disable_caps;
+		return 0;
 	}
 
+	host->crypt = devm_kzalloc(dev, sizeof(*host->crypt), GFP_KERNEL);
+	if (!host->crypt)
+		return -ENOMEM;
+
 	cfg = host->crypt;
-	if (ufs_mtk_init_host_clk(hba, "crypt_mux",
-				  &cfg->clk_crypt_mux))
-		goto disable_caps;
+	ret = ufs_mtk_init_host_clk(hba, "crypt_mux", &cfg->clk_crypt_mux);
+	if (ret)
+		goto out;
 
-	if (ufs_mtk_init_host_clk(hba, "crypt_lp",
-				  &cfg->clk_crypt_lp))
-		goto disable_caps;
+	ret = ufs_mtk_init_host_clk(hba, "crypt_lp", &cfg->clk_crypt_lp);
+	if (ret)
+		goto out;
 
-	if (ufs_mtk_init_host_clk(hba, "crypt_perf",
-				  &cfg->clk_crypt_perf))
-		goto disable_caps;
+	ret = ufs_mtk_init_host_clk(hba, "crypt_perf", &cfg->clk_crypt_perf);
+	if (ret)
+		goto out;
 
 	cfg->reg_vcore = reg;
 	cfg->vcore_volt = volt;
 	host->caps |= UFS_MTK_CAP_BOOST_CRYPT_ENGINE;
 
-disable_caps:
-	return;
+out:
+	if (ret)
+		devm_kfree(dev, host->crypt);
+	return 0;
 }
 
 static int ufs_mtk_init_va09_pwr_ctrl(struct ufs_hba *hba)
@@ -649,8 +653,9 @@  static int ufs_mtk_init_host_caps(struct ufs_hba *hba)
 	struct device_node *np = hba->dev->of_node;
 	int ret;
 
-	if (of_property_read_bool(np, "mediatek,ufs-boost-crypt"))
-		ufs_mtk_init_boost_crypt(hba);
+	ret = ufs_mtk_init_boost_crypt(hba);
+	if (ret)
+		return ret;
 
 	ret = ufs_mtk_init_va09_pwr_ctrl(hba);
 	if (ret)