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: <20220817231434.GA157998@hopium>
Date:   Thu, 18 Aug 2022 01:14:34 +0200
From:   Matej Vasilevski <matej.vasilevski@...nam.cz>
To:     Marc Kleine-Budde <mkl@...gutronix.de>
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

Hello Marc,

I have two questions before I send the next patch version, please
bear with me.

On Wed, Aug 03, 2022 at 10:53:03AM +0200, Marc Kleine-Budde wrote:

[...]

> > > > +	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/

1) I can't use clocks_calc_max_nsecs(), because it isn't exported
symbol (and I get modpost error during linking). Is that simply an
oversight on your end or I'm doing something incorrectly?

I've also listed all the exported symbols from /kernel/time, and nothing
really stood out to me as super useful for this patch. So I would
continue using ctucan_calculate_work_delay().

2) Instead of using clk_prepare_enable() manually in probe, I've added
the prepare_enable and disable_unprepare(ts_clk) calls into pm_runtime
suspend and resume callbacks. And I call clk_get_rate(ts_clk) only after
the pm_runtime_enable() and pm_runtime_get_sync() are called. This
seemed nicer to me, because the core clock prepare/unprepare will go
into the pm_runtime callbacks too.

Is that a correct approach, or should I really use the clk_prepare_enable()
and clk_disable_unprepare() "manually" in ctucan_common_probe()/ctucan_timestamp_stop()?

On my Zynq board I don't see the ctucan_resume() callback executed during probe
(after pm_runtime_enable() and pm_runtime_get_sync() are called in _probe()),
but in theory it seems like the correct approach. Xilinx_can driver does this too.
Other drivers (e.g. flexcan, mpc251xfd, rcar) call clk_get_rate() right after
devm_clk_get() in probe, but maybe the situation there is different, I don't
know too much about clocks and pm_runtime yet.

Thanks and best regards,
Matej