linux-kernel - Re: [PATCH] phy: freescale: fsl-samsung-hdmi: fix build error in fsl_samsung_hdmi_phy_configure_pll_lock

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <Z3aDU_dhfgEHlL4u@atmark-techno.com>
Date: Thu, 2 Jan 2025 21:15:15 +0900
From: Dominique Martinet <dominique.martinet@...ark-techno.com>
To: Adam Ford <aford173@...il.com>
Cc: Pei Xiao <xiaopei01@...inos.cn>, geert@...ux-m68k.org,
	aford@...conembedded.com, arnd@...db.de,
	frieder.schrempf@...tron.de, kishon@...nel.org,
	l.stach@...gutronix.de, linux-kernel@...r.kernel.org,
	linux-phy@...ts.infradead.org, m.felsch@...gutronix.de,
	sandor.yu@....com, u.kleine-koenig@...libre.com, vkoul@...nel.org
Subject: Re: [PATCH] phy: freescale: fsl-samsung-hdmi: fix build error in
 fsl_samsung_hdmi_phy_configure_pll_lock_det

Adam Ford wrote on Mon, Dec 30, 2024 at 08:11:16PM -0600:
> > index 5eac70a1e858..3e4d1a5160ea 100644
> > --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > @@ -341,7 +341,7 @@ fsl_samsung_hdmi_phy_configure_pll_lock_det(struct fsl_samsung_hdmi_phy *phy,
> >                         break;
> >         }
> >
> > -       writeb(FIELD_PREP(REG12_CK_DIV_MASK, div), phy->regs + PHY_REG(12));
> > +       writeb(FIELD_PREP(REG12_CK_DIV_MASK, div == 4 ? div - 1 : div), phy->regs + PHY_REG(12));
> 
> The for-loop above this line states:   for (div = 0; div < 4; div++)
> How could this ever reach 4?  If it did reach 4, the calculation for
> int_pllclk would need to be recalculated since int_pllclk = pclk / (1
> << div);

But... for (div = 0; div < 4; div++) does reach 4, if the break
condition didn't match, which is something the compiler cannot ensure
here.

The old code would just fall out of any of the switch cases and fallback
to div = 1 if pixclk > 297000000, which is likely incorrect, so in that
sense just padding this through `& 3` and pretending it will never
happen is probably acceptable, but this ought to have a better comment
than what Pei just sent.
(this was correct with the old lookup tables, I'm not sure if we can't
compute any higher frequencies now?)

My preference would be to actually check and handle this somehow since I
don't think this part of the code is that performance critical that we
can't afford an extra instruction, e.g. something like that:
if (WARN_ONCE(div == 4, "pixclk %u out of range", pclk))
	(appropriate fallback or return?)

but I haven't spent the time to actually check so will leave that up to
you.

Thank you both,
-- 
Dominique