[PATCH 0/4] Four more patches for the sTec s1120 driver (skd)

[PATCH 0/4] Four more patches for the sTec s1120 driver (skd)

[Bart Van Assche]

Hello Jens,

The previous series of patches for the sTec s1120 driver was followed by two
requests from Christoph to reorganize the code slightly and one report from
Dan Carpenter with a (false positive) Smatch report. This patch series
addresses that feedback and includes a fourth patch I came up with
myself. Please consider these patches for kernel v4.14.

Thanks,

Bart.

Bart Van Assche (4):
  skd: Rename skd_softirq_done() into skd_complete_rq()
  skd: Inline skd_end_request()
  skd: Make it easier for static analyzers to analyze skd_free_disk()
  skd: Remove SKD_ID_INCR

 drivers/block/skd_main.c | 57 +++++++++++++++---------------------------------
 1 file changed, 17 insertions(+), 40 deletions(-)

-- 
2.14.1

[PATCH 1/4] skd: Rename skd_softirq_done() into skd_complete_rq()

[Bart Van Assche]

The latter name follows more closely the function names used in
other blk-mq drivers.

Suggested-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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
index 577618c57975..a55c8ef1a21d 100644
--- a/drivers/block/skd_main.c
+++ b/drivers/block/skd_main.c
@@ -629,7 +629,7 @@ static void skd_end_request(struct skd_device *skdev, struct request *req,
 	blk_mq_complete_request(req);
 }
 
-static void skd_softirq_done(struct request *req)
+static void skd_complete_rq(struct request *req)
 {
 	struct skd_request_context *skreq = blk_mq_rq_to_pdu(req);
 
@@ -2821,7 +2821,7 @@ 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,
+	.complete	= skd_complete_rq,
 	.timeout	= skd_timed_out,
 	.init_request	= skd_init_request,
 	.exit_request	= skd_exit_request,
-- 
2.14.1

[PATCH 2/4] skd: Inline skd_end_request()

[Bart Van Assche]

It is not worth to keep the debug statements in skd_end_request().
Without debug statements that function only consists of two
statements. Hence inline skd_end_request().

Suggested-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 | 45 +++++++++++++--------------------------------
 1 file changed, 13 insertions(+), 32 deletions(-)

diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
index a55c8ef1a21d..8ae0320f02b5 100644
--- a/drivers/block/skd_main.c
+++ b/drivers/block/skd_main.c
@@ -360,8 +360,6 @@ static void skd_send_fitmsg(struct skd_device *skdev,
 			    struct skd_fitmsg_context *skmsg);
 static void skd_send_special_fitmsg(struct skd_device *skdev,
 				    struct skd_special_context *skspcl);
-static void skd_end_request(struct skd_device *skdev, struct request *req,
-			    blk_status_t status);
 static bool skd_preop_sg_list(struct skd_device *skdev,
 			     struct skd_request_context *skreq);
 static void skd_postop_sg_list(struct skd_device *skdev,
@@ -520,8 +518,8 @@ static blk_status_t skd_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
 
 	if (req->bio && !skd_preop_sg_list(skdev, skreq)) {
 		dev_dbg(&skdev->pdev->dev, "error Out\n");
-		skd_end_request(skdev, blk_mq_rq_from_pdu(skreq),
-				BLK_STS_RESOURCE);
+		skreq->status = BLK_STS_RESOURCE;
+		blk_mq_complete_request(req);
 		return BLK_STS_OK;
 	}
 
@@ -608,27 +606,6 @@ static enum blk_eh_timer_return skd_timed_out(struct request *req,
 	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);
-		u32 count = blk_rq_sectors(req);
-
-		dev_err(&skdev->pdev->dev,
-			"Error cmd=%s sect=%u count=%u id=0x%x\n", cmd, lba,
-			count, req->tag);
-	} else
-		dev_dbg(&skdev->pdev->dev, "id=0x%x error=%d\n", req->tag,
-			error);
-
-	skreq->status = error;
-	blk_mq_complete_request(req);
-}
-
 static void skd_complete_rq(struct request *req)
 {
 	struct skd_request_context *skreq = blk_mq_rq_to_pdu(req);
@@ -1438,7 +1415,8 @@ static void skd_resolve_req_exception(struct skd_device *skdev,
 	switch (skd_check_status(skdev, cmp_status, &skreq->err_info)) {
 	case SKD_CHECK_STATUS_REPORT_GOOD:
 	case SKD_CHECK_STATUS_REPORT_SMART_ALERT:
-		skd_end_request(skdev, req, BLK_STS_OK);
+		skreq->status = BLK_STS_OK;
+		blk_mq_complete_request(req);
 		break;
 
 	case SKD_CHECK_STATUS_BUSY_IMMINENT:
@@ -1460,7 +1438,8 @@ static void skd_resolve_req_exception(struct skd_device *skdev,
 
 	case SKD_CHECK_STATUS_REPORT_ERROR:
 	default:
-		skd_end_request(skdev, req, BLK_STS_IOERR);
+		skreq->status = BLK_STS_IOERR;
+		blk_mq_complete_request(req);
 		break;
 	}
 }
@@ -1579,10 +1558,12 @@ static int skd_isr_completion_posted(struct skd_device *skdev,
 		/*
 		 * Capture the outcome and post it back to the native request.
 		 */
-		if (likely(cmp_status == SAM_STAT_GOOD))
-			skd_end_request(skdev, rq, BLK_STS_OK);
-		else
+		if (likely(cmp_status == SAM_STAT_GOOD)) {
+			skreq->status = BLK_STS_OK;
+			blk_mq_complete_request(rq);
+		} else {
 			skd_resolve_req_exception(skdev, skreq, rq);
+		}
 
 		/* skd_isr_comp_limit equal zero means no limit */
 		if (limit) {
@@ -1926,8 +1907,8 @@ static void skd_recover_request(struct request *req, void *data, bool reserved)
 		skd_postop_sg_list(skdev, skreq);
 
 	skreq->state = SKD_REQ_STATE_IDLE;
-
-	skd_end_request(skdev, req, BLK_STS_IOERR);
+	skreq->status = BLK_STS_IOERR;
+	blk_mq_complete_request(req);
 }
 
 static void skd_recover_requests(struct skd_device *skdev)
-- 
2.14.1

[PATCH 3/4] skd: Make it easier for static analyzers to analyze skd_free_disk()

[Bart Van Assche]

Although it is easy to see that skdev->disk != NULL if skdev->queue
!= NULL, add a test for skdev->disk to avoid that smatch reports the
following warning:

drivers/block/skd_main.c:3080 skd_free_disk()
         error: we previously assumed 'disk' could be null (see line 3074)

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

diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
index 8ae0320f02b5..34188a600bfa 100644
--- a/drivers/block/skd_main.c
+++ b/drivers/block/skd_main.c
@@ -3041,7 +3041,8 @@ static void skd_free_disk(struct skd_device *skdev)
 	if (skdev->queue) {
 		blk_cleanup_queue(skdev->queue);
 		skdev->queue = NULL;
-		disk->queue = NULL;
+		if (disk)
+			disk->queue = NULL;
 	}
 
 	if (skdev->tag_set.tags)
-- 
2.14.1

[PATCH 4/4] skd: Remove SKD_ID_INCR

[Bart Van Assche]

The SKD_ID_INCR flag in skd_request_context.id duplicates information
that is already available otherwise, e.g. through the block layer
request state and through skd_request_context.state. Hence remove
the code that manipulates this flag and also the flag itself.
Since skd_isr_completion_posted() only uses the lower bits of
skd_request_context.id as hardware tag, this patch does not change
the behavior of the skd driver. I'm referring to the following code:

    tag = req_id & SKD_ID_SLOT_AND_TABLE_MASK;

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 | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
index 34188a600bfa..00a86252b3c5 100644
--- a/drivers/block/skd_main.c
+++ b/drivers/block/skd_main.c
@@ -89,7 +89,6 @@ MODULE_DESCRIPTION("STEC s1120 PCIe SSD block driver");
 	  sizeof(struct fit_comp_error_info)) * SKD_N_COMPLETION_ENTRY)
 
 /* 5 bits of uniqifier, 0xF800 */
-#define SKD_ID_INCR             (0x400)
 #define SKD_ID_TABLE_MASK       (3u << 8u)
 #define  SKD_ID_RW_REQUEST      (0u << 8u)
 #define  SKD_ID_INTERNAL        (1u << 8u)
@@ -921,9 +920,7 @@ static void skd_send_internal_skspcl(struct skd_device *skdev,
 		 */
 		return;
 
-	SKD_ASSERT((skspcl->req.id & SKD_ID_INCR) == 0);
 	skspcl->req.state = SKD_REQ_STATE_BUSY;
-	skspcl->req.id += SKD_ID_INCR;
 
 	scsi = &skspcl->msg_buf->scsi[0];
 	scsi->hdr.tag = skspcl->req.id;
@@ -1044,7 +1041,6 @@ static void skd_complete_internal(struct skd_device *skdev,
 
 	skspcl->req.completion = *skcomp;
 	skspcl->req.state = SKD_REQ_STATE_IDLE;
-	skspcl->req.id += SKD_ID_INCR;
 
 	status = skspcl->req.completion.status;
 
@@ -1451,7 +1447,6 @@ static void skd_release_skreq(struct skd_device *skdev,
 	 * Reclaim the skd_request_context
 	 */
 	skreq->state = SKD_REQ_STATE_IDLE;
-	skreq->id += SKD_ID_INCR;
 }
 
 static int skd_isr_completion_posted(struct skd_device *skdev,
-- 
2.14.1

Re: [PATCH 0/4] Four more patches for the sTec s1120 driver (skd)

[Jens Axboe]

On 08/25/2017 03:24 PM, Bart Van Assche wrote:
> Hello Jens,
> 
> The previous series of patches for the sTec s1120 driver was followed by two
> requests from Christoph to reorganize the code slightly and one report from
> Dan Carpenter with a (false positive) Smatch report. This patch series
> addresses that feedback and includes a fourth patch I came up with
> myself. Please consider these patches for kernel v4.14.

LGTM, applied for 4.14, thanks.

-- 
Jens Axboe

Re: [PATCH 3/4] skd: Make it easier for static analyzers to analyze skd_free_disk()

[Dan Carpenter]

No, no.  That's not required.  I've been thinking about this false
positive and I have a plan to fix Smatch to parse this code correctly.
Potentially it will take a year or so to get around to it but I'm sure
it will get done.

regards,
dan carpenter