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

[AngeloGioacchino Del Regno]

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@xxxxxxxxxxxxx>
> > ---
> >   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)