[PATCH] s390/stp: Drop CLOCK_SYNC_STP

[PATCH] s390/stp: Drop CLOCK_SYNC_STP

[Sven Schnelle]

CLOCK_SYNC_STP is never set when the stp=1 kernel command line option
is used, or stp is enabled by default. This cause get_phys_clock to
return -EACCES. Fix this by testing stp_online and remove
CLOCK_SYNC_STP.

Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
---
 arch/s390/kernel/time.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
index 2b989bebd220..de80e2a316ba 100644
--- a/arch/s390/kernel/time.c
+++ b/arch/s390/kernel/time.c
@@ -69,6 +69,7 @@ EXPORT_SYMBOL(ptff_function_mask);
 
 static unsigned long lpar_offset;
 static unsigned long initial_leap_seconds;
+static bool stp_online = true;
 
 /*
  * Get time offsets with PTFF
@@ -274,8 +275,7 @@ static DEFINE_MUTEX(stp_mutex);
 static unsigned long clock_sync_flags;
 
 #define CLOCK_SYNC_HAS_STP		0
-#define CLOCK_SYNC_STP			1
-#define CLOCK_SYNC_STPINFO_VALID	2
+#define CLOCK_SYNC_STPINFO_VALID	1
 
 /*
  * The get_clock function for the physical clock. It will get the current
@@ -299,7 +299,7 @@ int get_phys_clock(unsigned long *clock)
 		return 0;
 	if (!test_bit(CLOCK_SYNC_HAS_STP, &clock_sync_flags))
 		return -EOPNOTSUPP;
-	if (!test_bit(CLOCK_SYNC_STP, &clock_sync_flags))
+	if (!stp_online)
 		return -EACCES;
 	return -EAGAIN;
 }
@@ -397,7 +397,6 @@ struct clock_sync_data {
 /*
  * Server Time Protocol (STP) code.
  */
-static bool stp_online = true;
 static struct stp_sstpi stp_info;
 static void *stp_page;
 
@@ -795,10 +794,6 @@ static ssize_t online_store(struct device *dev,
 		return -EOPNOTSUPP;
 	mutex_lock(&stp_mutex);
 	stp_online = value;
-	if (stp_online)
-		set_bit(CLOCK_SYNC_STP, &clock_sync_flags);
-	else
-		clear_bit(CLOCK_SYNC_STP, &clock_sync_flags);
 	queue_work(time_sync_wq, &stp_work);
 	mutex_unlock(&stp_mutex);
 	return count;
-- 
2.53.0

Re: [PATCH] s390/stp: Drop CLOCK_SYNC_STP

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Booting on systems with XRC-capable DASDs but no active STP network will hang indefinitely due to an unhandled -EAGAIN loop.
- [Low] Lockless plain read/write of `stp_online` introduces a formal data race.
--

--- Patch [1]: [PATCH] s390/stp: Drop CLOCK_SYNC_STP ---
commit 39bb08d28d4e120a87017a08eb301d774dd6baa2
Author: Sven Schnelle <svens@linux.ibm.com>

s390/stp: Drop CLOCK_SYNC_STP

This commit removes the CLOCK_SYNC_STP flag in favor of using the stp_online
boolean directly. This change prevents get_phys_clock() from returning -EACCES
when the kernel command line option stp=1 is used or STP is enabled by default.

> diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
> index 2b989bebd220a..de80e2a316ba7 100644
> --- a/arch/s390/kernel/time.c
> +++ b/arch/s390/kernel/time.c
[ ... ]
> @@ -299,7 +299,7 @@ int get_phys_clock(unsigned long *clock)
>  		return 0;
>  	if (!test_bit(CLOCK_SYNC_HAS_STP, &clock_sync_flags))
>  		return -EOPNOTSUPP;
> -	if (!test_bit(CLOCK_SYNC_STP, &clock_sync_flags))
> +	if (!stp_online)

[Severity: Low]
Does this introduce a formal data race under the LKMM?

get_phys_clock() now performs a lockless plain read of stp_online.
Meanwhile, online_store() modifies it concurrently via a plain write:

arch/s390/kernel/time.c:online_store() {
    ...
    mutex_lock(&stp_mutex);
    stp_online = value;
    ...
}

Could this trigger KCSAN warnings when userspace toggles the sysfs
online attribute while get_phys_clock() is executing?

>  		return -EACCES;
>  	return -EAGAIN;
>  }

[Severity: High]
Could this change cause a boot hang on systems with XRC-capable DASDs
but no active STP network?

Because stp_online is initialized to true, if the machine supports the
STP facility but is not connected to a usable STP time source, time
synchronization won't occur and stp_online will remain true.

With stp_online remaining true, get_phys_clock() will continuously fall
through and return -EAGAIN, rather than safely returning -EACCES as it
did prior to this patch.

The DASD driver ignores -EACCES but propagates -EAGAIN:

drivers/s390/block/dasd_eckd.c:set_timestamp() {
    ...
	if ((rc && !private->rdc_data.facilities.XRC_supported) ||
	    rc == -EOPNOTSUPP || rc == -EACCES)
		return 0;
    ...
}

If -EAGAIN is propagated up, will the block layer endlessly retry the I/O,
resulting in a complete hang during boot?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814072223.2218864-1-svens@linux.ibm.com?part=1