linux-kernel - Re: [PATCH v3 09/20] soc: mediatek: mtk-svs: Move t-calibration-data retrieval to svs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 22 Nov 2023 13:23:54 +0200
From:   Eugen Hristev <eugen.hristev@...labora.com>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>, matthias.bgg@...il.com
Cc:     krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        robh+dt@...nel.org, p.zabel@...gutronix.de,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, kernel@...labora.com,
        wenst@...omium.org
Subject: Re: [PATCH v3 09/20] soc: mediatek: mtk-svs: Move t-calibration-data
 retrieval to svs_probe()

On 11/21/23 14:50, AngeloGioacchino Del Regno wrote:
> The t-calibration-data (SVS-Thermal calibration data) shall exist for
> all SoCs or SVS won't work anyway: move it to the common svs_probe()
> function and remove it from all of the per-SoC efuse_parsing() probe
> callbacks.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
> ---
>   drivers/soc/mediatek/mtk-svs.c | 32 ++++++--------------------------
>   1 file changed, 6 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index ab564d48092b..1042af2aee3f 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -1884,11 +1884,6 @@ static bool svs_mt8195_efuse_parsing(struct svs_platform *svsp)
>   		svsb->vmax += svsb->dvt_fixed;
>   	}
>   
> -	ret = svs_get_efuse_data(svsp, "t-calibration-data",
> -				 &svsp->tefuse, &svsp->tefuse_max);
> -	if (ret)
> -		return false;
> -

Hello Angelo,

if you removed the code using `ret` in this patch, it makes sense to 
also remove the variable here instead of doing it in patch 18.
It will avoid unused variable warnings for this patch.


>   	for (i = 0; i < svsp->tefuse_max; i++)
>   		if (svsp->tefuse[i] != 0)
>   			break;
> @@ -1949,11 +1944,6 @@ static bool svs_mt8192_efuse_parsing(struct svs_platform *svsp)
>   		svsb->vmax += svsb->dvt_fixed;
>   	}
>   
> -	ret = svs_get_efuse_data(svsp, "t-calibration-data",
> -				 &svsp->tefuse, &svsp->tefuse_max);
> -	if (ret)
> -		return false;
> -
>   	for (i = 0; i < svsp->tefuse_max; i++)
>   		if (svsp->tefuse[i] != 0)
>   			break;
> @@ -2009,11 +1999,6 @@ static bool svs_mt8188_efuse_parsing(struct svs_platform *svsp)
>   		svsb->vmax += svsb->dvt_fixed;
>   	}
>   
> -	ret = svs_get_efuse_data(svsp, "t-calibration-data",
> -				 &svsp->tefuse, &svsp->tefuse_max);
> -	if (ret)
> -		return false;
> -
>   	for (i = 0; i < svsp->tefuse_max; i++)
>   		if (svsp->tefuse[i] != 0)
>   			break;
> @@ -2097,11 +2082,6 @@ static bool svs_mt8186_efuse_parsing(struct svs_platform *svsp)
>   		svsb->vmax += svsb->dvt_fixed;
>   	}
>   
> -	ret = svs_get_efuse_data(svsp, "t-calibration-data",
> -				 &svsp->tefuse, &svsp->tefuse_max);
> -	if (ret)
> -		return false;
> -
>   	golden_temp = (svsp->tefuse[0] >> 24) & GENMASK(7, 0);
>   	if (!golden_temp)
>   		golden_temp = 50;
> @@ -2198,11 +2178,6 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp)
>   		}
>   	}
>   
> -	ret = svs_get_efuse_data(svsp, "t-calibration-data",
> -				 &svsp->tefuse, &svsp->tefuse_max);
> -	if (ret)
> -		return false;
> -
>   	/* Thermal efuse parsing */
>   	adc_ge_t = (svsp->tefuse[1] >> 22) & GENMASK(9, 0);
>   	adc_oe_t = (svsp->tefuse[1] >> 12) & GENMASK(9, 0);
> @@ -3040,8 +3015,13 @@ static int svs_probe(struct platform_device *pdev)
>   
>   	ret = svs_get_efuse_data(svsp, "svs-calibration-data",
>   				 &svsp->efuse, &svsp->efuse_max);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Cannot read SVS calibration\n");

With the previous code, if svs-calibration-data could not be read, the 
code would go to svs_probe_free_efuse. In your case, it returns directly.
I believe that svs_get_efuse_data using nvmem_cell_read does not 
allocate the buffer for the efuse , hence no more need to free it ? The 
exit code is checking if it's ERR or NULL, but still, if the buffer was 
not allocated, it doesn't make sense to jump there indeed.
In that case, you are also changing the behavior here , and your commit 
appears to do more than a simple move.

> +
> +	ret = svs_get_efuse_data(svsp, "t-calibration-data",
> +				 &svsp->tefuse, &svsp->tefuse_max);
>   	if (ret) {
> -		ret = -EPERM;
> +		dev_err_probe(&pdev->dev, ret, "Cannot read SVS-Thermal calibration\n");
>   		goto svs_probe_free_efuse;

again in this case the tefuse has not been allocated I assume.

So previous code was a bit excessive in trying to free the efuse/tefuse ?

Eugen

>   	}
>