| Message ID | 20230413-fixes-for-mt8195-hdmi-phy-v1-1-b8482458df0d@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix mtk-hdmi-mt8195 unitialized variable usage and clock rate calculation | expand |
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 <lkp@intel.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- > 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
On Fri, 14 Apr 2023 12:31, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> 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 <lkp@intel.com> >> Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> >> --- >> 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/20230414075842.4006164-1-arnd@kernel.org/ https://lore.kernel.org/linux-phy/20230414122253.3171524-1-trix@redhat.com/ Thx, Guillaume. > ret = mtk_hdmi_pll_set_hw(....) > if (ret) > return ret; > > return 0; > > >Regards, >Angelo
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;
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 <lkp@intel.com> Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> --- drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)