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

Date:   Tue, 25 Oct 2022 11:25:20 +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 12.10.2022 08:25:56, Matej Vasilevski wrote:
> This patch adds support for retrieving hardware timestamps to RX and
> error CAN frames. It uses timecounter and cyclecounter structures,
> because the timestamping counter width depends on the IP core integration
> (it might not always be 64-bit).
> For platform devices, you should specify "ts" clock in device tree.
> For PCI devices, the timestamping frequency is assumed to be the same
> as bus frequency.
> 
> Signed-off-by: Matej Vasilevski <matej.vasilevski@...nam.cz>

[...]

>  int ctucan_suspend(struct device *dev)
> @@ -1337,12 +1456,41 @@ int ctucan_resume(struct device *dev)
>  }
>  EXPORT_SYMBOL(ctucan_resume);
>  
> +int ctucan_runtime_suspend(struct device *dev)
> +{
> +	struct net_device *ndev = dev_get_drvdata(dev);
> +	struct ctucan_priv *priv = netdev_priv(ndev);
> +
> +	clk_disable_unprepare(priv->timestamp_clk);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ctucan_runtime_suspend);
> +
> +int ctucan_runtime_resume(struct device *dev)
> +{
> +	struct net_device *ndev = dev_get_drvdata(dev);
> +	struct ctucan_priv *priv = netdev_priv(ndev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(priv->timestamp_clk);
> +	if (ret) {
> +		dev_err(dev, "Cannot enable timestamping clock: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ctucan_runtime_resume);

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.

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)