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

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

On 16.06.2026 11:16, Cornelia Huck wrote:
> On Tue, Jun 16 2026, Peter Oberparleiter <oberpar@linux.ibm.com> wrote:
> 
>> 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" :)
> 
> :)
> 
> I'm wondering whether we should still expect to hit the "ssch with
> ena==0" situation, given that pm support has been removed again in the
> meanwhile. (Well, other than in situations like this, where it is a
> follow-up to other problems.) IOW, can callers expect not to see
> -EINVAL, unless they are doing something really stupid?

As Halil also pointed out, this would be a programming error, either on
the side of the driver that starts I/O without setting the device
properly online, or in the common I/O layer (hopefully not, but you
never know). Having a dedicated return code to identify this situation
is definitely useful, and we'll also consider documenting it accordingly
in the function comment.

>>>> 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.
> 
> Can't you zero the whole SCHIB, or do you still need some of the
> measurement block things for cleanup?

I faintly remember that there WAS a reason to use the remainder of the
SCHIB contents because of some unwanted effect that occurred if we
didn't, but I don't recall the details. We'll need to dig up the
associated bug report to understand it and determine if we can simply
clear all of the SCHIB, or need to keep some of the information intact.

>>>> 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.
> 
> I agree, I'd not expect to get -EINVAL in ccw_io_helper().
Yeah, this was definitely an unexpected side effect of the DNV commit.


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