[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20241016102046.16801-C-hca@linux.ibm.com>
Date: Wed, 16 Oct 2024 12:20:46 +0200
From: Heiko Carstens <hca@...ux.ibm.com>
To: Sven Schnelle <svens@...ux.ibm.com>
Cc: Vasily Gorbik <gor@...ux.ibm.com>,
Alexander Gordeev <agordeev@...ux.ibm.com>,
Christian Borntraeger <borntraeger@...ux.ibm.com>,
Richard Cochran <richardcochran@...il.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Ricardo B. Marliere" <ricardo@...liere.net>,
linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
netdev@...r.kernel.org
Subject: Re: [PATCH 3/3] s390/time: Add PtP driver
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@...ux.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.
Powered by blists - more mailing lists