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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 16/04/24 09:03, Peter Wang (王信友) ha scritto:
> On Mon, 2024-04-15 at 13:00 +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 688d85909ad6..47f16e6720f4 100644
>> --- a/drivers/ufs/host/ufs-mediatek.c
>> +++ b/drivers/ufs/host/ufs-mediatek.c
>> @@ -575,51 +575,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;
>> +
>>
> 
> Hi Angelo,
> 
> If retrun -ENOMEN, host will init fail.
> But previous is skip boost crypt feature only.
> It change the driver behavior.
> 

This is fully intentional: if a platform supports boost-crypt, this means that the
feature *MUST* be enabled, and must *not* be disabled if a memory allocation fails,
as that is relative to available pages at boot, and not to SoC feature support.

Keep in mind that the allocation was moved to *after* checking if such platform
does indeed support the boost-crypt feature, and it is critical to FAIL probing
if there was no memory to allocate the host->crypt structure.

> 
> 
> 
>>   	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)
>> @@ -648,8 +652,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;
>>   
> 
> Most ufs-mediatek platform dosen't need "mediatek,ufs-boost-crypt"
> Remove this property will casue most platform try error and add init
> latency.
> 

Yes this causes -> less than half of a millisecond <- of additional boot time
if the dvfsrc-supply is present but boost-microvolt is not.

I really don't see the problem with that :-)

Regards,
Angelo

> Thanks.
> Peter
> 
> 
> 
>>   	ret = ufs_mtk_init_va09_pwr_ctrl(hba);
>>   	if (ret)