I've received a report from kernel test report [1] that a variable was used
unitialized in the mtk8195 hdmi phy code.
I've upon fixing that issue found out that the clock rate calculation
was erroneous since the calculus was moved to div_u64.
I'm providing those two fixes on top of 45810d486bb44 from the linux-phy
repository [2].
[1] https://lore.kernel.org/oe-kbuild-all/[email protected]/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/phy/linux-phy.git
Signed-off-by: Guillaume Ranquet <[email protected]>
---
Guillaume Ranquet (2):
phy: mediatek: hdmi: mt8195: fix uninitialized variable usage in pll_calc
phy: mediatek: hdmi: mt8195: fix wrong pll calculus
drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
---
base-commit: 45810d486bb44bd60213d5f09a713df81b987972
change-id: 20230413-fixes-for-mt8195-hdmi-phy-9e1513b5baad
Best regards,
--
Guillaume Ranquet <[email protected]>
The ret variable in mtk_hdmi_pll_calc() was used unitialized as reported
by the kernel test robot.
Fix the issue by removing the variable altogether and testing out the
return value of mtk_hdmi_pll_set_hw()
Fixes: 45810d486bb44 ("phy: mediatek: add support for phy-mtk-hdmi-mt8195")
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Guillaume Ranquet <[email protected]>
---
drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
index abfc077fb0a8..e10da6c4147e 100644
--- a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
+++ b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
@@ -213,7 +213,7 @@ static int mtk_hdmi_pll_calc(struct mtk_hdmi_phy *hdmi_phy, struct clk_hw *hw,
u64 tmds_clk, pixel_clk, da_hdmitx21_ref_ck, ns_hdmipll_ck, pcw;
u8 txpredivs[4] = { 2, 4, 6, 12 };
u32 fbkdiv_low;
- int i, ret;
+ int i;
pixel_clk = rate;
tmds_clk = pixel_clk;
@@ -292,10 +292,9 @@ static int mtk_hdmi_pll_calc(struct mtk_hdmi_phy *hdmi_phy, struct clk_hw *hw,
if (!(digital_div <= 32 && digital_div >= 1))
return -EINVAL;
- mtk_hdmi_pll_set_hw(hw, PLL_PREDIV, fbkdiv_high, fbkdiv_low,
+ if (mtk_hdmi_pll_set_hw(hw, PLL_PREDIV, fbkdiv_high, fbkdiv_low,
PLL_FBKDIV_HS3, posdiv1, posdiv2, txprediv,
- txposdiv, digital_div);
- if (ret)
+ txposdiv, digital_div))
return -EINVAL;
return 0;
--
2.39.2
The clock rate calculus in mtk_hdmi_pll_calc() was wrong when it has
been replaced by 'div_u64'.
Fix the issue by multiplying the values in the denominator instead of
dividing them.
Fixes: 45810d486bb44 ("phy: mediatek: add support for phy-mtk-hdmi-mt8195")
Signed-off-by: Guillaume Ranquet <[email protected]>
---
drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
index e10da6c4147e..5e84b294a43e 100644
--- a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
+++ b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
@@ -271,7 +271,7 @@ static int mtk_hdmi_pll_calc(struct mtk_hdmi_phy *hdmi_phy, struct clk_hw *hw,
* [32,24] 9bit integer, [23,0]:24bit fraction
*/
pcw = div_u64(((u64)ns_hdmipll_ck) << PCW_DECIMAL_WIDTH,
- da_hdmitx21_ref_ck / PLL_FBKDIV_HS3);
+ da_hdmitx21_ref_ck * PLL_FBKDIV_HS3);
if (pcw > GENMASK_ULL(32, 0))
return -EINVAL;
@@ -288,7 +288,7 @@ static int mtk_hdmi_pll_calc(struct mtk_hdmi_phy *hdmi_phy, struct clk_hw *hw,
posdiv2 = 1;
/* Digital clk divider, max /32 */
- digital_div = div_u64((u64)ns_hdmipll_ck, posdiv1 / posdiv2 / pixel_clk);
+ digital_div = div_u64(ns_hdmipll_ck, posdiv1 * posdiv2 * pixel_clk);
if (!(digital_div <= 32 && digital_div >= 1))
return -EINVAL;
--
2.39.2
Il 13/04/23 14:46, Guillaume Ranquet ha scritto:
> The ret variable in mtk_hdmi_pll_calc() was used unitialized as reported
> by the kernel test robot.
>
> Fix the issue by removing the variable altogether and testing out the
> return value of mtk_hdmi_pll_set_hw()
>
> Fixes: 45810d486bb44 ("phy: mediatek: add support for phy-mtk-hdmi-mt8195")
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Guillaume Ranquet <[email protected]>
> ---
> drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
> index abfc077fb0a8..e10da6c4147e 100644
> --- a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
> +++ b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
> @@ -213,7 +213,7 @@ static int mtk_hdmi_pll_calc(struct mtk_hdmi_phy *hdmi_phy, struct clk_hw *hw,
> u64 tmds_clk, pixel_clk, da_hdmitx21_ref_ck, ns_hdmipll_ck, pcw;
> u8 txpredivs[4] = { 2, 4, 6, 12 };
> u32 fbkdiv_low;
> - int i, ret;
> + int i;
>
> pixel_clk = rate;
> tmds_clk = pixel_clk;
> @@ -292,10 +292,9 @@ static int mtk_hdmi_pll_calc(struct mtk_hdmi_phy *hdmi_phy, struct clk_hw *hw,
> if (!(digital_div <= 32 && digital_div >= 1))
> return -EINVAL;
>
> - mtk_hdmi_pll_set_hw(hw, PLL_PREDIV, fbkdiv_high, fbkdiv_low,
> + if (mtk_hdmi_pll_set_hw(hw, PLL_PREDIV, fbkdiv_high, fbkdiv_low,
> PLL_FBKDIV_HS3, posdiv1, posdiv2, txprediv,
> - txposdiv, digital_div);
> - if (ret)
> + txposdiv, digital_div))
> return -EINVAL;
>
I don't get why we're returning -EINVAL unconditionally in the first place, here.
Function mtk_hdmi_pll_set_hw() should return zero or a negative error number: in
that case, the previous *intention* was fine, so this should be
ret = mtk_hdmi_pll_set_hw(....)
if (ret)
return ret;
return 0;
Regards,
Angelo
Il 13/04/23 14:46, Guillaume Ranquet ha scritto:
> The clock rate calculus in mtk_hdmi_pll_calc() was wrong when it has
> been replaced by 'div_u64'.
>
> Fix the issue by multiplying the values in the denominator instead of
> dividing them.
>
> Fixes: 45810d486bb44 ("phy: mediatek: add support for phy-mtk-hdmi-mt8195")
> Signed-off-by: Guillaume Ranquet <[email protected]>
> ---
> drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
> index e10da6c4147e..5e84b294a43e 100644
> --- a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
> +++ b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
> @@ -271,7 +271,7 @@ static int mtk_hdmi_pll_calc(struct mtk_hdmi_phy *hdmi_phy, struct clk_hw *hw,
> * [32,24] 9bit integer, [23,0]:24bit fraction
> */
> pcw = div_u64(((u64)ns_hdmipll_ck) << PCW_DECIMAL_WIDTH,
> - da_hdmitx21_ref_ck / PLL_FBKDIV_HS3);
> + da_hdmitx21_ref_ck * PLL_FBKDIV_HS3);
How did that even work?!?!?!? Because ... it worked, I did test it. Bah!
Luck was on your side :-P
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
On Fri, 14 Apr 2023 12:31, AngeloGioacchino Del Regno
<[email protected]> wrote:
>Il 13/04/23 14:46, Guillaume Ranquet ha scritto:
>> The ret variable in mtk_hdmi_pll_calc() was used unitialized as reported
>> by the kernel test robot.
>>
>> Fix the issue by removing the variable altogether and testing out the
>> return value of mtk_hdmi_pll_set_hw()
>>
>> Fixes: 45810d486bb44 ("phy: mediatek: add support for phy-mtk-hdmi-mt8195")
>> Reported-by: kernel test robot <[email protected]>
>> Signed-off-by: Guillaume Ranquet <[email protected]>
>> ---
>> drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
>> index abfc077fb0a8..e10da6c4147e 100644
>> --- a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
>> +++ b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
>> @@ -213,7 +213,7 @@ static int mtk_hdmi_pll_calc(struct mtk_hdmi_phy *hdmi_phy, struct clk_hw *hw,
>> u64 tmds_clk, pixel_clk, da_hdmitx21_ref_ck, ns_hdmipll_ck, pcw;
>> u8 txpredivs[4] = { 2, 4, 6, 12 };
>> u32 fbkdiv_low;
>> - int i, ret;
>> + int i;
>>
>> pixel_clk = rate;
>> tmds_clk = pixel_clk;
>> @@ -292,10 +292,9 @@ static int mtk_hdmi_pll_calc(struct mtk_hdmi_phy *hdmi_phy, struct clk_hw *hw,
>> if (!(digital_div <= 32 && digital_div >= 1))
>> return -EINVAL;
>>
>> - mtk_hdmi_pll_set_hw(hw, PLL_PREDIV, fbkdiv_high, fbkdiv_low,
>> + if (mtk_hdmi_pll_set_hw(hw, PLL_PREDIV, fbkdiv_high, fbkdiv_low,
>> PLL_FBKDIV_HS3, posdiv1, posdiv2, txprediv,
>> - txposdiv, digital_div);
>> - if (ret)
>> + txposdiv, digital_div))
>> return -EINVAL;
>>
>
>I don't get why we're returning -EINVAL unconditionally in the first place, here.
>
>Function mtk_hdmi_pll_set_hw() should return zero or a negative error number: in
>that case, the previous *intention* was fine, so this should be
>
Hi Angelo,
I was maybe a bit too quick on fixing this that way.
Anyway it doesn't change a thing as mtk_hdmi_pll_set_hw() eitheir
returns 0 or -EINVAL.
But I agree that the logic is dubious and propagating the return value
is the right thing
to do.
I see that Arnd and Tom posted versions that you might prefer:
https://lore.kernel.org/linux-phy/[email protected]/
https://lore.kernel.org/linux-phy/[email protected]/
Thx,
Guillaume.
> ret = mtk_hdmi_pll_set_hw(....)
> if (ret)
> return ret;
>
> return 0;
>
>
>Regards,
>Angelo
On 13-04-23, 14:46, Guillaume Ranquet wrote:
> I've received a report from kernel test report [1] that a variable was used
> unitialized in the mtk8195 hdmi phy code.
>
> I've upon fixing that issue found out that the clock rate calculation
> was erroneous since the calculus was moved to div_u64.
Applied both, thanks
--
~Vinod