[PATCH v2 0/1] s390/qdio: Ensure QDIO_IRQ_STATE_ACTIVE is set only after firmware activates.

[PATCH v2 0/1] s390/qdio: Ensure QDIO_IRQ_STATE_ACTIVE is set only after firmware activates.

[Nihar Panda]

Data Race Fix: 
Moved the execution of cio_update_schib() and qdio_set_state() 
under ccwdev_lock.
This prevents a data race on the shared sch->schib structure and ensures 
qdio_set_state() does not overwrite a STOPPED/ERR state when called 
locklessly (as identified by Sashiko AI).

Timeout Optimization: 
Converted the timeout polling loop from a while to a do..while loop. 
This fixes a preemption bug where a thread interruption could trigger 
a false ETIMEDOUT failure before the loop body could execute even once.

Nihar Panda (1):
  s390/qdio: Ensure QDIO_IRQ_STATE_ACTIVE is set only after firmware
    activates.

 arch/s390/include/asm/scsw.h |  4 +--
 drivers/s390/cio/qdio_main.c | 59 +++++++++++++++++++++++++++---------
 2 files changed, 47 insertions(+), 16 deletions(-)

-- 
2.53.0

[PATCH v5 1/1] s390/qdio: Ensure QDIO_IRQ_STATE_ACTIVE is set only after firmware activates.

[Nihar Panda]

Set QDIO_IRQ_STATE_ACTIVE only if both the subchannel-active bit and
the QDIO-active bit are set in the Subchannel Status Word (SCSW).

The channel subsystem sets the SCSW_ACTL_SCHACT bit in scsw.actl and
scsw.qact = 1 in the SCHIB to indicate that the activate-QDIO-queues
CCW program is running and the queues are ready.

An interrupt-driven approach is not applicable here.
Using CCW_FLAG_PCI on the activate CCW generates an intermediate interrupt
too early, before the firmware sets qact=1.
Therefore, polling the SCHIB via cio_update_schib() is the only way to
reliably detect when the queues are ready.

Signed-off-by: Nihar Panda <niharp@linux.ibm.com>
Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
Reviewed-by: Nagamani PV <nagamani@linux.ibm.com>
---
 arch/s390/include/asm/scsw.h |  4 +--
 drivers/s390/cio/qdio_main.c | 59 +++++++++++++++++++++++++++---------
 2 files changed, 47 insertions(+), 16 deletions(-)

diff --git a/arch/s390/include/asm/scsw.h b/arch/s390/include/asm/scsw.h
index 56003e26cdbf..bf00d827d72b 100644
--- a/arch/s390/include/asm/scsw.h
+++ b/arch/s390/include/asm/scsw.h
@@ -28,7 +28,7 @@
  * @zcc: zero condition code
  * @ectl: extended control
  * @pno: path not operational
- * @res: reserved
+ * @qact: qdio active
  * @fctl: function control
  * @actl: activity control
  * @stctl: status control
@@ -50,7 +50,7 @@ struct cmd_scsw {
 	__u32 zcc  : 1;
 	__u32 ectl : 1;
 	__u32 pno  : 1;
-	__u32 res  : 1;
+	__u32 qact : 1;
 	__u32 fctl : 3;
 	__u32 actl : 7;
 	__u32 stctl : 5;
diff --git a/drivers/s390/cio/qdio_main.c b/drivers/s390/cio/qdio_main.c
index c1e09fa34e77..0f58ae1175ee 100644
--- a/drivers/s390/cio/qdio_main.c
+++ b/drivers/s390/cio/qdio_main.c
@@ -1140,11 +1140,29 @@ EXPORT_SYMBOL_GPL(qdio_establish);
 /**
  * qdio_activate - activate queues on a qdio subchannel
  * @cdev: associated cdev
+ *
+ * This function must only be called when the QDIO subchannel is in
+ * QDIO_IRQ_STATE_ESTABLISHED state (i.e., after successful qdio_establish()).
+ * Any other state indicates either the subchannel is not ready or an error
+ * condition that requires proper recovery through qdio_shutdown() and
+ * qdio_establish() before activation can be attempted.
+ *
+ * Return:
+ * * 0		- success
+ * * -ENODEV	- device is not initialized
+ * * -EIO	- adapter lacks QDIO activation support, or
+ *                the IRQ state changed unexpectedly during activation
+ * * -EBUSY	- subchannel state is not QDIO_IRQ_STATE_ESTABLISHED
+ *                at call time
+ * * -ETIMEDOUT - subchannel failed to become active within the timeout
+ * * other	- standard error code forwarded from ccw_device_start()
  */
 int qdio_activate(struct ccw_device *cdev)
 {
+	struct subchannel *sch = to_subchannel(cdev->dev.parent);
 	struct qdio_irq *irq_ptr = cdev->private->qdio_data;
 	struct subchannel_id schid;
+	unsigned long timeout;
 	struct ciw *ciw;
 	int rc;
 
@@ -1161,7 +1179,8 @@ int qdio_activate(struct ccw_device *cdev)
 	}
 
 	mutex_lock(&irq_ptr->setup_mutex);
-	if (irq_ptr->state == QDIO_IRQ_STATE_INACTIVE) {
+	if (irq_ptr->state != QDIO_IRQ_STATE_ESTABLISHED) {
+		DBF_ERROR("%4x act WS:%d", schid.sch_no, irq_ptr->state);
 		rc = -EBUSY;
 		goto out;
 	}
@@ -1178,23 +1197,35 @@ int qdio_activate(struct ccw_device *cdev)
 			      0, DOIO_DENY_PREFETCH);
 	spin_unlock_irq(get_ccwdev_lock(cdev));
 	if (rc) {
-		DBF_ERROR("%4x act IO ERR", irq_ptr->schid.sch_no);
-		DBF_ERROR("rc:%4x", rc);
+		DBF_ERROR("%4x act IE:%d", irq_ptr->schid.sch_no, rc);
 		goto out;
 	}
 
-	/* wait for subchannel to become active */
-	msleep(5);
+	rc = -ETIMEDOUT;
+	timeout = jiffies + HZ;
 
-	switch (irq_ptr->state) {
-	case QDIO_IRQ_STATE_STOPPED:
-	case QDIO_IRQ_STATE_ERR:
-		rc = -EIO;
-		break;
-	default:
-		qdio_set_state(irq_ptr, QDIO_IRQ_STATE_ACTIVE);
-		rc = 0;
-	}
+	do {
+		msleep(1);
+		if (irq_ptr->state != QDIO_IRQ_STATE_ESTABLISHED) {
+			rc = -EIO;
+			DBF_ERROR("%4x act WS:%d", irq_ptr->schid.sch_no, irq_ptr->state);
+			break;
+		}
+		/* Query hardware */
+		spin_lock_irq(get_ccwdev_lock(cdev));
+		if (cio_update_schib(sch) == 0) {
+			if ((sch->schib.scsw.cmd.actl & SCSW_ACTL_SCHACT)
+					&& sch->schib.scsw.cmd.qact) {
+				qdio_set_state(irq_ptr, QDIO_IRQ_STATE_ACTIVE);
+				rc = 0;
+			}
+		}
+		spin_unlock_irq(get_ccwdev_lock(cdev));
+		if (!rc)
+			break;
+	} while (time_before(jiffies, timeout));
+	if (rc == -ETIMEDOUT)
+		DBF_ERROR("%4x act TMOUT", irq_ptr->schid.sch_no);
 out:
 	mutex_unlock(&irq_ptr->setup_mutex);
 	return rc;
-- 
2.53.0

Re: [PATCH v5 1/1] s390/qdio: Ensure QDIO_IRQ_STATE_ACTIVE is set only after firmware activates.

[Heiko Carstens]

On Thu, Sep 24, 2026 at 07:01:36AM +0200, Nihar Panda wrote:
> Set QDIO_IRQ_STATE_ACTIVE only if both the subchannel-active bit and
> the QDIO-active bit are set in the Subchannel Status Word (SCSW).
> 
> The channel subsystem sets the SCSW_ACTL_SCHACT bit in scsw.actl and
> scsw.qact = 1 in the SCHIB to indicate that the activate-QDIO-queues
> CCW program is running and the queues are ready.
> 
> An interrupt-driven approach is not applicable here.
> Using CCW_FLAG_PCI on the activate CCW generates an intermediate interrupt
> too early, before the firmware sets qact=1.
> Therefore, polling the SCHIB via cio_update_schib() is the only way to
> reliably detect when the queues are ready.
> 
> Signed-off-by: Nihar Panda <niharp@linux.ibm.com>
> Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
> Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
> Reviewed-by: Nagamani PV <nagamani@linux.ibm.com>
> ---
>  arch/s390/include/asm/scsw.h |  4 +--
>  drivers/s390/cio/qdio_main.c | 59 +++++++++++++++++++++++++++---------
>  2 files changed, 47 insertions(+), 16 deletions(-)

Unfortunately the cover letter does not mention what has changed
compared to the previous version. Also there seems to be a confusion
between versions. Cover-letter says v2, while the patch says v5.

In addition the code changed obviously. Is it ok to keep the Reviewed-by tags
from above which were given to a previous version of the code?

> -	/* wait for subchannel to become active */
> -	msleep(5);
> +	rc = -ETIMEDOUT;
> +	timeout = jiffies + HZ;
>  
> -	switch (irq_ptr->state) {
> -	case QDIO_IRQ_STATE_STOPPED:
> -	case QDIO_IRQ_STATE_ERR:
> -		rc = -EIO;
> -		break;
> -	default:
> -		qdio_set_state(irq_ptr, QDIO_IRQ_STATE_ACTIVE);
> -		rc = 0;
> -	}
> +	do {
> +		msleep(1);
> +		if (irq_ptr->state != QDIO_IRQ_STATE_ESTABLISHED) {
> +			rc = -EIO;
> +			DBF_ERROR("%4x act WS:%d", irq_ptr->schid.sch_no, irq_ptr->state);
> +			break;
> +		}
> +		/* Query hardware */
> +		spin_lock_irq(get_ccwdev_lock(cdev));
> +		if (cio_update_schib(sch) == 0) {
> +			if ((sch->schib.scsw.cmd.actl & SCSW_ACTL_SCHACT)
> +					&& sch->schib.scsw.cmd.qact) {
> +				qdio_set_state(irq_ptr, QDIO_IRQ_STATE_ACTIVE);
> +				rc = 0;
> +			}
> +		}
> +		spin_unlock_irq(get_ccwdev_lock(cdev));
> +		if (!rc)
> +			break;
> +	} while (time_before(jiffies, timeout));
> +	if (rc == -ETIMEDOUT)
> +		DBF_ERROR("%4x act TMOUT", irq_ptr->schid.sch_no);
>  out:
>  	mutex_unlock(&irq_ptr->setup_mutex);

As already mentioned in a previous comment: "worst case" is that this would
timeout after waiting only 1ms (+ preemption), compared to before where there
was a guaranteed minimum wait time of 5ms.
Is this change intended? Could this lead to regressions?
If this is intended it should be described.

Usually problems like this are avoided by retrying n times, instead of using a
fixed timeout value.