[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

Re: [PATCH] s390/stp: Drop CLOCK_SYNC_STP

[Stefan Haberland]

Am 14.08.26 um 09:34 schrieb sashiko-bot@kernel.org:
> 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?
> 

From what I see, Sashiko is right. This will break how the DASD driver
actually checks if the clock is enabled and in sync.

The DASD driver relies on -EACCES to know that the clock exists but STP
sync is not online/applicable which in turn will skip it, whereas
-EAGAIN means the clock does exist but is not in sync yet and we should
wait and retry, which in turn leads to endless loop of request never
being built because we never get a valid timestamp.

So I guess we will still need a third bit to differentiate those states.

Re: [PATCH] s390/stp: Drop CLOCK_SYNC_STP

[Sven Schnelle]

Stefan Haberland <sth@linux.ibm.com> writes:

> Am 14.08.26 um 09:34 schrieb sashiko-bot@kernel.org:
>> [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?
>> 
>
> From what I see, Sashiko is right. This will break how the DASD driver
> actually checks if the clock is enabled and in sync.
>
> The DASD driver relies on -EACCES to know that the clock exists but STP
> sync is not online/applicable which in turn will skip it, whereas
> -EAGAIN means the clock does exist but is not in sync yet and we should
> wait and retry, which in turn leads to endless loop of request never
> being built because we never get a valid timestamp.
>
> So I guess we will still need a third bit to differentiate those states.

If I understood the code correctly, it would only block until STP is in
sync, so this is expected behaviour - looking at z/VM documentation (especially
the XRC_OPTional flag), DASD I/O should be blocked until time is in
sync.

I think the correct way would be to add a patch on top that adds the
XRC_OPTional parameter to either block DASD I/O when STP is in unsynchronized
state (current behaviour) or just omits the timestamp when XRC_OPTional
is set.

Re: [PATCH] s390/stp: Drop CLOCK_SYNC_STP

[Stefan Haberland]

Am 09.09.26 um 14:29 schrieb Sven Schnelle:
> Stefan Haberland <sth@linux.ibm.com> writes:
> 
>> Am 14.08.26 um 09:34 schrieb sashiko-bot@kernel.org:
>>> [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?
>>>
>>
>> From what I see, Sashiko is right. This will break how the DASD driver
>> actually checks if the clock is enabled and in sync.
>>
>> The DASD driver relies on -EACCES to know that the clock exists but STP
>> sync is not online/applicable which in turn will skip it, whereas
>> -EAGAIN means the clock does exist but is not in sync yet and we should
>> wait and retry, which in turn leads to endless loop of request never
>> being built because we never get a valid timestamp.
>>
>> So I guess we will still need a third bit to differentiate those states.
> 
> If I understood the code correctly, it would only block until STP is in
> sync, so this is expected behaviour - looking at z/VM documentation (especially
> the XRC_OPTional flag), DASD I/O should be blocked until time is in
> sync.
> 
> I think the correct way would be to add a patch on top that adds the
> XRC_OPTional parameter to either block DASD I/O when STP is in unsynchronized
> state (current behaviour) or just omits the timestamp when XRC_OPTional
> is set.

Not sure XRC_OPTional fully covers the case here, though.
The existing "don't count -EAGAIN against retries" logic assumes the
clock will eventually converge, a real STP network mid-sync.

But the scenario here is different: no CTN/network attached at all, so
there's nothing to converge the state only changes via admin action, not
by waiting.

Wouldn't a mandatory-XRC device on a machine with no STP network at all
still block forever either way, regardless of XRC_OPTional? That flag
decides whether a given volume needs a timestamp, but not whether the
system can ever produce one.

Could it make sense for get_phys_clock() to distinguish "converging"
from "no usable source at all" itself independent of the XRC_OPTional patch?