| Message ID | 20240711154741.174745-1-thorsten.blum@toblux.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f7023b3d697c6a7dfe2d9c70e0d8c2c580ccbd76 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: mvpp2: Improve data types and use min() | expand |
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@toblux.com> Reviewed-by: Simon Horman <horms@kernel.org>
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Thu, 11 Jul 2024 17:47:43 +0200 you 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: > > [...] Here is the summary with links: - [net-next] net: mvpp2: Improve data types and use min() https://git.kernel.org/netdev/net-next/c/f7023b3d697c You are awesome, thank you!
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@toblux.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.
On Mon, 15 Jul 2024 16:44:01 +0100 Russell King (Oracle) wrote:
> I don't think this is a good idea.
Reverted.
On Mon, Jul 15, 2024 at 04:44:01PM +0100, Russell King (Oracle) wrote: > 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@toblux.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. Sorry, I thought that I had checked the types. But clearly I wasn't nearly careful enough.
On 15. Jul 2024, at 17:44, Russell King (Oracle) <linux@armlinux.org.uk> wrote: > 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@toblux.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. I don't think tclk should be an unsigned long. In [1] Eric Dumazet wrote: "This is silly, clk_hz fits in a u32, why pretends it is 64bit ?" and all functions in mvpp2_main.c (mvpp2_write(), do_div(), device_property_read_u32(), and mvpp22_gop_fca_set_timer()), which have tclk as a direct or indirect argument, assume tclk is a u32. Although mvpp2_cycles_to_usec() suggests it can be called with an unsigned long clk_hz, do_div() then immediately casts it to a u32 anyway. Yes, the function clk_get_rate() returns an unsigned long according to its signature, but tclk is always used as a u32 afterwards. I'm not familiar with the hardware, but I guess the clock rate always fits into 32 bits (just like Eric wrote)? Thanks, Thorsten > 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. [1] https://lore.kernel.org/linux-kernel/cbcdb354-04de-2a9a-1754-c32dd014e859@gmail.com/
From: Thorsten Blum > Sent: 16 July 2024 18:49 > > On 15. Jul 2024, at 17:44, Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > 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@toblux.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. > > I don't think tclk should be an unsigned long. > > In [1] Eric Dumazet wrote: > > "This is silly, clk_hz fits in a u32, why pretends it is 64bit ?" > > and all functions in mvpp2_main.c (mvpp2_write(), do_div(), > device_property_read_u32(), and mvpp22_gop_fca_set_timer()), which have > tclk as a direct or indirect argument, assume tclk is a u32. > > Although mvpp2_cycles_to_usec() suggests it can be called with an > unsigned long clk_hz, do_div() then immediately casts it to a u32 > anyway. > > Yes, the function clk_get_rate() returns an unsigned long according to > its signature, but tclk is always used as a u32 afterwards. > > I'm not familiar with the hardware, but I guess the clock rate always > fits into 32 bits (just like Eric wrote)? 'long' can't be correct - it is 32bit on 32bit systems. They are just as likely to have a clock that is faster than 4GHz than a 64bit system. The type should either be u64 or u32 (or just unsigned int - Linux isn't going to get far if int is 16 bits). This is true of a lot of the uses of 'long'. There are cases where 'long' will generate better code than 'int' on 64bit systems. In particular it can save sign/zero extension (and maybe masking) of function parameters and results. But that is only likely to matter on very hot paths - and particularly for array indexes. (OTOH the masking for char/short is likely to be really horrid on anything except x86.) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index 9adf4301c9b1..1e52256a9ea8 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -2766,29 +2766,29 @@ static void mvpp2_tx_pkts_coal_set(struct mvpp2_port *port, } } -static u32 mvpp2_usec_to_cycles(u32 usec, unsigned long clk_hz) +static u32 mvpp2_usec_to_cycles(u32 usec, u32 clk_hz) { u64 tmp = (u64)clk_hz * usec; do_div(tmp, USEC_PER_SEC); - return tmp > U32_MAX ? U32_MAX : tmp; + return min(tmp, U32_MAX); } -static u32 mvpp2_cycles_to_usec(u32 cycles, unsigned long clk_hz) +static u32 mvpp2_cycles_to_usec(u32 cycles, u32 clk_hz) { u64 tmp = (u64)cycles * USEC_PER_SEC; do_div(tmp, clk_hz); - return tmp > U32_MAX ? U32_MAX : tmp; + return min(tmp, U32_MAX); } /* Set the time delay in usec before Rx interrupt */ static void mvpp2_rx_time_coal_set(struct mvpp2_port *port, struct mvpp2_rx_queue *rxq) { - unsigned long freq = port->priv->tclk; + u32 freq = port->priv->tclk; u32 val = mvpp2_usec_to_cycles(rxq->time_coal, freq); if (val > MVPP2_MAX_ISR_RX_THRESHOLD) { @@ -2804,7 +2804,7 @@ static void mvpp2_rx_time_coal_set(struct mvpp2_port *port, static void mvpp2_tx_time_coal_set(struct mvpp2_port *port) { - unsigned long freq = port->priv->tclk; + u32 freq = port->priv->tclk; u32 val = mvpp2_usec_to_cycles(port->tx_time_coal, freq); if (val > MVPP2_MAX_ISR_TX_THRESHOLD) {
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@toblux.com> --- drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)