Re: [PATCH 3/3] s390/time: Add PtP driver

[Heiko Carstens <hca@linux.ibm.com>]

On Tue, Oct 15, 2024 at 12:54:14PM +0200, Sven Schnelle wrote:
> Add a small PtP driver which allows user space to get
> the values of the physical and tod clock. This allows
> programs like chrony to use STP as clock source and
> steer the kernel clock. The physical clock can be used
> as a debugging aid to get the clock without any additional
> offsets like STP steering or LPAR offset.
> 
> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>

...

> +static __always_inline unsigned long eitod_to_ns(union tod_clock *clk)
> +{
> +	clk->eitod -= TOD_UNIX_EPOCH;
> +	return ((clk->eitod >> 9) * 125) + (((clk->eitod & 0x1ff) * 125) >> 9);
> +}

This is quite odd ;). This helper modifies the input union, which may
be very surprising to callers. It subtracts TOD_UNIX_EPOCH, which may
also be surprising, especially since we don't do that for tod_to_ns().

In addition the input value contains 72 bits, while the output value
is truncated to the lower 64 bits.

From my point of view this should look like

static __always_inline u128 eitod_to_ns(u128 todval)
{
	return (todval * 125) >> 9;
}

This way there are no surpring semantics (at least to me), and this
is more or less similar to tod_to_ns(). Since there won't be any
overflow with the multiplication it is also possible to simplify the
calculation.

Then it is up to the caller to subtract TOD_UNIX_EPOCH from the input
value before calling this helper, and the caller can also do
truncation in whatever way wanted.

> +bool stp_enabled(void);
>  #endif

Besides that there is an empty line missing before the endif: why is
this in timex.h and not in stp.h where it naturally would belong to?

> diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
> index 4214901c3ab0..47b20235953c 100644
> --- a/arch/s390/kernel/time.c
> +++ b/arch/s390/kernel/time.c
> @@ -469,6 +469,13 @@ static void __init stp_reset(void)
>  	}
>  }
>  
> +bool stp_enabled(void)
> +{
> +	return test_bit(CLOCK_SYNC_HAS_STP, &clock_sync_flags) &&
> +		stp_online;
> +}

Make this one long line, please.

> +config PTP_S390
> +	tristate "S390 PTP driver"
> +	depends on PTP_1588_CLOCK
> +	default y

Why default y?

> +static int ptp_s390_stcke_gettime(struct ptp_clock_info *ptp,
> +				  struct timespec64 *ts)
> +{
> +	union tod_clock tod;
> +
> +	if (!stp_enabled())
> +		return -EOPNOTSUPP;
> +
> +	store_tod_clock_ext_cc(&tod);

Why store_tod_clock_ext_cc()? This doesn't check the condition code,
but generates dead instructions (ipm+srl). store_tod_clock_ext() is
probably what should be used here?

> +static int s390_arch_ptp_get_crosststamp(ktime_t *device_time,
> +					 struct system_counterval_t *system_counter,
> +					 void *ctx)
> +{
> +	union tod_clock clk;
> +
> +	store_tod_clock_ext_cc(&clk);

Same here.

> +static struct ptp_clock_info ptp_s390_stcke_info = {
> +	.owner		= THIS_MODULE,
> +	.name		= "IBM Z STCKE Clock",

Please use "s390..." instead of "IBM Z...", which is the architecture
name within the kernel.

> +static struct ptp_clock_info ptp_s390_qpt_info = {
> +	.owner		= THIS_MODULE,
> +	.name		= "IBM Z Physical Clock",

Same.