netdev - Re: [PATCH v5 2/4] can: ctucanfd: add HW timestamps to RX and error CAN frames

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] [day] [month] [year] [list]

Message-ID: <20221026071541.ydvvtreum242he6w@pengutronix.de>
Date:   Wed, 26 Oct 2022 09:15:41 +0200
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Matej Vasilevski <matej.vasilevski@...nam.cz>
Cc:     Pavel Pisa <pisa@....felk.cvut.cz>,
        Ondrej Ille <ondrej.ille@...il.com>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-can@...r.kernel.org, netdev@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v5 2/4] can: ctucanfd: add HW timestamps to RX and error
 CAN frames

On 26.10.2022 00:22:37, Matej Vasilevski wrote:
> On Mon, Oct 24, 2022 at 10:02:38PM +0200, Marc Kleine-Budde wrote:
> > On 12.10.2022 08:25:56, Matej Vasilevski wrote:
> > > This patch adds support for retrieving hardware timestamps to RX and
> > 
> > Later in the code you set struct ethtool_ts_info::tx_types but the
> > driver doesn't set TX timestamps, does it?
> 
> No, it doesn't explicitly. Unless something changed and I don't know about it,
> all the drivers using can_put_echo_skb() (includes ctucanfd) now report
> software (hardware if available) tx timestamps thanks to Vincent's patch.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?id=12a18d79dc14c80b358dbd26461614b97f2ea4a6

Yes, since that patch, drivers using can_put_echo_skb() support software
TX timestamps. But you have to set the HW timestamp on the TX'ed CAN
frame prior to the can_rx_offload_get_echo_skb() call for HW TX
timestamps, e.g.:

| https://elixir.bootlin.com/linux/v6.0.3/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-tef.c#L112

[...]

> > > +	/* Setup conversion constants and work delay */
> > > +	if (priv->timestamp_possible) {
> > > +		u64 max_cycles;
> > > +		u64 work_delay_ns;
> > > +		u32 maxsec;
> > > +
> > > +		priv->cc.read = ctucan_read_timestamp_cc_wrapper;
> > > +		priv->cc.mask = CYCLECOUNTER_MASK(timestamp_bit_size);
> > > +		maxsec = min_t(u32, CTUCANFD_MAX_WORK_DELAY_SEC,
> > > +			       div_u64(priv->cc.mask, timestamp_clk_rate));
> > > +		clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift,
> > > +				       timestamp_clk_rate, NSEC_PER_SEC, maxsec);
> > > +
> > > +		/* shortened copy of clocks_calc_max_nsecs() */
> > > +		max_cycles = div_u64(ULLONG_MAX, priv->cc.mult);
> > > +		max_cycles = min(max_cycles, priv->cc.mask);
> > > +		work_delay_ns = clocksource_cyc2ns(max_cycles, priv->cc.mult,
> > > +						   priv->cc.shift) >> 2;
> > 
> > I think we can use cyclecounter_cyc2ns() for this, see:
> > 
> > | https://elixir.bootlin.com/linux/v6.0.3/source/drivers/net/ethernet/ti/cpts.c#L642
> > 
> > BTW: This is the only networking driver using clocks_calc_mult_shift()
> > (so far) :D
> > 
> 
> I don't really see the benefit at the moment (I have to include
> clocksource.h anyway due to the clocks_calc_mult_shift()), but sure,
> I'll use cyclecounter_cyc2ns().
> 
> Fun fact :-D I might look into the cpts.c

The benefit is less variance in the kernel tree, use the same pattern to
calculate the delay if both register width and frequency are unknown at
compile time.

[...]

> >Regarding the timestamp_clk handling:
> >
> >If you prepare_enable the timestamp_clk during probe_common() and don't
> >disable_unprepare it, it stays on the whole lifetime of the driver. So
> >there's no need/reason for the runtime suspend/resume functions.
> >
> >So either keep the clock powered and remove the suspend/resume functions
> >or shut down the clock after probe.
> >
> >If you want to make things 1000% clean, you can get the timestamp's
> >clock rate during open() and re-calculate the mult and shift. The
> >background is that the clock rate might change if the clock is not
> >enabled (at least that's not guaranteed by the common clock framework).
> >Actual HW implementations might differ.
> 
> Hmm, I thought that pm_runtime_put() will eventually run runtime suspend
> callback, but now I see that it will run only the idle callback (which
> I haven't defined).
> I'll remove the runtime suspend/resume callbacks.

If your clock stays enabled the whole driver lifetime you can use
devm_clk_get_enabled(), devm will take care of the disable & unprepare.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)