Re: [RFC PATCH v7 4/7] time: Add mechanism to recognize clocksource in time_get_snapshot

From: Thomas Gleixner <tglx@linutronix.de>
To: Jianyong Wu <jianyong.wu@arm.com>
Cc: netdev@vger.kernel.org, yangbo.lu@nxp.com,
	john.stultz@linaro.org, pbonzini@redhat.com,
	sean.j.christopherson@intel.com, maz@kernel.org,
	richardcochran@gmail.com, Mark.Rutland@arm.com, will@kernel.org,
	suzuki.poulose@arm.com, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
	Steve.Capper@arm.com, Kaly.Xin@arm.com, justin.he@arm.com,
	nd@arm.com
Subject: Re: [RFC PATCH v7 4/7] time: Add mechanism to recognize clocksource in time_get_snapshot
Date: Thu, 14 Nov 2019 15:19:01 +0100 (CET)	[thread overview]
Message-ID: <alpine.DEB.2.21.1911141507010.2507@nanos.tec.linutronix.de> (raw)
In-Reply-To: <20191114121358.6684-5-jianyong.wu@arm.com>

On Thu, 14 Nov 2019, Jianyong Wu wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> In some scenario like return device time to ptp_kvm guest,
> we need identify the current clocksource outside core time code.
> A mechanism added to recognize the current clocksource
> by export clocksource id in time_get_snapshot.

Can you please replace that with the following:

 System time snapshots are not conveying information about the current
 clocksource which was used, but callers like the PTP KVM guest
 implementation have the requirement to evaluate the clocksource type to
 select the appropriate mechanism.

 Introduce a clocksource id field in struct clocksource which is by default
 set to CSID_GENERIC (0). Clocksource implementations can set that field to
 a value which allows to identify the clocksource.

 Store the clocksource id of the current clocksource in the
 system_time_snapshot so callers can evaluate which clocksource was used to
 take the snapshot and act accordingly.

> diff --git a/include/linux/clocksource_ids.h b/include/linux/clocksource_ids.h
> new file mode 100644
> index 000000000000..93bec8426c44
> --- /dev/null
> +++ b/include/linux/clocksource_ids.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_CLOCKSOURCE_IDS_H
> +#define _LINUX_CLOCKSOURCE_IDS_H
> +
> +/* Enum to give clocksources a unique identifier */
> +enum clocksource_ids {
> +	CSID_GENERIC		= 0,
> +	CSID_ARM_ARCH_COUNTER,

This should only add the infrastructure with just CSID_GENERIC in place.

The ARM_ARCH variant needs to come in a seperate patch which adds the enum
and uses it in the corresponding driver. Seperate means a patch doing only
that and nothing else, i.e. not hidden in some other patch which actually
makes use of it.

Thanks,

	tglx