[PATCH 0/5] skd fixes for kernel v4.14

[PATCH 0/5] skd fixes for kernel v4.14

[Bart Van Assche]

Hello Jens,

Code review resulted in five more patches for the skd (sTec s1120) kernel
driver. These patches have been tested on top of the for-next branch from this
morning of your block layer git repository. That means that Dan Carpenter's
patches were included in the kernel tree used for testing. Please consider
these patches for kernel v4.14.

Thanks,

Bart.

Bart Van Assche (5):
  block: Warn if blk_queue_rq_timed_out() is called for a blk-mq queue
  skd: Report completion mismatches once
  skd: Inline skd_process_request()
  skd: Avoid double completions in case of a timeout
  skd: Change default interrupt mode to MSI-X

 block/blk-settings.c     |  1 +
 drivers/block/skd_main.c | 68 ++++++++++++++++++------------------------------
 2 files changed, 27 insertions(+), 42 deletions(-)

-- 
2.14.0

[PATCH 1/5] block: Warn if blk_queue_rq_timed_out() is called for a blk-mq queue

[Bart Van Assche]

The timeout handler set by blk_queue_rq_timed_out() is only used
in single queue mode. Calling this function for blk-mq drivers is
wrong. Hence issue a warning if this function is called by a blk-mq
driver.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 block/blk-settings.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index be1f115b538b..8559e9563c52 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -68,6 +68,7 @@ EXPORT_SYMBOL_GPL(blk_queue_rq_timeout);
 
 void blk_queue_rq_timed_out(struct request_queue *q, rq_timed_out_fn *fn)
 {
+	WARN_ON_ONCE(q->mq_ops);
 	q->rq_timed_out_fn = fn;
 }
 EXPORT_SYMBOL_GPL(blk_queue_rq_timed_out);
-- 
2.14.0

Re: [PATCH 1/5] block: Warn if blk_queue_rq_timed_out() is called for a blk-mq queue

[Christoph Hellwig]

On Wed, Aug 23, 2017 at 10:56:29AM -0700, Bart Van Assche wrote:
> The timeout handler set by blk_queue_rq_timed_out() is only used
> in single queue mode. Calling this function for blk-mq drivers is
> wrong. Hence issue a warning if this function is called by a blk-mq
> driver.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/5] skd: Report completion mismatches once

[Bart Van Assche]

This patch removes one debug statement but otherwise does not change
any functionality.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/block/skd_main.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
index a467c18cc047..d21fc76c5ed9 100644
--- a/drivers/block/skd_main.c
+++ b/drivers/block/skd_main.c
@@ -1564,17 +1564,11 @@ static int skd_isr_completion_posted(struct skd_device *skdev,
 		 * Make sure the request ID for the slot matches.
 		 */
 		if (skreq->id != req_id) {
-			dev_dbg(&skdev->pdev->dev,
-				"mismatch comp_id=0x%x req_id=0x%x\n", req_id,
-				skreq->id);
-			{
-				u16 new_id = cmp_cntxt;
-				dev_err(&skdev->pdev->dev,
-					"Completion mismatch comp_id=0x%04x skreq=0x%04x new=0x%04x\n",
-					req_id, skreq->id, new_id);
+			dev_err(&skdev->pdev->dev,
+				"Completion mismatch comp_id=0x%04x skreq=0x%04x new=0x%04x\n",
+				req_id, skreq->id, cmp_cntxt);
 
-				continue;
-			}
+			continue;
 		}
 
 		SKD_ASSERT(skreq->state == SKD_REQ_STATE_BUSY);
-- 
2.14.0

Re: [PATCH 2/5] skd: Report completion mismatches once

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 3/5] skd: Inline skd_process_request()

[Bart Van Assche]

This patch does not change any functionality but makes the skd
driver code more similar to that of other blk-mq kernel drivers.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/block/skd_main.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
index d21fc76c5ed9..0d6340884009 100644
--- a/drivers/block/skd_main.c
+++ b/drivers/block/skd_main.c
@@ -478,8 +478,10 @@ static bool skd_fail_all(struct request_queue *q)
 	}
 }
 
-static void skd_process_request(struct request *req, bool last)
+static blk_status_t skd_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
+				    const struct blk_mq_queue_data *mqd)
 {
+	struct request *const req = mqd->rq;
 	struct request_queue *const q = req->q;
 	struct skd_device *skdev = q->queuedata;
 	struct skd_fitmsg_context *skmsg;
@@ -492,6 +494,11 @@ static void skd_process_request(struct request *req, bool last)
 	const u32 count = blk_rq_sectors(req);
 	const int data_dir = rq_data_dir(req);
 
+	if (unlikely(skdev->state != SKD_DRVR_STATE_ONLINE))
+		return skd_fail_all(q) ? BLK_STS_IOERR : BLK_STS_RESOURCE;
+
+	blk_mq_start_request(req);
+
 	WARN_ONCE(tag >= skd_max_queue_depth, "%#x > %#x (nr_requests = %lu)\n",
 		  tag, skd_max_queue_depth, q->nr_requests);
 
@@ -514,7 +521,7 @@ static void skd_process_request(struct request *req, bool last)
 		dev_dbg(&skdev->pdev->dev, "error Out\n");
 		skd_end_request(skdev, blk_mq_rq_from_pdu(skreq),
 				BLK_STS_RESOURCE);
-		return;
+		return BLK_STS_OK;
 	}
 
 	dma_sync_single_for_device(&skdev->pdev->dev, skreq->sksg_dma_address,
@@ -578,30 +585,13 @@ static void skd_process_request(struct request *req, bool last)
 	if (skd_max_req_per_msg == 1) {
 		skd_send_fitmsg(skdev, skmsg);
 	} else {
-		if (last ||
+		if (mqd->last ||
 		    fmh->num_protocol_cmds_coalesced >= skd_max_req_per_msg) {
 			skd_send_fitmsg(skdev, skmsg);
 			skdev->skmsg = NULL;
 		}
 		spin_unlock_irqrestore(&skdev->lock, flags);
 	}
-}
-
-static blk_status_t skd_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
-				    const struct blk_mq_queue_data *mqd)
-{
-	struct request *req = mqd->rq;
-	struct request_queue *q = req->q;
-	struct skd_device *skdev = q->queuedata;
-
-	if (skdev->state == SKD_DRVR_STATE_ONLINE) {
-		blk_mq_start_request(req);
-		skd_process_request(req, mqd->last);
-
-		return BLK_STS_OK;
-	} else {
-		return skd_fail_all(q) ? BLK_STS_IOERR : BLK_STS_RESOURCE;
-	}
 
 	return BLK_STS_OK;
 }
-- 
2.14.0

Re: [PATCH 3/5] skd: Inline skd_process_request()

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 4/5] skd: Avoid double completions in case of a timeout

[Bart Van Assche]

Avoid that normal request completion and the timeout handler can
run concurrently by calling blk_mq_complete_request() instead of
blk_mq_end_request() from skd_end_request(). Avoid that the block
layer can reuse a request while the firmware is still processing
it. Convert skd_softirq_done() to blk-mq. Pass the pointer to
skd_softirq_done() to the block layer core through
blk_mq_ops.complete instead of by calling blk_queue_softirq_done().
Pass the pointer to skd_timed_out() to the block layer core
through blk_mq_ops.timeout instead of by calling
blk_queue_timed_out(). The timeout handler has been tested as
follows:

    echo 1 > /sys/block/skd0/io-timeout-fail &&
    (cd /sys/kernel/debug/fail_io_timeout &&
      echo 100 > probability &&
      echo N > task-filter &&
      echo 1 > times)

Fixes: commit a74d5b76fab9 ("skd: Switch to block layer timeout mechanism")
Reported-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/block/skd_main.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
index 0d6340884009..ff288f1a5dec 100644
--- a/drivers/block/skd_main.c
+++ b/drivers/block/skd_main.c
@@ -184,6 +184,7 @@ struct skd_request_context {
 
 	struct fit_comp_error_info err_info;
 
+	blk_status_t status;
 };
 
 struct skd_special_context {
@@ -596,19 +597,22 @@ static blk_status_t skd_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
 	return BLK_STS_OK;
 }
 
-static enum blk_eh_timer_return skd_timed_out(struct request *req)
+static enum blk_eh_timer_return skd_timed_out(struct request *req,
+					      bool reserved)
 {
 	struct skd_device *skdev = req->q->queuedata;
 
 	dev_err(&skdev->pdev->dev, "request with tag %#x timed out\n",
 		blk_mq_unique_tag(req));
 
-	return BLK_EH_HANDLED;
+	return BLK_EH_RESET_TIMER;
 }
 
 static void skd_end_request(struct skd_device *skdev, struct request *req,
 			    blk_status_t error)
 {
+	struct skd_request_context *skreq = blk_mq_rq_to_pdu(req);
+
 	if (unlikely(error)) {
 		char *cmd = (rq_data_dir(req) == READ) ? "read" : "write";
 		u32 lba = (u32)blk_rq_pos(req);
@@ -621,19 +625,15 @@ static void skd_end_request(struct skd_device *skdev, struct request *req,
 		dev_dbg(&skdev->pdev->dev, "id=0x%x error=%d\n", req->tag,
 			error);
 
-	blk_mq_end_request(req, error);
+	skreq->status = error;
+	blk_mq_complete_request(req);
 }
 
-/* Only called in case of a request timeout */
 static void skd_softirq_done(struct request *req)
 {
-	struct skd_device *skdev = req->q->queuedata;
 	struct skd_request_context *skreq = blk_mq_rq_to_pdu(req);
-	unsigned long flags;
 
-	spin_lock_irqsave(&skdev->lock, flags);
-	skd_end_request(skdev, blk_mq_rq_from_pdu(skreq), BLK_STS_TIMEOUT);
-	spin_unlock_irqrestore(&skdev->lock, flags);
+	blk_mq_end_request(req, skreq->status);
 }
 
 static bool skd_preop_sg_list(struct skd_device *skdev,
@@ -2821,6 +2821,8 @@ static int skd_cons_sksb(struct skd_device *skdev)
 
 static const struct blk_mq_ops skd_mq_ops = {
 	.queue_rq	= skd_mq_queue_rq,
+	.complete	= skd_softirq_done,
+	.timeout	= skd_timed_out,
 	.init_request	= skd_init_request,
 	.exit_request	= skd_exit_request,
 };
@@ -2884,8 +2886,6 @@ static int skd_cons_disk(struct skd_device *skdev)
 	queue_flag_clear_unlocked(QUEUE_FLAG_ADD_RANDOM, q);
 
 	blk_queue_rq_timeout(q, 8 * HZ);
-	blk_queue_rq_timed_out(q, skd_timed_out);
-	blk_queue_softirq_done(q, skd_softirq_done);
 
 	spin_lock_irqsave(&skdev->lock, flags);
 	dev_dbg(&skdev->pdev->dev, "stopping queue\n");
-- 
2.14.0

Re: [PATCH 4/5] skd: Avoid double completions in case of a timeout

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Although as a follow on I would also move the debug printks
from skd_end_request to skd_softirq_done, then remove skd_end_request
and rename skd_softirq_done to skd_complete_rq to fit what other
drivers are doing a little more closely.

Re: [PATCH 4/5] skd: Avoid double completions in case of a timeout

[Bart Van Assche]

T24gV2VkLCAyMDE3LTA4LTIzIGF0IDIwOjA4ICswMjAwLCBDaHJpc3RvcGggSGVsbHdpZyB3cm90
ZToNCj4gQWx0aG91Z2ggYXMgYSBmb2xsb3cgb24gSSB3b3VsZCBhbHNvIG1vdmUgdGhlIGRlYnVn
IHByaW50a3MNCj4gZnJvbSBza2RfZW5kX3JlcXVlc3QgdG8gc2tkX3NvZnRpcnFfZG9uZSwgdGhl
biByZW1vdmUgc2tkX2VuZF9yZXF1ZXN0DQo+IGFuZCByZW5hbWUgc2tkX3NvZnRpcnFfZG9uZSB0
byBza2RfY29tcGxldGVfcnEgdG8gZml0IHdoYXQgb3RoZXINCj4gZHJpdmVycyBhcmUgZG9pbmcg
YSBsaXR0bGUgbW9yZSBjbG9zZWx5Lg0KDQpIZWxsbyBDaHJpc3RvcGgsDQoNClRoYW5rcyBmb3Ig
dGhlIHJldmlldyBvZiB0aGlzIHBhdGNoIHNlcmllcy4gSSB3aWxsIGluY2x1ZGUgdGhlIGFib3Zl
IGNoYW5nZXMNCndoZW4gSSBwb3N0IG15IG5leHQgcGF0Y2ggc2VyaWVzIGZvciB0aGUgc2tkIGRy
aXZlci4NCg0KQmFydC4=

[PATCH 5/5] skd: Change default interrupt mode to MSI-X

[Bart Van Assche]

Since MSI support on some motherboards is unreliable, change the
default interrupt mode from MSI to MSI-X. This patch avoids that
the following message appears sporadially in the kernel logs of
my test setup:

do_IRQ: 3.193 No irq handler for vector

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/block/skd_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
index ff288f1a5dec..577618c57975 100644
--- a/drivers/block/skd_main.c
+++ b/drivers/block/skd_main.c
@@ -310,7 +310,7 @@ static inline void skd_reg_write64(struct skd_device *skdev, u64 val,
 }
 
 
-#define SKD_IRQ_DEFAULT SKD_IRQ_MSI
+#define SKD_IRQ_DEFAULT SKD_IRQ_MSIX
 static int skd_isr_type = SKD_IRQ_DEFAULT;
 
 module_param(skd_isr_type, int, 0444);
-- 
2.14.0

Re: [PATCH 5/5] skd: Change default interrupt mode to MSI-X

[Christoph Hellwig]

On Wed, Aug 23, 2017 at 10:56:33AM -0700, Bart Van Assche wrote:
> Since MSI support on some motherboards is unreliable, change the
> default interrupt mode from MSI to MSI-X. This patch avoids that
> the following message appears sporadially in the kernel logs of
> my test setup:

It might be worth noting that the driver still supports MSI
and INTX interrupts in this mode, which makes it match what most
other drivers do.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 0/5] skd fixes for kernel v4.14

[Jens Axboe]

On 08/23/2017 11:56 AM, Bart Van Assche wrote:
> Hello Jens,
> 
> Code review resulted in five more patches for the skd (sTec s1120) kernel
> driver. These patches have been tested on top of the for-next branch from this
> morning of your block layer git repository. That means that Dan Carpenter's
> patches were included in the kernel tree used for testing. Please consider
> these patches for kernel v4.14.

Looks fine to me, added. Thanks Blart.

-- 
Jens Axboe