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@xxxxxxxxxxxxx>
> Reviewed-by: Alexandra Winter <wintera@xxxxxxxxxxxxx>
> Reviewed-by: Benjamin Block <bblock@xxxxxxxxxxxxx>
> Reviewed-by: Nagamani PV <nagamani@xxxxxxxxxxxxx>
> ---
>  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.