netdev - Re: [PATCH net-next] net: mvpp2: Improve data types and use min()

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: <ZpVDVHXh4FTZnmUv@shell.armlinux.org.uk>
Date: Mon, 15 Jul 2024 16:44:01 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Thorsten Blum <thorsten.blum@...lux.com>
Cc: marcin.s.wojtas@...il.com, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: mvpp2: Improve data types and use min()

On Thu, Jul 11, 2024 at 05:47:43PM +0200, Thorsten Blum wrote:
> Change the data type of the variable freq in mvpp2_rx_time_coal_set()
> and mvpp2_tx_time_coal_set() to u32 because port->priv->tclk also has
> the data type u32.
> 
> Change the data type of the function parameter clk_hz in
> mvpp2_usec_to_cycles() and mvpp2_cycles_to_usec() to u32 accordingly
> and remove the following Coccinelle/coccicheck warning reported by
> do_div.cocci:
> 
>   WARNING: do_div() does a 64-by-32 division, please consider using div64_ul instead
> 
> Use min() to simplify the code and improve its readability.
> 
> Compile-tested only.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@...lux.com>

I'm still on holiday, but it's a wet day today. Don't expect replies
from me to be regular.

I don't think this is a good idea.

priv->tclk comes from clk_get_rate() which returns an unsigned long.
tclk should _also_ be an unsigned long, not a u32, so that the range
of values clk_get_rate() returns can be represented without being
truncted.

Thus the use of unsigned long elsewhere where tclk is passed into is
actually correct.

If we need to limit tclk to values that u32 can represent, then that
needs to be done here:

                priv->tclk = clk_get_rate(priv->pp_clk);

by assigning the return value to an unsigned long local variable,
then checking its upper liit before assigning it to priv->tclk.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!