| 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
| ||
|
Message-ID: <deff5728-4571-492d-bfb7-2666d0fa2557@collabora.com>
Date: Wed, 22 Nov 2023 14:51:31 +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/22/23 14:41, AngeloGioacchino Del Regno wrote:
> Il 22/11/23 12:23, Eugen Hristev ha scritto:
>> 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.
>>
>>
>
> Yes, though the comment is not for this function, but rather for 8183.
> Anyway, that
> makes sense, but if it's the only change of this v3, it's something that
> I can fix
> while applying instead of sending another 20 patches round. Thanks.
>
>>> 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.
>>
>
> I'm not changing the behavior: the previous behavior was to fail and
> free the efuse
> variable if previously allocated, the current behavior is to fail and
> free the
> efuse variable if previously allocated, and the tefuse variable if
> previously
> allocated, which is a result of the actual move of the retrieval of the
> thermal
> fuse calibration data.
>
> I really don't see anything implicit here.
>
Previous behavior was
ret = svs_get_efuse_data (efuse)
if (ret) goto svs_probe_free_efuse
Now, you have it as
ret = svs_get_efuse_data (efuse)
if (ret) return dev_err_probe...
>>> +
>>> + 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 ?
>
> The previous code was performing an useless error check on something
> that was not
> supposed to be allocated *yet*. Yes, it was wrong before.
>
> Cheers,
> Angelo
> _______________________________________________
> Kernel mailing list -- kernel@...lman.collabora.com
> To unsubscribe send an email to kernel-leave@...lman.collabora.com
Powered by blists - more mailing lists