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

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

[William Bezenah]

When detaching virtio devices with multiple queues, spurious and
non-fatal error messages appear in the guest kernel log. While
virtio-net devices have multiple queues by default, this issue can
also be reproduced with other virtio device types (e.g., virtio-blk)
when configured with multiple queues:

[   33.820621] virtio_ccw 0.0.0001: Failed to deregister indicators (-22)
[   33.820628] virtio_net virtio2: Error -22 while deleting queue 0
[   33.820632] virtio_net virtio2: Error -22 while deleting queue 1
[   33.820634] virtio_net virtio2: Error -22 while deleting queue 2

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.

Fixes: 8c58a229688c ("s390/cio: Do not unregister the subchannel based on DNV")
Signed-off-by: William Bezenah <wbezenah@linux.ibm.com>
---
 drivers/s390/virtio/virtio_ccw.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index bab6cad3fd5c..02fd8bf7e469 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -429,7 +429,7 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev,
 			    vcdev->is_thinint ?
 			    VIRTIO_CCW_DOING_SET_IND_ADAPTER :
 			    VIRTIO_CCW_DOING_SET_IND);
-	if (ret && (ret != -ENODEV))
+	if (ret && (ret != -ENODEV) && (ret != -EINVAL))
 		dev_info(&vcdev->cdev->dev,
 			 "Failed to deregister indicators (%d)\n", ret);
 	else if (vcdev->is_thinint)
@@ -515,10 +515,10 @@ static void virtio_ccw_del_vq(struct virtqueue *vq, struct ccw1 *ccw)
 	ret = ccw_io_helper(vcdev, ccw,
 			    VIRTIO_CCW_DOING_SET_VQ | index);
 	/*
-	 * -ENODEV isn't considered an error: The device is gone anyway.
+	 * -ENODEV and -EINVAL aren't considered errors: The device is gone anyway.
 	 * This may happen on device detach.
 	 */
-	if (ret && (ret != -ENODEV))
+	if (ret && (ret != -ENODEV) && (ret != -EINVAL))
 		dev_warn(&vq->vdev->dev, "Error %d while deleting queue %d\n",
 			 ret, index);
 
-- 
2.54.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] If deregistering indicators from the host fails with an error other than -ENODEV or -EINVAL (e.g., -EIO), virtio_ccw_drop_indicator() skips the cleanup of local adapter interrupt pointers, leading to a subsequent use-after-free when virtqueues are deleted.
--

commit 6f54104d3533aa779587af76d171dec3ef8e1b8f
Author: William Bezenah <wbezenah@linux.ibm.com>

s390/virtio_ccw: Also suppress -EINVAL on device detach
    
This patch updates the error handling in virtio_ccw_del_vq() and
virtio_ccw_drop_indicator() to suppress -EINVAL along with -ENODEV
during device detach. This avoids spurious warning messages in the
kernel log when detaching virtio devices with multiple queues.

> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -429,7 +429,7 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev,
>  			    vcdev->is_thinint ?
>  			    VIRTIO_CCW_DOING_SET_IND_ADAPTER :
>  			    VIRTIO_CCW_DOING_SET_IND);
> -	if (ret && (ret != -ENODEV))
> +	if (ret && (ret != -ENODEV) && (ret != -EINVAL))
>  		dev_info(&vcdev->cdev->dev,
>  			 "Failed to deregister indicators (%d)\n", ret);
>  	else if (vcdev->is_thinint)
>  		virtio_ccw_drop_indicators(vcdev);

[Severity: Critical]
This is a pre-existing issue, but does this if/else structure create a
use-after-free risk during virtqueue deletion?

If ccw_io_helper() fails with an error other than -ENODEV or -EINVAL
(for example, -EIO), the first branch executes and logs the warning, but
the 'else if' branch is entirely skipped.

This means virtio_ccw_drop_indicators() is never called, failing to clear
the virtqueue pointers in the shared airq_info->aiv array.

Because virtio_ccw_del_vqs() subsequently proceeds to free these virtqueues
via vring_del_virtqueue(), the uncleared pointers in the shared airq_info
will point to freed memory.

If an adapter interrupt arrives later for this leaked bit, could
virtio_airq_handler() retrieve the dangling pointer and trigger a
use-after-free by calling vring_interrupt() on the freed queue?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612155407.199218-1-wbezenah@linux.ibm.com?part=1

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

[Halil Pasic]

On Fri, 12 Jun 2026 16:05:10 +0000
sashiko-bot@kernel.org wrote:

> [Severity: Critical]
> This is a pre-existing issue, but does this if/else structure create a
> use-after-free risk during virtqueue deletion?
> 
> If ccw_io_helper() fails with an error other than -ENODEV or -EINVAL
> (for example, -EIO), the first branch executes and logs the warning, but
> the 'else if' branch is entirely skipped.
> 
> This means virtio_ccw_drop_indicators() is never called, failing to clear
> the virtqueue pointers in the shared airq_info->aiv array.
> 
> Because virtio_ccw_del_vqs() subsequently proceeds to free these virtqueues
> via vring_del_virtqueue(), the uncleared pointers in the shared airq_info
> will point to freed memory.
> 
> If an adapter interrupt arrives later for this leaked bit, could
> virtio_airq_handler() retrieve the dangling pointer and trigger a
> use-after-free by calling vring_interrupt() on the freed queue?

Looks like this could be more robust against scenarios where we fail
to execute the channel program that is supposed to deregister the
indicators from the device.

The key question here is, at what point, can we consider adpter
interruption and event notification quiesced. I will have to do some
reading on that, I'm afraid. 

To me, it looks like just setting the virtqueue pointers in
airq_info->aiv would not do the trick either, because I don't see
a corresponding null pointer check.

Regards,
Halil

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

[Halil Pasic]

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.

Regards,
Halil 

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

[Cornelia Huck]

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.

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

[William Bezenah]

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.

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.

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.

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

[Halil Pasic]

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")?

> 
> 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.

> 
> 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?

Regards,
Halil

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

[Peter Oberparleiter]

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

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

[Cornelia Huck]

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?

>
>>> 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?

>
>>> 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().

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

[Halil Pasic]

On Tue, 16 Jun 2026 09:58:38 +0200
Peter Oberparleiter <oberpar@linux.ibm.com> wrote:

> >> 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.
> 

That would be a "programming error". I think it would be nice to document
that ccw_device_start() can return -EINVAL as well, and that the semantic
of EINVAL is basically execution of the function was not attempted,
because of a programming error. Or if we think that the list is not
exhaustive even after adding -EINVAL, we should state that as well in
some way one can program against.


> 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 fully agree! With the semantic of -EINVAL pinned down to programming
error, logging the condition seems to be the right thing to do. So we
should keep the old handing IMHO. 

Regards,
Halill