netdev - Re: [PATCH v2 1/3] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220803085303.2u4l5l6wmualq33v@pengutronix.de>
Date:   Wed, 3 Aug 2022 10:53:03 +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 v2 1/3] can: ctucanfd: add HW timestamps to RX and error
 CAN frames

On 03.08.2022 02:09:03, Matej Vasilevski wrote:
[...]

> > > @@ -682,9 +708,10 @@ static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *c
> > >  	if (unlikely(len > wc * 4))
> > >  		len = wc * 4;
> > >  
> > > -	/* Timestamp - Read and throw away */
> > > -	ctucan_read32(priv, CTUCANFD_RX_DATA);
> > > -	ctucan_read32(priv, CTUCANFD_RX_DATA);
> > > +	/* Timestamp */
> > > +	tstamp_low = ctucan_read32(priv, CTUCANFD_RX_DATA);
> > > +	tstamp_high = ctucan_read32(priv, CTUCANFD_RX_DATA);
> > > +	*timestamp = concatenate_two_u32(tstamp_high, tstamp_low) & priv->cc.mask;
> > >  
> > >  	/* Data */
> > >  	for (i = 0; i < len; i += 4) {
> > > @@ -713,6 +740,7 @@ static int ctucan_rx(struct net_device *ndev)
> > >  	struct net_device_stats *stats = &ndev->stats;
> > >  	struct canfd_frame *cf;
> > >  	struct sk_buff *skb;
> > > +	u64 timestamp;
> > >  	u32 ffw;
> > >  
> > >  	if (test_bit(CTUCANFD_FLAG_RX_FFW_BUFFERED, &priv->drv_flags)) {
> > > @@ -736,7 +764,9 @@ static int ctucan_rx(struct net_device *ndev)
> > >  		return 0;
> > >  	}
> > >  
> > > -	ctucan_read_rx_frame(priv, cf, ffw);
> > > +	ctucan_read_rx_frame(priv, cf, ffw, &timestamp);
> > > +	if (priv->timestamp_enabled)
> > > +		ctucan_skb_set_timestamp(priv, skb, timestamp);
> > 
> > Can the ctucan_skb_set_timestamp() and ctucan_read_timestamp_counter()
> > happen concurrently? AFAICS they are all called from ctucan_rx_poll(),
> > right?
> 
> Yes, I see no problem when two ctucan_read_timestamp_counter run
> concurrently, same goes for two ctucan_skb_set_timestamp and 
> ctucan_skb_set_timestamp concurrently with
> ctucan_read_timestamp_counter.

Right!

> The _counter() function only reads from the core's registers and returns
> a new timestamp. The _set_timestamp() only writes to the skb, but the
> skb will be allocated new in every _rx_poll() call.
> 
> The only concurrency issue I can remotely see is when the periodic worker
> updates timecounter->cycle_last, right when the value is used in
> timecounter_cyc2time (from _set_timestamp()). But I don't think this is
> worth using some synchronization primitive.

Yes, I'm worried about the cycle_last on 32 bit systems.

[...]

> > > +	priv->cc.read = ctucan_read_timestamp_cc_wrapper;
> > > +	priv->cc.mask = CYCLECOUNTER_MASK(timestamp_bit_size);
> > 
> > Does the driver use these 2 if timestamping is not possible?
> 
> Cc.mask is always used in ctucan_read_rx_frame(), cc.read isn't used
> when timestamps aren't possible. I can move cc.read inside the 'if' for
> maximal efficiency.

Ok

> > > +	if (priv->timestamp_possible) {
> > > +		clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq,
> > > +				       NSEC_PER_SEC, CTUCANFD_MAX_WORK_DELAY_SEC);
> > > +		priv->work_delay_jiffies =
> > > +			ctucan_calculate_work_delay(timestamp_bit_size, timestamp_freq);
> > > +		if (priv->work_delay_jiffies == 0)
> > > +			priv->timestamp_possible = false;
> > 
> > You'll get a higher precision if you take the mask into account, at
> > least if the counter overflows before CTUCANFD_MAX_WORK_DELAY_SEC:
> > 
> >         maxsec = min(CTUCANFD_MAX_WORK_DELAY_SEC, priv->cc.mask / timestamp_freq);
> > 	
> >         clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq, NSEC_PER_SEC,  maxsec);
> >         work_delay_in_ns = clocks_calc_max_nsecs(&priv->cc.mult, &priv->cc.shift, 0, &priv->cc.mask, NULL);
> > 
> > You can use clocks_calc_max_nsecs() to calculate the work delay.
> 
> This is a good point, thanks. I'll incorporate it into the patch.

And do this calculation after a clk_prepare_enable(), see other mail to
Pavel
| https://lore.kernel.org/all/20220803083718.7bh2edmsorwuv4vu@pengutronix.de/

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)