Re: [PATCH] s390/stp: Drop CLOCK_SYNC_STP

[Sven Schnelle <svens@linux.ibm.com>]

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.