Skip to contentSkip to conversation

two more s390 bug fixes for 4.17

GIT PULL Martin Schwidefsky 3 participants 7 messages May 30, 2018 – Jun 8, 2018

Subsystems (2)
s390 s390 DASD driver
lore
Share & tools
lore

Martin Schwidefsky

author
#0 lore raw

Hi Linus,

please pull from the 'for-linus' branch of

git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git for-linus

to receive the following updates:

Two bug fixes for 4.17

* A missing -msoft-float for the compile of the kexec purgatory

* A fix for the dasd driver to avoid the double use of a field in the 'struct request'

Philipp Rudo (1):
s390/purgatory: Fix endless interrupt loop

Sebastian Ott (1):
s390/dasd: use blk_mq_rq_from_pdu for per request data

 arch/s390/purgatory/Makefile | 2 +-
 drivers/s390/block/dasd.c    | 7 +++++--
 2 files changed, 6 insertions(+), 3 deletions(-)
arch/s390/purgatory/Makefile +1 -1
index e9525bc..1ace023 100644
@@ -21,7 +21,7 @@ LDFLAGS_purgatory.ro += -z nodefaultlib
 KBUILD_CFLAGS := -fno-strict-aliasing -Wall -Wstrict-prototypes
 KBUILD_CFLAGS += -Wno-pointer-sign -Wno-sign-compare
 KBUILD_CFLAGS += -fno-zero-initialized-in-bss -fno-builtin -ffreestanding
-KBUILD_CFLAGS += -c -MD -Os -m64
+KBUILD_CFLAGS += -c -MD -Os -m64 -msoft-float
 KBUILD_CFLAGS += $(call cc-option,-fno-PIE)
 
 $(obj)/purgatory.ro: $(PURGATORY_OBJS) FORCE
drivers/s390/block/dasd.c +5 -2
index 04143c0..02c03e4 100644
@@ -3034,7 +3034,8 @@ static blk_status_t do_dasd_request(struct blk_mq_hw_ctx *hctx,
 	cqr->callback_data = req;
 	cqr->status = DASD_CQR_FILLED;
 	cqr->dq = dq;
-	req->completion_data = cqr;
+	*((struct dasd_ccw_req **) blk_mq_rq_to_pdu(req)) = cqr;
+
 	blk_mq_start_request(req);
 	spin_lock(&block->queue_lock);
 	list_add_tail(&cqr->blocklist, &block->ccw_queue);

@@ -3058,12 +3059,13 @@ static blk_status_t do_dasd_request(struct blk_mq_hw_ctx *hctx,
  */
 enum blk_eh_timer_return dasd_times_out(struct request *req, bool reserved)
 {
-	struct dasd_ccw_req *cqr = req->completion_data;
 	struct dasd_block *block = req->q->queuedata;
 	struct dasd_device *device;
+	struct dasd_ccw_req *cqr;
 	unsigned long flags;
 	int rc = 0;
 
+	cqr = *((struct dasd_ccw_req **) blk_mq_rq_to_pdu(req));
 	if (!cqr)
 		return BLK_EH_NOT_HANDLED;
 

@@ -3169,6 +3171,7 @@ static int dasd_alloc_queue(struct dasd_block *block)
 	int rc;
 
 	block->tag_set.ops = &dasd_mq_ops;
+	block->tag_set.cmd_size = sizeof(struct dasd_ccw_req *);
 	block->tag_set.nr_hw_queues = DASD_NR_HW_QUEUES;
 	block->tag_set.queue_depth = DASD_MAX_LCU_DEV * DASD_REQ_PER_DEV;
 	block->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
#1 lore raw

drivers/s390/block/dasd.c static blk_status_t do_dasd_request(struct blk_mq_hw_ctx *hctx, · line 3038

Quoted 3 lines
drivers/s390/block/dasd.c +2 -1
-	req->completion_data = cqr;
+	*((struct dasd_ccw_req **) blk_mq_rq_to_pdu(req)) = cqr;
+

Please don't play such tricks. In general your driver structure should have struct request embedded. If for some reason struct dasd_ccw_req has a different life time please create a new structure instead of these hacks.

In either way this really doesn't look like a post-rc7 change.

author
#2 lore raw
Quoted 8 lines · On Wed, 30 May 2018 00:41:30 -0700 Christoph Hellwig <[email protected]> wrote:

drivers/s390/block/dasd.c static blk_status_t do_dasd_request(struct blk_mq_hw_ctx *hctx, · line 3038

Quoted 3 lines
drivers/s390/block/dasd.c +2 -1
-	req->completion_data = cqr;
+	*((struct dasd_ccw_req **) blk_mq_rq_to_pdu(req)) = cqr;
+

Please don't play such tricks. In general your driver structure should have struct request embedded. If for some reason struct dasd_ccw_req has a different life time please create a new structure instead of these hacks.

Why do you consider this to be a 'trick'? The blk_mq_rq_to_pdu is meant to be used to access a block of data that is is associated with a request, no? With the change we store a single value, the pointer to a struct dasd_ccw_req. The struct request comes first, later do_dasd_request creates the struct dasd_ccw_req with the CCW chain for the request. And for the blk timeout handler we need a way to find the dasd_ccw_req again.

Quoted 1 line

In either way this really doesn't look like a post-rc7 change.

You think so? It fixes a crash for s390 with a minimal number of changed lines:

 drivers/s390/block/dasd.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Signature · 4 lines
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.
#3 lore raw
Quoted 12 lines · Martin Schwidefsky
Quoted 8 lines · On Wed, 30 May 2018 00:41:30 -0700 Christoph Hellwig <[email protected]> wrote:

drivers/s390/block/dasd.c static blk_status_t do_dasd_request(struct blk_mq_hw_ctx *hctx, · line 3038

Quoted 3 lines
drivers/s390/block/dasd.c +2 -1
-	req->completion_data = cqr;
+	*((struct dasd_ccw_req **) blk_mq_rq_to_pdu(req)) = cqr;
+

Please don't play such tricks. In general your driver structure should have struct request embedded. If for some reason struct dasd_ccw_req has a different life time please create a new structure instead of these hacks.

Quoted 7 lines

Why do you consider this to be a 'trick'? The blk_mq_rq_to_pdu is meant to be used to access a block of data that is is associated with a request, no? With the change we store a single value, the pointer to a struct dasd_ccw_req. The struct request comes first, later do_dasd_request creates the struct dasd_ccw_req with the CCW chain for the request. And for the blk timeout handler we need a way to find the dasd_ccw_req again.

We'll we try really hard to have a structure that we can use container_of on. At least a minimal container with just the pointer for a quick fix, but in general it seems like you should be able to allocate the whole dasd_ccw_req with the request and just initialize it later.

author
#4 lore raw
Quoted 21 lines · On Thu, 31 May 2018 09:14:00 -0700 Christoph Hellwig <[email protected]> wrote:
Quoted 12 lines · Martin Schwidefsky
Quoted 8 lines · On Wed, 30 May 2018 00:41:30 -0700 Christoph Hellwig <[email protected]> wrote:

drivers/s390/block/dasd.c static blk_status_t do_dasd_request(struct blk_mq_hw_ctx *hctx, · line 3038

Quoted 3 lines
drivers/s390/block/dasd.c +2 -1
-	req->completion_data = cqr;
+	*((struct dasd_ccw_req **) blk_mq_rq_to_pdu(req)) = cqr;
+

Please don't play such tricks. In general your driver structure should have struct request embedded. If for some reason struct dasd_ccw_req has a different life time please create a new structure instead of these hacks.

Quoted 7 lines

Why do you consider this to be a 'trick'? The blk_mq_rq_to_pdu is meant to be used to access a block of data that is is associated with a request, no? With the change we store a single value, the pointer to a struct dasd_ccw_req. The struct request comes first, later do_dasd_request creates the struct dasd_ccw_req with the CCW chain for the request. And for the blk timeout handler we need a way to find the dasd_ccw_req again.

Quoted 5 lines

We'll we try really hard to have a structure that we can use container_of on. At least a minimal container with just the pointer for a quick fix, but in general it seems like you should be able to allocate the whole dasd_ccw_req with the request and just initialize it later.

A minimal container with a pointer is easy enough to do. To replace our allocation scheme for the CCW chains will be hard. For one there are internal I/Os that do not have an associated 'struct request'. For those we need to allocate the memory on our own anyway. Then the size of the memory block you need for the CCW varies wildly and can be quite large. The tag cmd_size is fixed at the time blk_mq_alloc_tag_set is called. If we replace out allocation scheme this would force us to use the maximum size CCW chain for the tag size, no?

Signature · 4 lines
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.
#5 lore raw
In reply to Christoph Hellwig
Quoted 8 lines · Christoph Hellwig

drivers/s390/block/dasd.c static blk_status_t do_dasd_request(struct blk_mq_hw_ctx *hctx, · line 3038

Quoted 3 lines
drivers/s390/block/dasd.c +2 -1
-	req->completion_data = cqr;
+	*((struct dasd_ccw_req **) blk_mq_rq_to_pdu(req)) = cqr;
+

Please don't play such tricks. In general your driver structure should have struct request embedded. If for some reason struct dasd_ccw_req has a different life time please create a new structure instead of these hacks.

Would you agree that this is an improvement over using completion_data (at least now we don't mess with the block layers internal data)?

I already looked into what needs to be done to put struct dasd_ccw_req behind the request and let it be managed by the block layer.

My concerns are lifetime and memory constraints (some of our HW interfaces still use 31 bit pointers) but I think both of these are manageable with the blk-mq interfaces.

But this would not be a small change and nothing for rc7. Would you be OK with doing the small fix now and I'll work on the mentioned improvement?

Regards,
Sebastian

Reviewed-by Stefan Haberland
#6 lore raw

Hello,

Quoted 7 lines · Sebastian Ott
Quoted 6 lines · Christoph Hellwig

Review

Quoted 3 lines
(quoted diff) +2 -1
-	req->completion_data = cqr;
+	*((struct dasd_ccw_req **) blk_mq_rq_to_pdu(req)) = cqr;
+

Please don't play such tricks. In general your driver structure should have struct request embedded.

OK. I did that. Executive summary:

@@ -3077,7 +3078,7 @@ enum blk_eh_timer_return dasd_times_out(struct request *req, bool reserved)

        unsigned long flags;
        int rc = 0;

- cqr = *((struct dasd_ccw_req **) blk_mq_rq_to_pdu(req));

+       cqr = blk_mq_rq_to_pdu(req);
        if (!cqr)
                return BLK_EH_NOT_HANDLED;

@@ -3179,7 +3180,7 @@ static int dasd_alloc_queue(struct dasd_block *block)

        int rc;
 
        block->tag_set.ops = &dasd_mq_ops;

- block->tag_set.cmd_size = sizeof(struct dasd_ccw_req *);
+ block->tag_set.cmd_size = sizeof(struct dasd_ccw_req);

If you agree with this I'll prepare that one for inclusion via Martins tree.

Regards,
Sebastian

------->8

From 72470aaf8721e3cff9aac9f1095c22c66164eb7f Mon Sep 17 00:00:00 2001
From: Sebastian Ott <[email protected]>
Date: Mon, 4 Jun 2018 19:07:39 +0200
Subject: [PATCH] s390/dasd: move dasd_ccw_req to per request data

Let the block layer allocate per request data to store struct dasd_ccw_req. We still need extra preallocated memory for usage by ccw programs (which vary in length) and for requests which don't originate from the block layer.

Link https://lkml.kernel.org/r/[email protected] open here
Signed-off-by Sebastian Ott <[email protected]>
Reviewed-by Stefan Haberland <[email protected]>
---
 drivers/s390/block/dasd.c      | 46 ++++++++++++++++++++++--------------------
 drivers/s390/block/dasd_diag.c |  3 ++-
 drivers/s390/block/dasd_eckd.c | 46 +++++++++++++++++++++++-------------------
 drivers/s390/block/dasd_fba.c  |  6 ++++--
 drivers/s390/block/dasd_int.h  |  3 ++-
 5 files changed, 57 insertions(+), 47 deletions(-)
drivers/s390/block/dasd.c +24 -22
index ed039ab..a8ffbac 100644
@@ -1267,35 +1267,37 @@ struct dasd_ccw_req *dasd_kmalloc_request(int magic, int cplength,
 }
 EXPORT_SYMBOL(dasd_kmalloc_request);
 
-struct dasd_ccw_req *dasd_smalloc_request(int magic, int cplength,
-					  int datasize,
-					  struct dasd_device *device)
+struct dasd_ccw_req *dasd_smalloc_request(int magic, int cplength, int datasize,
+					  struct dasd_device *device,
+					  struct dasd_ccw_req *cqr)
 {
 	unsigned long flags;
-	struct dasd_ccw_req *cqr;
-	char *data;
-	int size;
+	char *data, *chunk;
+	int size = 0;
 
-	size = (sizeof(struct dasd_ccw_req) + 7L) & -8L;
 	if (cplength > 0)
 		size += cplength * sizeof(struct ccw1);
 	if (datasize > 0)
 		size += datasize;
+	if (!cqr)
+		size += (sizeof(*cqr) + 7L) & -8L;
+
 	spin_lock_irqsave(&device->mem_lock, flags);
-	cqr = (struct dasd_ccw_req *)
-		dasd_alloc_chunk(&device->ccw_chunks, size);
+	data = chunk = dasd_alloc_chunk(&device->ccw_chunks, size);
 	spin_unlock_irqrestore(&device->mem_lock, flags);
-	if (cqr == NULL)
+	if (!chunk)
 		return ERR_PTR(-ENOMEM);
-	memset(cqr, 0, sizeof(struct dasd_ccw_req));
-	data = (char *) cqr + ((sizeof(struct dasd_ccw_req) + 7L) & -8L);
-	cqr->cpaddr = NULL;
+	if (!cqr) {
+		cqr = (void *) data;
+		data += (sizeof(*cqr) + 7L) & -8L;
+	}
+	memset(cqr, 0, sizeof(*cqr));
+	cqr->mem_chunk = chunk;
 	if (cplength > 0) {
-		cqr->cpaddr = (struct ccw1 *) data;
-		data += cplength*sizeof(struct ccw1);
-		memset(cqr->cpaddr, 0, cplength*sizeof(struct ccw1));
+		cqr->cpaddr = data;
+		data += cplength * sizeof(struct ccw1);
+		memset(cqr->cpaddr, 0, cplength * sizeof(struct ccw1));
 	}
-	cqr->data = NULL;
 	if (datasize > 0) {
 		cqr->data = data;
  		memset(cqr->data, 0, datasize);

@@ -1333,7 +1335,7 @@ void dasd_sfree_request(struct dasd_ccw_req *cqr, struct dasd_device *device)
 	unsigned long flags;
 
 	spin_lock_irqsave(&device->mem_lock, flags);
-	dasd_free_chunk(&device->ccw_chunks, cqr);
+	dasd_free_chunk(&device->ccw_chunks, cqr->mem_chunk);
 	spin_unlock_irqrestore(&device->mem_lock, flags);
 	dasd_put_device(device);
 }

@@ -3046,7 +3048,6 @@ static blk_status_t do_dasd_request(struct blk_mq_hw_ctx *hctx,
 	cqr->callback_data = req;
 	cqr->status = DASD_CQR_FILLED;
 	cqr->dq = dq;
-	*((struct dasd_ccw_req **) blk_mq_rq_to_pdu(req)) = cqr;
 
 	blk_mq_start_request(req);
 	spin_lock(&block->queue_lock);

@@ -3077,7 +3078,7 @@ enum blk_eh_timer_return dasd_times_out(struct request *req, bool reserved)
 	unsigned long flags;
 	int rc = 0;
 
-	cqr = *((struct dasd_ccw_req **) blk_mq_rq_to_pdu(req));
+	cqr = blk_mq_rq_to_pdu(req);
 	if (!cqr)
 		return BLK_EH_NOT_HANDLED;
 

@@ -3179,7 +3180,7 @@ static int dasd_alloc_queue(struct dasd_block *block)
 	int rc;
 
 	block->tag_set.ops = &dasd_mq_ops;
-	block->tag_set.cmd_size = sizeof(struct dasd_ccw_req *);
+	block->tag_set.cmd_size = sizeof(struct dasd_ccw_req);
 	block->tag_set.nr_hw_queues = DASD_NR_HW_QUEUES;
 	block->tag_set.queue_depth = DASD_MAX_LCU_DEV * DASD_REQ_PER_DEV;
 	block->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;

@@ -4043,7 +4044,8 @@ static struct dasd_ccw_req *dasd_generic_build_rdc(struct dasd_device *device,
 	struct ccw1 *ccw;
 	unsigned long *idaw;
 
-	cqr = dasd_smalloc_request(magic, 1 /* RDC */, rdc_buffer_size, device);
+	cqr = dasd_smalloc_request(magic, 1 /* RDC */, rdc_buffer_size, device,
+				   NULL);
 
 	if (IS_ERR(cqr)) {
 		/* internal error 13 - Allocating the RDC request failed*/
drivers/s390/block/dasd_diag.c +2 -1
index 131f198..e1fe024 100644
@@ -536,7 +536,8 @@ static struct dasd_ccw_req *dasd_diag_build_cp(struct dasd_device *memdev,
 	/* Build the request */
 	datasize = sizeof(struct dasd_diag_req) +
 		count*sizeof(struct dasd_diag_bio);
-	cqr = dasd_smalloc_request(DASD_DIAG_MAGIC, 0, datasize, memdev);
+	cqr = dasd_smalloc_request(DASD_DIAG_MAGIC, 0, datasize, memdev,
+				   blk_mq_rq_to_pdu(req));
 	if (IS_ERR(cqr))
 		return cqr;
drivers/s390/block/dasd_eckd.c +25 -21

166 lines not on this page · raw message

drivers/s390/block/dasd_fba.c +4 -2
index a6b132f..56007a3 100644
@@ -356,7 +356,8 @@ static struct dasd_ccw_req *dasd_fba_build_cp_discard(
 	datasize = sizeof(struct DE_fba_data) +
 		nr_ccws * (sizeof(struct LO_fba_data) + sizeof(struct ccw1));
 
-	cqr = dasd_smalloc_request(DASD_FBA_MAGIC, cplength, datasize, memdev);
+	cqr = dasd_smalloc_request(DASD_FBA_MAGIC, cplength, datasize, memdev,
+				   blk_mq_rq_to_pdu(req));
 	if (IS_ERR(cqr))
 		return cqr;
 

@@ -490,7 +491,8 @@ static struct dasd_ccw_req *dasd_fba_build_cp_regular(
 		datasize += (count - 1)*sizeof(struct LO_fba_data);
 	}
 	/* Allocate the ccw request. */
-	cqr = dasd_smalloc_request(DASD_FBA_MAGIC, cplength, datasize, memdev);
+	cqr = dasd_smalloc_request(DASD_FBA_MAGIC, cplength, datasize, memdev,
+				   blk_mq_rq_to_pdu(req));
 	if (IS_ERR(cqr))
 		return cqr;
 	ccw = cqr->cpaddr;
drivers/s390/block/dasd_int.h +2 -1
index 96709b1..0844e5e 100644
@@ -184,6 +184,7 @@ struct dasd_ccw_req {
 	struct irb irb;			/* device status in case of an error */
 	struct dasd_ccw_req *refers;	/* ERP-chain queueing. */
 	void *function; 		/* originating ERP action */
+	void *mem_chunk;
 
 	/* these are for statistics only */
 	unsigned long buildclk;		/* TOD-clock of request generation */

@@ -716,7 +717,7 @@ extern struct kmem_cache *dasd_page_cache;
 struct dasd_ccw_req *
 dasd_kmalloc_request(int , int, int, struct dasd_device *);
 struct dasd_ccw_req *
-dasd_smalloc_request(int , int, int, struct dasd_device *);
+dasd_smalloc_request(int, int, int, struct dasd_device *, struct dasd_ccw_req *);
 void dasd_kfree_request(struct dasd_ccw_req *, struct dasd_device *);
 void dasd_sfree_request(struct dasd_ccw_req *, struct dasd_device *);
 void dasd_wakeup_cb(struct dasd_ccw_req *, void *);
Signature · 1 line
2.7.4