From: thomas.petazzoni@free-electrons•com (Thomas Petazzoni)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCHv2 net-next 02/11] net: mvpp2: handle too large value in mvpp2_rx_time_coal_set()
Date: Thu, 2 Feb 2017 17:11:21 +0100 [thread overview]
Message-ID: <20170202171121.4540faf3@free-electrons.com> (raw)
In-Reply-To: <20170106125923.GA14217@n2100.armlinux.org.uk>
Russell,
On Fri, 6 Jan 2017 12:59:24 +0000, Russell King - ARM Linux wrote:
> Beware of rounding and overflow errors. usec and val are u32's.
[...]
Thanks for all the suggestions, I've taken this into account in my v3
that I've sent a few minutes ago? I've re-used almost exactly your
implementation, with a few changes (see below).
> static u32 usec_to_cycles(u32 usec, unsigned long clock_rate_hz)
> {
> u64 tmp = clock_rate_hz * usec;
I had to cast one of the variables to u64 here otherwise the
multiplication was done on 32 bits, and then the result was expanded to
64 bits.
> do_div(tmp, USEC_PER_SEC);
>
> return tmp > 0xffffffff ? 0xffffffff : tmp;
I've used U32_MAX here.
> static u32 cycles_to_usec(u32 cycles, unsigned long clock_rate_hz)
> {
> u64 tmp = cycles * USEC_PER_SEC;
>
> do_div(tmp, clock_rate_hz);
>
> return tmp > 0xffffffff ? 0xffffffff : tmp;
Same comments for this function.
> and:
> u32 val = usec_to_cycles(usec, port->priv->tclk);
>
> if (val > MVPP2_MAX_ISR_RX_THRESHOLD) {
> usec = cycles_to_usec(MVPP2_MAX_ISR_RX_THRESHOLD,
> port->priv->tclk);
>
> /* re-evaluate to get actual register value for usec */
> val = usec_to_cycles(usec, port->priv->tclk);
> }
>
> > mvpp2_write(port->priv, MVPP2_ISR_RX_THRESHOLD_REG(rxq->id), val);
> >
> > rxq->time_coal = usec;
>
> This function appears to be called from two places:
>
> mvpp2_rxq_init():
> mvpp2_rx_time_coal_set(port, rxq, rxq->time_coal);
>
> mvpp2_ethtool_set_coalesce():
> rxq->time_coal = c->rx_coalesce_usecs;
> mvpp2_rx_time_coal_set(port, rxq, rxq->time_coal);
>
> It seems rather pointless to pass in rxq->time_coal when you're also
> passing in rxq.
Indeed, so I've added another patch in the series that does this.
Thanks for the review!
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
next prev parent reply other threads:[~2017-02-02 16:11 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-28 16:45 [PATCHv2 net-next 00/11] net: mvpp2: misc improvements and preparation patches Thomas Petazzoni
2016-12-28 16:45 ` [PATCHv2 net-next 01/11] net: mvpp2: handle too large value handling in mvpp2_rx_pkts_coal_set() Thomas Petazzoni
2016-12-28 16:45 ` [PATCHv2 net-next 02/11] net: mvpp2: handle too large value in mvpp2_rx_time_coal_set() Thomas Petazzoni
2017-01-06 12:59 ` Russell King - ARM Linux
2017-02-02 16:11 ` Thomas Petazzoni [this message]
2016-12-28 16:45 ` [PATCHv2 net-next 03/11] net: mvpp2: release reference to txq_cpu[] entry after unmapping Thomas Petazzoni
2016-12-28 16:46 ` [PATCHv2 net-next 04/11] net: mvpp2: remove unused 'tx_skb' field of 'struct mvpp2_tx_queue' Thomas Petazzoni
2016-12-28 16:46 ` [PATCHv2 net-next 05/11] net: mvpp2: drop useless fields in mvpp2_bm_pool and related code Thomas Petazzoni
2016-12-28 16:46 ` [PATCHv2 net-next 06/11] net: mvpp2: simplify mvpp2_bm_bufs_add() Thomas Petazzoni
2016-12-28 16:46 ` [PATCHv2 net-next 07/11] net: mvpp2: remove unused register definitions Thomas Petazzoni
2016-12-28 16:46 ` [PATCHv2 net-next 08/11] net: mvpp2: fix indentation of MVPP2_EXT_GLOBAL_CTRL_DEFAULT Thomas Petazzoni
2016-12-28 16:46 ` [PATCHv2 net-next 09/11] net: mvpp2: simplify MVPP2_PRS_RI_* definitions Thomas Petazzoni
2017-01-06 13:07 ` Russell King - ARM Linux
2017-02-02 16:11 ` Thomas Petazzoni
2016-12-28 16:46 ` [PATCHv2 net-next 10/11] net: mvpp2: switch to build_skb() in the RX path Thomas Petazzoni
2016-12-28 16:46 ` [PATCHv2 net-next 11/11] net: mvpp2: enable building on 64-bit platforms Thomas Petazzoni
2016-12-29 17:03 ` [PATCHv2 net-next 00/11] net: mvpp2: misc improvements and preparation patches David Miller
2017-02-02 16:12 ` Thomas Petazzoni
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170202171121.4540faf3@free-electrons.com \
--to=thomas.petazzoni@free-electrons$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox