linux-kernel - Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate instead LUT

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: <Z861gsaGY6bGSisf@atmark-techno.com>
Date: Mon, 10 Mar 2025 18:48:50 +0900
From: Dominique Martinet <dominique.martinet@...ark-techno.com>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
Cc: Vinod Koul <vkoul@...nel.org>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Adam Ford <aford173@...il.com>,
	Frieder Schrempf <frieder.schrempf@...tron.de>,
	Marco Felsch <m.felsch@...gutronix.de>,
	Lucas Stach <l.stach@...gutronix.de>, linux-phy@...ts.infradead.org,
	linux-kernel@...r.kernel.org,
	Makoto Sato <makoto.sato@...ark-techno.com>
Subject: Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate
 instead LUT

Uwe Kleine-König wrote on Mon, Mar 10, 2025 at 10:14:23AM +0100:
> > for 83.5mHz, the integer divider generates 83.2mHz (-0.36%), but the
> > next LUT value (82.5mHz) is 1.2% off which incorrectly rejects modes
> > requiring this frequency.
> 
> Is the unit here MHz or mHz? I suspect the former?

Err, yes MHz; I was still half asleep when I added that example to the
commit message..

> Without having looked in detail, I think it would be nice to reduce code
> duplication between phy_clk_round_rate() and phy_clk_set_rate(). The
> former has
> 
> 	if (rate > 297000000 || rate < 22250000)
> 		return -EINVAL;
> 
> which seems to be lacking from the latter so I suspect there are more
> differences between the two functions than fixed here?

For this particular rate check, I believe that if phy_clk_round_rate()
rejected the frequency then phy_clk_set_rate() cannot be called at all
with the said value (at least on this particular setup going through the
imx8mp-hdmi-tx bridge validating frequencies first before allowing
modes), not that it'd hurt to recheck.

In general though I agree these are doing pretty much the same thing,
with clk_round_rate() throwing away the pms values and there's more
duplication than ideal...
Unfortunately the code that computes registers for the integer divider
does it in a global variable so just computing everything in
round_rate() would forget what last setting was actually applied and
break e.g. resume, but yes that's just refactoring that should be done.

While we're here I also have no idea what recalc_rate() is supposed to
do but that 'return 74250000;' is definitely odd, and I'm sure there are
other improvements that could be made at the edges.

That's quite rude of me given I just sent the patch, but we probably
won't have time to rework this until mid-april after some urgent work
ends (this has actually been waiting for testing for 3 months
already...)
If this doesn't bother anyone else we can wait for a v2 then, but
otherwise it might be worth considering getting as is until refactoring
happens (and I pinky promise I'll find time before this summer -- I can
send a v2 with commit message fixed up as Frieder suggested if this is
acceptable to you)

Thanks for the quick feedback either way, and sorry for the long delay.
-- 
Dominique