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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20221024200238.tgqkjjyagklglshu@pengutronix.de>
Date:   Mon, 24 Oct 2022 22:02:38 +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

Later in the code you set struct ethtool_ts_info::tx_types but the
driver doesn't set TX timestamps, does it?

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

[...]

> diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/ctucanfd/ctucanfd_base.c
> index b8da15ea6ad9..079819d53e23 100644
> --- a/drivers/net/can/ctucanfd/ctucanfd_base.c
> +++ b/drivers/net/can/ctucanfd/ctucanfd_base.c

[...]

> @@ -950,6 +986,11 @@ static int ctucan_rx_poll(struct napi_struct *napi, int quota)
>  			cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
>  			stats->rx_packets++;
>  			stats->rx_bytes += cf->can_dlc;
> +			if (priv->timestamp_enabled) {
> +				u64 tstamp = ctucan_read_timestamp_counter(priv);
> +
> +				ctucan_skb_set_timestamp(priv, skb, tstamp);
> +			}
>  			netif_rx(skb);
>  		}
>  
> @@ -1230,6 +1271,9 @@ static int ctucan_open(struct net_device *ndev)
>  		goto err_chip_start;
>  	}
>  
> +	if (priv->timestamp_possible)
> +		ctucan_timestamp_init(priv);
> +

This is racy. You have to init the timestamping before the start of the
chip, i.e. enabling the IRQs. I had the same problem with the gs_usb
driver:

| https://lore.kernel.org/all/20220921081329.385509-1-mkl@pengutronix.de

>  	netdev_info(ndev, "ctu_can_fd device registered\n");
>  	napi_enable(&priv->napi);
>  	netif_start_queue(ndev);
> @@ -1262,6 +1306,8 @@ static int ctucan_close(struct net_device *ndev)
>  	ctucan_chip_stop(ndev);
>  	free_irq(ndev->irq, ndev);
>  	close_candev(ndev);
> +	if (priv->timestamp_possible)
> +		ctucan_timestamp_stop(priv);

Can you make this symmetric with respect to the ctucan_open() function.
>  
>  	pm_runtime_put(priv->dev);
>  
> @@ -1294,15 +1340,88 @@ static int ctucan_get_berr_counter(const struct net_device *ndev, struct can_ber
>  	return 0;
>  }

[...]

> @@ -1385,15 +1534,29 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigne
>  
>  	/* Getting the can_clk info */
>  	if (!can_clk_rate) {
> -		priv->can_clk = devm_clk_get(dev, NULL);
> +		priv->can_clk = devm_clk_get_optional(dev, "core");
> +		if (!priv->can_clk)
> +			/* For compatibility with (older) device trees without clock-names */
> +			priv->can_clk = devm_clk_get(dev, NULL);
>  		if (IS_ERR(priv->can_clk)) {
> -			dev_err(dev, "Device clock not found.\n");
> +			dev_err(dev, "Device clock not found: %pe.\n", priv->can_clk);
>  			ret = PTR_ERR(priv->can_clk);
>  			goto err_free;
>  		}
>  		can_clk_rate = clk_get_rate(priv->can_clk);
>  	}
>  
> +	if (!timestamp_clk_rate) {
> +		priv->timestamp_clk = devm_clk_get(dev, "ts");
> +		if (IS_ERR(priv->timestamp_clk)) {
> +			/* Take the core clock instead */
> +			dev_info(dev, "Failed to get ts clk\n");
> +			priv->timestamp_clk = priv->can_clk;
> +		}
> +		clk_prepare_enable(priv->timestamp_clk);
> +		timestamp_clk_rate = clk_get_rate(priv->timestamp_clk);
> +	}
> +
>  	priv->write_reg = ctucan_write32_le;
>  	priv->read_reg = ctucan_read32_le;
>  
> @@ -1424,6 +1587,50 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigne
>  
>  	priv->can.clock.freq = can_clk_rate;
>  
> +	/* Obtain timestamping counter bit size */
> +	timestamp_bit_size = FIELD_GET(REG_ERR_CAPT_TS_BITS,
> +				       ctucan_read32(priv, CTUCANFD_ERR_CAPT));
> +
> +	/* The register value is actually bit_size - 1 */
> +	if (timestamp_bit_size) {
> +		timestamp_bit_size += 1;
> +	} else {
> +		/* For 2.x versions of the IP core, we will assume 64-bit counter
> +		 * if there was a 0 in the register.
> +		 */
> +		u32 version_reg = ctucan_read32(priv, CTUCANFD_DEVICE_ID);
> +		u32 major = FIELD_GET(REG_DEVICE_ID_VER_MAJOR, version_reg);
> +
> +		if (major == 2)
> +			timestamp_bit_size = 64;
> +		else
> +			priv->timestamp_possible = false;
> +	}
> +
> +	/* 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

> +		priv->work_delay_jiffies = nsecs_to_jiffies(work_delay_ns);
> +
> +		if (priv->work_delay_jiffies == 0)
> +			priv->timestamp_possible = false;
> +	}
> +

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)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ