Re: [PATCH] s390/stp: Drop CLOCK_SYNC_STP

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 <[email protected]>

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/[email protected]?part=1