From 152485bf76907ac7a2cc0a63b0822b23ef25da56 Mon Sep 17 00:00:00 2001 From: Julian Wiedmann Date: Wed, 6 Dec 2017 08:53:33 +0100 Subject: s390/qdio: simplify math in get_*_buffer_frontier() When determining the buffer count that get_buf_states() should be queried for, 'count' is capped at 127 buffers. So the check q->first_to_check == (q->first_to_check + count) % 128 can be reduced to count == 0 This helps to emphasize that get_buf_states() is really only called with count > 0. Signed-off-by: Julian Wiedmann Reviewed-by: Benjamin Block Signed-off-by: Martin Schwidefsky --- drivers/s390/cio/qdio_main.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) (limited to 'drivers/s390/cio/qdio_main.c') diff --git a/drivers/s390/cio/qdio_main.c b/drivers/s390/cio/qdio_main.c index d5b02de02a3a..6b340e6e29ac 100644 --- a/drivers/s390/cio/qdio_main.c +++ b/drivers/s390/cio/qdio_main.c @@ -502,8 +502,8 @@ static inline void inbound_primed(struct qdio_q *q, int count) static int get_inbound_buffer_frontier(struct qdio_q *q) { - int count, stop; unsigned char state = 0; + int count; q->timestamp = get_tod_clock_fast(); @@ -512,9 +512,7 @@ static int get_inbound_buffer_frontier(struct qdio_q *q) * would return 0. */ count = min(atomic_read(&q->nr_buf_used), QDIO_MAX_BUFFERS_MASK); - stop = add_buf(q->first_to_check, count); - - if (q->first_to_check == stop) + if (!count) goto out; /* @@ -734,8 +732,8 @@ void qdio_inbound_processing(unsigned long data) static int get_outbound_buffer_frontier(struct qdio_q *q) { - int count, stop; unsigned char state = 0; + int count; q->timestamp = get_tod_clock_fast(); @@ -751,8 +749,7 @@ static int get_outbound_buffer_frontier(struct qdio_q *q) * would return 0. */ count = min(atomic_read(&q->nr_buf_used), QDIO_MAX_BUFFERS_MASK); - stop = add_buf(q->first_to_check, count); - if (q->first_to_check == stop) + if (!count) goto out; count = get_buf_states(q, q->first_to_check, &state, count, 0, 1); -- cgit v1.2.3 From 0cf1e05157b9e5530dcc3ca9fec9bf617fc93375 Mon Sep 17 00:00:00 2001 From: Julian Wiedmann Date: Wed, 7 Mar 2018 14:01:01 +0100 Subject: s390/qdio: don't merge ERROR output buffers On an Output queue, both EMPTY and PENDING buffer states imply that the buffer is ready for completion-processing by the upper-layer drivers. So for a non-QEBSM Output queue, get_buf_states() merges mixed batches of PENDING and EMPTY buffers into one large batch of EMPTY buffers. The upper-layer driver (ie. qeth) later distuingishes PENDING from EMPTY by inspecting the slsb_state for QDIO_OUTBUF_STATE_FLAG_PENDING. But the merge logic in get_buf_states() contains a bug that causes us to erronously also merge ERROR buffers into such a batch of EMPTY buffers (ERROR is 0xaf, EMPTY is 0xa1; so ERROR & EMPTY == EMPTY). Effectively, most outbound ERROR buffers are currently discarded silently and processed as if they had succeeded. Note that this affects _all_ non-QEBSM device types, not just IQD with CQ. Fix it by explicitly spelling out the exact conditions for merging. For extracting the "get initial state" part out of the loop, this relies on the fact that get_buf_states() is never called with a count of 0. The QEBSM path already strictly requires this, and the two callers with variable 'count' make sure of it. Fixes: 104ea556ee7f ("qdio: support asynchronous delivery of storage blocks") Cc: #v3.2+ Signed-off-by: Julian Wiedmann Reviewed-by: Ursula Braun Reviewed-by: Benjamin Block Signed-off-by: Martin Schwidefsky --- drivers/s390/cio/qdio_main.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) (limited to 'drivers/s390/cio/qdio_main.c') diff --git a/drivers/s390/cio/qdio_main.c b/drivers/s390/cio/qdio_main.c index 6b340e6e29ac..bd26df85f559 100644 --- a/drivers/s390/cio/qdio_main.c +++ b/drivers/s390/cio/qdio_main.c @@ -214,7 +214,10 @@ again: return 0; } -/* returns number of examined buffers and their common state in *state */ +/* + * Returns number of examined buffers and their common state in *state. + * Requested number of buffers-to-examine must be > 0. + */ static inline int get_buf_states(struct qdio_q *q, unsigned int bufnr, unsigned char *state, unsigned int count, int auto_ack, int merge_pending) @@ -225,17 +228,23 @@ static inline int get_buf_states(struct qdio_q *q, unsigned int bufnr, if (is_qebsm(q)) return qdio_do_eqbs(q, state, bufnr, count, auto_ack); - for (i = 0; i < count; i++) { - if (!__state) { - __state = q->slsb.val[bufnr]; - if (merge_pending && __state == SLSB_P_OUTPUT_PENDING) - __state = SLSB_P_OUTPUT_EMPTY; - } else if (merge_pending) { - if ((q->slsb.val[bufnr] & __state) != __state) - break; - } else if (q->slsb.val[bufnr] != __state) - break; + /* get initial state: */ + __state = q->slsb.val[bufnr]; + if (merge_pending && __state == SLSB_P_OUTPUT_PENDING) + __state = SLSB_P_OUTPUT_EMPTY; + + for (i = 1; i < count; i++) { bufnr = next_buf(bufnr); + + /* merge PENDING into EMPTY: */ + if (merge_pending && + q->slsb.val[bufnr] == SLSB_P_OUTPUT_PENDING && + __state == SLSB_P_OUTPUT_EMPTY) + continue; + + /* stop if next state differs from initial state: */ + if (q->slsb.val[bufnr] != __state) + break; } *state = __state; return i; -- cgit v1.2.3 From c11a3dfd6fedd5266c2f9d7286981dc804dfb7cc Mon Sep 17 00:00:00 2001 From: Julian Wiedmann Date: Wed, 7 Mar 2018 14:19:43 +0100 Subject: s390/qdio: restrict buffer merging to eligible devices Only attempt to merge PENDING into EMPTY buffers for devices where the PENDING state is actually expected (ie. IQD with CQ). This might speed up the hot path a little bit. Signed-off-by: Julian Wiedmann Reviewed-by: Ursula Braun Reviewed-by: Benjamin Block Signed-off-by: Martin Schwidefsky --- drivers/s390/cio/qdio_main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'drivers/s390/cio/qdio_main.c') diff --git a/drivers/s390/cio/qdio_main.c b/drivers/s390/cio/qdio_main.c index bd26df85f559..63c6e9cf958f 100644 --- a/drivers/s390/cio/qdio_main.c +++ b/drivers/s390/cio/qdio_main.c @@ -761,7 +761,8 @@ static int get_outbound_buffer_frontier(struct qdio_q *q) if (!count) goto out; - count = get_buf_states(q, q->first_to_check, &state, count, 0, 1); + count = get_buf_states(q, q->first_to_check, &state, count, 0, + q->u.out.use_cq); if (!count) goto out; -- cgit v1.2.3 From dae55b6fef58530c13df074bcc182c096609339e Mon Sep 17 00:00:00 2001 From: Julian Wiedmann Date: Mon, 5 Mar 2018 09:39:38 +0100 Subject: s390/qdio: don't retry EQBS after CCQ 96 Immediate retry of EQBS after CCQ 96 means that we potentially misreport the state of buffers inspected during the first EQBS call. This occurs when 1. the first EQBS finds all inspected buffers still in the initial state set by the driver (ie INPUT EMPTY or OUTPUT PRIMED), 2. the EQBS terminates early with CCQ 96, and 3. by the time that the second EQBS comes around, the state of those previously inspected buffers has changed. If the state reported by the second EQBS is 'driver-owned', all we know is that the previous buffers are driver-owned now as well. But we can't tell if they all have the same state. So for instance - the second EQBS reports OUTPUT EMPTY, but any number of the previous buffers could be OUTPUT ERROR by now, - the second EQBS reports OUTPUT ERROR, but any number of the previous buffers could be OUTPUT EMPTY by now. Effectively, this can result in both over- and underreporting of errors. If the state reported by the second EQBS is 'HW-owned', that doesn't guarantee that the previous buffers have not been switched to driver-owned in the mean time. So for instance - the second EQBS reports INPUT EMPTY, but any number of the previous buffers could be INPUT PRIMED (or INPUT ERROR) by now. This would result in failure to process pending work on the queue. If it's the final check before yielding initiative, this can cause a (temporary) queue stall due to IRQ avoidance. Fixes: 25f269f17316 ("[S390] qdio: EQBS retry after CCQ 96") Cc: #v3.2+ Signed-off-by: Julian Wiedmann Reviewed-by: Benjamin Block Signed-off-by: Martin Schwidefsky --- drivers/s390/cio/qdio_main.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) (limited to 'drivers/s390/cio/qdio_main.c') diff --git a/drivers/s390/cio/qdio_main.c b/drivers/s390/cio/qdio_main.c index 63c6e9cf958f..de647b7e17b1 100644 --- a/drivers/s390/cio/qdio_main.c +++ b/drivers/s390/cio/qdio_main.c @@ -128,7 +128,7 @@ static inline int qdio_check_ccq(struct qdio_q *q, unsigned int ccq) static int qdio_do_eqbs(struct qdio_q *q, unsigned char *state, int start, int count, int auto_ack) { - int rc, tmp_count = count, tmp_start = start, nr = q->nr, retried = 0; + int rc, tmp_count = count, tmp_start = start, nr = q->nr; unsigned int ccq = 0; qperf_inc(q, eqbs); @@ -151,14 +151,7 @@ again: qperf_inc(q, eqbs_partial); DBF_DEV_EVENT(DBF_WARN, q->irq_ptr, "EQBS part:%02x", tmp_count); - /* - * Retry once, if that fails bail out and process the - * extracted buffers before trying again. - */ - if (!retried++) - goto again; - else - return count - tmp_count; + return count - tmp_count; } DBF_ERROR("%4x EQBS ERROR", SCH_NO(q)); -- cgit v1.2.3 From 88bf319fc2d6d971ef8692c2cae7f96708340461 Mon Sep 17 00:00:00 2001 From: Julian Wiedmann Date: Tue, 6 Mar 2018 17:58:49 +0100 Subject: s390/qdio: split up CCQ handling for EQBS / SQBS Get rid of the confusing two-stage translation in a hot path, and only handle CCQs that we anticipate for the respective command. Any unexpected value (such as CCQ 97 (rc == 1) for SQBS) should be considered a severe HW/driver bug, and traced as such. Signed-off-by: Julian Wiedmann Reviewed-by: Benjamin Block Signed-off-by: Martin Schwidefsky --- drivers/s390/cio/qdio_main.c | 77 +++++++++++++++++++------------------------- 1 file changed, 33 insertions(+), 44 deletions(-) (limited to 'drivers/s390/cio/qdio_main.c') diff --git a/drivers/s390/cio/qdio_main.c b/drivers/s390/cio/qdio_main.c index de647b7e17b1..a337281337a7 100644 --- a/drivers/s390/cio/qdio_main.c +++ b/drivers/s390/cio/qdio_main.c @@ -98,22 +98,6 @@ static inline int do_siga_output(unsigned long schid, unsigned long mask, return cc; } -static inline int qdio_check_ccq(struct qdio_q *q, unsigned int ccq) -{ - /* all done or next buffer state different */ - if (ccq == 0 || ccq == 32) - return 0; - /* no buffer processed */ - if (ccq == 97) - return 1; - /* not all buffers processed */ - if (ccq == 96) - return 2; - /* notify devices immediately */ - DBF_ERROR("%4x ccq:%3d", SCH_NO(q), ccq); - return -EIO; -} - /** * qdio_do_eqbs - extract buffer states for QEBSM * @q: queue to manipulate @@ -128,7 +112,7 @@ static inline int qdio_check_ccq(struct qdio_q *q, unsigned int ccq) static int qdio_do_eqbs(struct qdio_q *q, unsigned char *state, int start, int count, int auto_ack) { - int rc, tmp_count = count, tmp_start = start, nr = q->nr; + int tmp_count = count, tmp_start = start, nr = q->nr; unsigned int ccq = 0; qperf_inc(q, eqbs); @@ -138,27 +122,30 @@ static int qdio_do_eqbs(struct qdio_q *q, unsigned char *state, again: ccq = do_eqbs(q->irq_ptr->sch_token, state, nr, &tmp_start, &tmp_count, auto_ack); - rc = qdio_check_ccq(q, ccq); - if (!rc) - return count - tmp_count; - - if (rc == 1) { - DBF_DEV_EVENT(DBF_WARN, q->irq_ptr, "EQBS again:%2d", ccq); - goto again; - } - if (rc == 2) { + switch (ccq) { + case 0: + case 32: + /* all done, or next buffer state different */ + return count - tmp_count; + case 96: + /* not all buffers processed */ qperf_inc(q, eqbs_partial); DBF_DEV_EVENT(DBF_WARN, q->irq_ptr, "EQBS part:%02x", tmp_count); return count - tmp_count; + case 97: + /* no buffer processed */ + DBF_DEV_EVENT(DBF_WARN, q->irq_ptr, "EQBS again:%2d", ccq); + goto again; + default: + DBF_ERROR("%4x ccq:%3d", SCH_NO(q), ccq); + DBF_ERROR("%4x EQBS ERROR", SCH_NO(q)); + DBF_ERROR("%3d%3d%2d", count, tmp_count, nr); + q->handler(q->irq_ptr->cdev, QDIO_ERROR_GET_BUF_STATE, q->nr, + q->first_to_kick, count, q->irq_ptr->int_parm); + return 0; } - - DBF_ERROR("%4x EQBS ERROR", SCH_NO(q)); - DBF_ERROR("%3d%3d%2d", count, tmp_count, nr); - q->handler(q->irq_ptr->cdev, QDIO_ERROR_GET_BUF_STATE, - q->nr, q->first_to_kick, count, q->irq_ptr->int_parm); - return 0; } /** @@ -178,7 +165,6 @@ static int qdio_do_sqbs(struct qdio_q *q, unsigned char state, int start, unsigned int ccq = 0; int tmp_count = count, tmp_start = start; int nr = q->nr; - int rc; if (!count) return 0; @@ -188,23 +174,26 @@ static int qdio_do_sqbs(struct qdio_q *q, unsigned char state, int start, nr += q->irq_ptr->nr_input_qs; again: ccq = do_sqbs(q->irq_ptr->sch_token, state, nr, &tmp_start, &tmp_count); - rc = qdio_check_ccq(q, ccq); - if (!rc) { + + switch (ccq) { + case 0: + case 32: + /* all done, or active buffer adapter-owned */ WARN_ON_ONCE(tmp_count); return count - tmp_count; - } - - if (rc == 1 || rc == 2) { + case 96: + /* not all buffers processed */ DBF_DEV_EVENT(DBF_INFO, q->irq_ptr, "SQBS again:%2d", ccq); qperf_inc(q, sqbs_partial); goto again; + default: + DBF_ERROR("%4x ccq:%3d", SCH_NO(q), ccq); + DBF_ERROR("%4x SQBS ERROR", SCH_NO(q)); + DBF_ERROR("%3d%3d%2d", count, tmp_count, nr); + q->handler(q->irq_ptr->cdev, QDIO_ERROR_SET_BUF_STATE, q->nr, + q->first_to_kick, count, q->irq_ptr->int_parm); + return 0; } - - DBF_ERROR("%4x SQBS ERROR", SCH_NO(q)); - DBF_ERROR("%3d%3d%2d", count, tmp_count, nr); - q->handler(q->irq_ptr->cdev, QDIO_ERROR_SET_BUF_STATE, - q->nr, q->first_to_kick, count, q->irq_ptr->int_parm); - return 0; } /* -- cgit v1.2.3 From 89286320a236d245834075fa13adb0bdd827ecaa Mon Sep 17 00:00:00 2001 From: Julian Wiedmann Date: Wed, 21 Mar 2018 17:14:00 +0100 Subject: s390/qdio: clear intparm during shutdown During shutdown, qdio returns its ccw device back to control by the upper-layer driver. But there is a remote chance that by the time where the IRQ handler gets switched back, the interrupt for the preceding ccw_device_{clear,halt} hasn't been presented yet. Upper-layer drivers would then need to handle this IRQ - and since the IO is issued with an intparm, it could very well be confused with whatever intparm mechanism the driver uses itself (eg intparm == request address). So when switching over the IRQ handler, also clear the intparm and have upper-layer drivers deal with any such delayed interrupt as if it was unsolicited. Suggested-by: Sebastian Ott Signed-off-by: Julian Wiedmann Signed-off-by: Martin Schwidefsky --- drivers/s390/cio/qdio_main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'drivers/s390/cio/qdio_main.c') diff --git a/drivers/s390/cio/qdio_main.c b/drivers/s390/cio/qdio_main.c index a337281337a7..f4ca72dd862f 100644 --- a/drivers/s390/cio/qdio_main.c +++ b/drivers/s390/cio/qdio_main.c @@ -1207,8 +1207,10 @@ no_cleanup: qdio_shutdown_thinint(irq_ptr); /* restore interrupt handler */ - if ((void *)cdev->handler == (void *)qdio_int_handler) + if ((void *)cdev->handler == (void *)qdio_int_handler) { cdev->handler = irq_ptr->orig_handler; + cdev->private->intparm = 0; + } spin_unlock_irq(get_ccwdev_lock(cdev)); qdio_set_state(irq_ptr, QDIO_IRQ_STATE_INACTIVE); -- cgit v1.2.3