Re: [PATCH v1] s390/virtio_ccw: Also suppress -EINVAL on device detach

[Peter Oberparleiter <oberpar@linux.ibm.com>]

On 15.06.2026 23:42, Halil Pasic wrote:
> On Mon, 15 Jun 2026 16:01:55 -0400
> William Bezenah <wbezenah@linux.ibm.com> wrote:
> 
>> On 6/15/2026 10:58 AM, Cornelia Huck wrote:
>>> On Mon, Jun 15 2026, Halil Pasic <pasic@linux.ibm.com> wrote:
>>>  
>>>> On Fri, 12 Jun 2026 17:54:07 +0200
>>>> William Bezenah <wbezenah@linux.ibm.com> wrote:
>>>>  
>>>>> Since commit 8c58a229688c ("s390/cio: Do not unregister the
>>>>> subchannel based on DNV"), subchannel behavior following a device
>>>>> detach has been updated and results in -EINVAL being propagated
>>>>> rather than -ENODEV, originating from ccw_device_start_timeout_key()
>>>>> in cio/device_ops. In the end, the virtio driver has no ability to
>>>>> react to the difference between device and subchannel states here,
>>>>> and during detach, both -ENODEV and -EINVAL indicate the device
>>>>> cannot be used and should not be treated as errors requiring
>>>>> attention. Update error handling in virtio_ccw_del_vq() and
>>>>> virtio_ccw_drop_indicator() to suppress -EINVAL in addition to
>>>>> -ENODEV.  
>>>> Hi William!
>>>>
>>>> Are you saying that ccw_device_start() started returning -EINVAL
>>>> since 8c58a229688c ("s390/cio: Do not unregister the subchannel based on
>>>> DNV")? Or did I somehow read the paragraph wrong?
>>>>
>>>> The funcition ccw_device_start is documented to return:
>>>>  * Returns:                                                                     
>>>>  *  %0, if the operation was successful;                                        
>>>>  *  -%EBUSY, if the device is busy, or status pending;                          
>>>>  *  -%EACCES, if no path specified in @lpm is operational;                      
>>>>  *  -%ENODEV, if the device is not operational. 
>>>> and the commit message does not say a thing about introducing -EINVAL to
>>>> the mix.  
>>> The function may return -EINVAL for non-enabled subchannels
>>> (i.e. pmcw.ena == 0), maybe we get an all-zeroes schib with dnv == 0?
>>> I'd expect it not to be enabled in that case anyway.  
>>
>> Yep, that's at least how I've come to understand what changed. The
>> function ccw_device_start_timeout_key() has always returned -EINVAL
>> for non-enabled subchannels (pmcw.ena == 0), though it's not
>> documented in the header.
> 
> Wasn't his -EINVAL actually introduced by commit:
> 823d494ac111 ("[S390] pm: ccw bus power management callbacks")?

In the context of virtio-ccw added in 2012, an EINVAL return code
introduced in 2009 might be considered "always" :)

>> What changed with commit 8c58a229688c is that cio_update_schib() now
>> updates the schib even when DNV=0, rather than returning early as it
>> did previously. Somehow this update results in pmcw.ena == 0 in
>> ccw_device_start_timeout_key(). Previously, it saw pmcw.ena == 1 and
>> moved to the condition (cdev->private->state == DEV_STATE_NOT_OPER)
>> where it returned -ENODEV.
> 
> Sounds fishy to me. As far as I understand the DNV takes precedence over
> all other pieces of PMCW.

And you're right about that! The Principles of Operation states (p. 15-4
in SA22-7832-14 [1]) that the contents of all other fields in the PMCW
are unpredictable when DNV is 0, therefore 8c58a229688c is in error.

I'll work with Vineeth to determine how to fix this issue, potentially
via manually clearing some relevant SCHIB fields instead of copying the
unpredictable results of the STSCH instruction.

>> So the commit didn't introduce -EINVAL as a new return value, rather,
>> it changed the subchannel lifecycle such that existing paths now
>> propagate -EINVAL rather than -ENODEV during the device detach
>> scenario.
> 
> I'm not convinced returning -EINVAL in the given situation is the
> right thing to do. Peter, would you mind to chime in?

I tend to agree that an attempt to start I/O for a subchannel that has
DNV 0 should result in ENODEV rather than EINVAL, though the latter is
still valid when a driver tries to start I/O on a subchannel that is not
enabled for I/O.

We'll make sure to design the fix for 8c58a229688c in away that ENODEV
will be returned when DNV is 0. Assuming that this is the only situation
where virtio-ccw's ccw_io_helper() receives -EINVAL from
ccw_device_start__timeout_key() during detach, the subject patch should
no longer be necessary.


[1] https://www.ibm.com/docs/en/module_1678991624569/pdf/SA22-7832-14.pdf

-- 
Peter Oberparleiter
Linux on IBM Z Development - IBM Germany R&D