[PATCH 0/6] More MMC block core refactorings

[PATCH 0/6] More MMC block core refactorings

[Linus Walleij]

This series builds on top of the previous series that created
custom DRV_OP requests for ioctl() operations in MMC.

The first patch is a suggestion from Christoph, the second
builds infrastructure for issuing more, currently orthogonal
custom operations through the block layer.

The first operation we move over is pretty uncontroversial
and straight-forward: it is the operation that
write-protect-locks the boot partitions from sysfs. This
is now done through the block layer so we do not need
to congest and starve in the big MMC lock.

The last two patches are more contoversial: they move the
two debugfs accesses for reading card status and EXT CSD
over to using the block layer funnel *if* *present*.

So if the block layer is configured out, these will still
issue operations directly and take the big MMC lock.

The patch series is fully ABI safe: any scripts or code
using the debugfs with or without the block layer will
still work.

However this leaves the mmc_card_get() locks in the block.h
header for the !CONFIG_MMC_BLOCK case and I'm not really happy
to keep them around, the idea is to terminate them.

Ways forward after these patches:

- Simply remove the debugfs files for status and ext_csd if
  the block layer is not there. The debugfs is not ABI after
  all, and there is an ioctl() to do the same job, and
  that is what mmc-utils is using.

- Simply remove the debugfs files for status and ext_csd
  completely - and require users to switch to using the
  ioctl() mmc-utils way of doing things if they want to
  inspect their MMC/SD cards.

- Wait and see: when I get to removing the big MMC lock from
  SDIO I will anyway have to deal with this mess since
  the big lock is no more a block layer problem, but a
  problem with the entire MMC/SD/SDIO stack.

In any case: these patches fixes the starvation of the
boot partition locking and the debugfs access when using
the block layer heavily at the same time.

Linus Walleij (6):
  mmc: block: remove req back pointer
  mmc: block: Tag DRV_OPs with a driver operation type
  mmc: block: Move DRV OP issue function
  mmc: block: Move boot partition locking into a driver op
  mmc: debugfs: Move card status retrieveal into the block layer
  mmc: debugfs: Move EXT CSD debugfs acces to block layer

 drivers/mmc/core/block.c   | 168 ++++++++++++++++++++++++++++++++-------------
 drivers/mmc/core/block.h   |  49 +++++++++++++
 drivers/mmc/core/debugfs.c |  19 +----
 drivers/mmc/core/queue.c   |  13 ++--
 drivers/mmc/core/queue.h   |  27 +++++++-
 5 files changed, 200 insertions(+), 76 deletions(-)

-- 
2.9.3

[PATCH 1/6] mmc: block: remove req back pointer

[Linus Walleij]

Just as we can use blk_mq_rq_from_pdu() to get the per-request
tag we can use blk_mq_rq_to_pdu() to get a request from a tag.
Introduce a static inline helper so we are on the clear what
is happening.

Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/block.c |  8 ++++----
 drivers/mmc/core/queue.c | 13 +++++--------
 drivers/mmc/core/queue.h |  8 +++++++-
 3 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index e9737987956f..553ab4d1db94 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1366,7 +1366,7 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
 	struct mmc_queue_req *mq_mrq = container_of(areq, struct mmc_queue_req,
 						    areq);
 	struct mmc_blk_request *brq = &mq_mrq->brq;
-	struct request *req = mq_mrq->req;
+	struct request *req = mmc_queue_req_to_req(mq_mrq);
 	int need_retune = card->host->need_retune;
 	bool ecc_err = false;
 	bool gen_err = false;
@@ -1473,7 +1473,7 @@ static void mmc_blk_data_prep(struct mmc_queue *mq, struct mmc_queue_req *mqrq,
 	struct mmc_blk_data *md = mq->blkdata;
 	struct mmc_card *card = md->queue.card;
 	struct mmc_blk_request *brq = &mqrq->brq;
-	struct request *req = mqrq->req;
+	struct request *req = mmc_queue_req_to_req(mqrq);
 
 	/*
 	 * Reliable writes are used to implement Forced Unit Access and
@@ -1578,7 +1578,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
 {
 	u32 readcmd, writecmd;
 	struct mmc_blk_request *brq = &mqrq->brq;
-	struct request *req = mqrq->req;
+	struct request *req = mmc_queue_req_to_req(mqrq);
 	struct mmc_blk_data *md = mq->blkdata;
 	bool do_rel_wr, do_data_tag;
 
@@ -1760,7 +1760,7 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
 		 */
 		mq_rq =	container_of(old_areq, struct mmc_queue_req, areq);
 		brq = &mq_rq->brq;
-		old_req = mq_rq->req;
+		old_req = mmc_queue_req_to_req(mq_rq);
 		type = rq_data_dir(old_req) == READ ? MMC_BLK_READ : MMC_BLK_WRITE;
 		mmc_queue_bounce_post(mq_rq);
 
diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index c18c41289ecf..4bf9978b707a 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -184,8 +184,6 @@ static int mmc_init_request(struct request_queue *q, struct request *req,
 	struct mmc_card *card = mq->card;
 	struct mmc_host *host = card->host;
 
-	mq_rq->req = req;
-
 	if (card->bouncesz) {
 		mq_rq->bounce_buf = kmalloc(card->bouncesz, gfp);
 		if (!mq_rq->bounce_buf)
@@ -223,8 +221,6 @@ static void mmc_exit_request(struct request_queue *q, struct request *req)
 
 	kfree(mq_rq->sg);
 	mq_rq->sg = NULL;
-
-	mq_rq->req = NULL;
 }
 
 /**
@@ -374,12 +370,13 @@ unsigned int mmc_queue_map_sg(struct mmc_queue *mq, struct mmc_queue_req *mqrq)
 	unsigned int sg_len;
 	size_t buflen;
 	struct scatterlist *sg;
+	struct request *req = mmc_queue_req_to_req(mqrq);
 	int i;
 
 	if (!mqrq->bounce_buf)
-		return blk_rq_map_sg(mq->queue, mqrq->req, mqrq->sg);
+		return blk_rq_map_sg(mq->queue, req, mqrq->sg);
 
-	sg_len = blk_rq_map_sg(mq->queue, mqrq->req, mqrq->bounce_sg);
+	sg_len = blk_rq_map_sg(mq->queue, req, mqrq->bounce_sg);
 
 	mqrq->bounce_sg_len = sg_len;
 
@@ -401,7 +398,7 @@ void mmc_queue_bounce_pre(struct mmc_queue_req *mqrq)
 	if (!mqrq->bounce_buf)
 		return;
 
-	if (rq_data_dir(mqrq->req) != WRITE)
+	if (rq_data_dir(mmc_queue_req_to_req(mqrq)) != WRITE)
 		return;
 
 	sg_copy_to_buffer(mqrq->bounce_sg, mqrq->bounce_sg_len,
@@ -417,7 +414,7 @@ void mmc_queue_bounce_post(struct mmc_queue_req *mqrq)
 	if (!mqrq->bounce_buf)
 		return;
 
-	if (rq_data_dir(mqrq->req) != READ)
+	if (rq_data_dir(mmc_queue_req_to_req(mqrq)) != READ)
 		return;
 
 	sg_copy_from_buffer(mqrq->bounce_sg, mqrq->bounce_sg_len,
diff --git a/drivers/mmc/core/queue.h b/drivers/mmc/core/queue.h
index dfe481a8b5ed..2793020a3c8c 100644
--- a/drivers/mmc/core/queue.h
+++ b/drivers/mmc/core/queue.h
@@ -12,6 +12,13 @@ static inline struct mmc_queue_req *req_to_mmc_queue_req(struct request *rq)
 	return blk_mq_rq_to_pdu(rq);
 }
 
+struct mmc_queue_req;
+
+static inline struct request *mmc_queue_req_to_req(struct mmc_queue_req *mqr)
+{
+	return blk_mq_rq_from_pdu(mqr);
+}
+
 struct task_struct;
 struct mmc_blk_data;
 struct mmc_blk_ioc_data;
@@ -26,7 +33,6 @@ struct mmc_blk_request {
 };
 
 struct mmc_queue_req {
-	struct request		*req;
 	struct mmc_blk_request	brq;
 	struct scatterlist	*sg;
 	char			*bounce_buf;
-- 
2.9.3

[PATCH 2/6] mmc: block: Tag DRV_OPs with a driver operation type

[Linus Walleij]

We will expand the DRV_OP usage, so we need to know which
operation we're performing. Tag the operations with an
enum:ed type and rename the function so it is clear that
it deals with any command and put a switch statement in
it. Currently only ioctls are supported.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/block.c | 37 ++++++++++++++++++++++++-------------
 drivers/mmc/core/queue.h |  9 +++++++++
 2 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 553ab4d1db94..b24e7f5171c9 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -602,6 +602,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 		idata->ic.write_flag ? REQ_OP_DRV_OUT : REQ_OP_DRV_IN,
 		__GFP_RECLAIM);
 	idatas[0] = idata;
+	req_to_mmc_queue_req(req)->drv_op = MMC_DRV_OP_IOCTL;
 	req_to_mmc_queue_req(req)->idata = idatas;
 	req_to_mmc_queue_req(req)->ioc_count = 1;
 	blk_execute_rq(mq->queue, NULL, req, 0);
@@ -618,11 +619,11 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 }
 
 /*
- * The ioctl commands come back from the block layer after it queued it and
+ * The non-block commands come back from the block layer after it queued it and
  * processed it with all other requests and then they get issued in this
  * function.
  */
-static void mmc_blk_ioctl_cmd_issue(struct mmc_queue *mq, struct request *req)
+static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)
 {
 	struct mmc_queue_req *mq_rq;
 	struct mmc_card *card = mq->card;
@@ -631,18 +632,27 @@ static void mmc_blk_ioctl_cmd_issue(struct mmc_queue *mq, struct request *req)
 	int i;
 
 	mq_rq = req_to_mmc_queue_req(req);
-	for (i = 0; i < mq_rq->ioc_count; i++) {
-		ioc_err = __mmc_blk_ioctl_cmd(card, md, mq_rq->idata[i]);
-		if (ioc_err)
-			break;
-	}
-	mq_rq->ioc_result = ioc_err;
 
-	/* Always switch back to main area after RPMB access */
-	if (md->area_type & MMC_BLK_DATA_AREA_RPMB)
-		mmc_blk_part_switch(card, dev_get_drvdata(&card->dev));
+	switch (mq_rq->drv_op) {
+	case MMC_DRV_OP_IOCTL:
+		for (i = 0; i < mq_rq->ioc_count; i++) {
+			ioc_err =
+				__mmc_blk_ioctl_cmd(card, md, mq_rq->idata[i]);
+			if (ioc_err)
+				break;
+		}
+		mq_rq->ioc_result = ioc_err;
+
+		/* Always switch back to main area after RPMB access */
+		if (md->area_type & MMC_BLK_DATA_AREA_RPMB)
+			mmc_blk_part_switch(card, dev_get_drvdata(&card->dev));
 
-	blk_end_request_all(req, ioc_err);
+		blk_end_request_all(req, ioc_err);
+		break;
+	default:
+		/* Unknown operation */
+		break;
+	}
 }
 
 static int mmc_blk_ioctl_multi_cmd(struct block_device *bdev,
@@ -705,6 +715,7 @@ static int mmc_blk_ioctl_multi_cmd(struct block_device *bdev,
 	req = blk_get_request(mq->queue,
 		idata[0]->ic.write_flag ? REQ_OP_DRV_OUT : REQ_OP_DRV_IN,
 		__GFP_RECLAIM);
+	req_to_mmc_queue_req(req)->drv_op = MMC_DRV_OP_IOCTL;
 	req_to_mmc_queue_req(req)->idata = idata;
 	req_to_mmc_queue_req(req)->ioc_count = num_of_cmds;
 	blk_execute_rq(mq->queue, NULL, req, 0);
@@ -1904,7 +1915,7 @@ void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
 			 */
 			if (mq->qcnt)
 				mmc_blk_issue_rw_rq(mq, NULL);
-			mmc_blk_ioctl_cmd_issue(mq, req);
+			mmc_blk_issue_drv_op(mq, req);
 			break;
 		case REQ_OP_DISCARD:
 			/*
diff --git a/drivers/mmc/core/queue.h b/drivers/mmc/core/queue.h
index 2793020a3c8c..1e6062eb3e07 100644
--- a/drivers/mmc/core/queue.h
+++ b/drivers/mmc/core/queue.h
@@ -32,6 +32,14 @@ struct mmc_blk_request {
 	int			retune_retry_done;
 };
 
+/**
+ * enum mmc_drv_op - enumerates the operations in the mmc_queue_req
+ * @MMC_DRV_OP_IOCTL: ioctl operation
+ */
+enum mmc_drv_op {
+	MMC_DRV_OP_IOCTL,
+};
+
 struct mmc_queue_req {
 	struct mmc_blk_request	brq;
 	struct scatterlist	*sg;
@@ -39,6 +47,7 @@ struct mmc_queue_req {
 	struct scatterlist	*bounce_sg;
 	unsigned int		bounce_sg_len;
 	struct mmc_async_req	areq;
+	enum mmc_drv_op		drv_op;
 	int			ioc_result;
 	struct mmc_blk_ioc_data	**idata;
 	unsigned int		ioc_count;
-- 
2.9.3

[PATCH 3/6] mmc: block: Move DRV OP issue function

[Linus Walleij]

We will need to access static functions above the pure block layer
operations in the file, so move the driver operations issue
function down so we can see all non-blocklayer symbols.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/block.c | 74 ++++++++++++++++++++++++------------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index b24e7f5171c9..75b1baacf28b 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -618,43 +618,6 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 	return ioc_err ? ioc_err : err;
 }
 
-/*
- * The non-block commands come back from the block layer after it queued it and
- * processed it with all other requests and then they get issued in this
- * function.
- */
-static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)
-{
-	struct mmc_queue_req *mq_rq;
-	struct mmc_card *card = mq->card;
-	struct mmc_blk_data *md = mq->blkdata;
-	int ioc_err;
-	int i;
-
-	mq_rq = req_to_mmc_queue_req(req);
-
-	switch (mq_rq->drv_op) {
-	case MMC_DRV_OP_IOCTL:
-		for (i = 0; i < mq_rq->ioc_count; i++) {
-			ioc_err =
-				__mmc_blk_ioctl_cmd(card, md, mq_rq->idata[i]);
-			if (ioc_err)
-				break;
-		}
-		mq_rq->ioc_result = ioc_err;
-
-		/* Always switch back to main area after RPMB access */
-		if (md->area_type & MMC_BLK_DATA_AREA_RPMB)
-			mmc_blk_part_switch(card, dev_get_drvdata(&card->dev));
-
-		blk_end_request_all(req, ioc_err);
-		break;
-	default:
-		/* Unknown operation */
-		break;
-	}
-}
-
 static int mmc_blk_ioctl_multi_cmd(struct block_device *bdev,
 				   struct mmc_ioc_multi_cmd __user *user)
 {
@@ -1222,6 +1185,43 @@ int mmc_access_rpmb(struct mmc_queue *mq)
 	return false;
 }
 
+/*
+ * The non-block commands come back from the block layer after it queued it and
+ * processed it with all other requests and then they get issued in this
+ * function.
+ */
+static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)
+{
+	struct mmc_queue_req *mq_rq;
+	struct mmc_card *card = mq->card;
+	struct mmc_blk_data *md = mq->blkdata;
+	int ioc_err;
+	int i;
+
+	mq_rq = req_to_mmc_queue_req(req);
+
+	switch (mq_rq->drv_op) {
+	case MMC_DRV_OP_IOCTL:
+		for (i = 0; i < mq_rq->ioc_count; i++) {
+			ioc_err =
+				__mmc_blk_ioctl_cmd(card, md, mq_rq->idata[i]);
+			if (ioc_err)
+				break;
+		}
+		mq_rq->ioc_result = ioc_err;
+
+		/* Always switch back to main area after RPMB access */
+		if (md->area_type & MMC_BLK_DATA_AREA_RPMB)
+			mmc_blk_part_switch(card, dev_get_drvdata(&card->dev));
+
+		blk_end_request_all(req, ioc_err);
+		break;
+	default:
+		/* Unknown operation */
+		break;
+	}
+}
+
 static void mmc_blk_issue_discard_rq(struct mmc_queue *mq, struct request *req)
 {
 	struct mmc_blk_data *md = mq->blkdata;
-- 
2.9.3

[PATCH 4/6] mmc: block: Move boot partition locking into a driver op

[Linus Walleij]

This moves the boot partition lock command (issued from sysfs)
into a custom block layer request, just like the ioctl()s,
getting rid of yet another instance of mmc_get_card().

Since we now have two operations issuing special DRV_OP's, we
rename the result variable ->drv_op_result.

Tested by locking the boot partition from userspace:
> cd /sys/devices/platform/soc/80114000.sdi4_per2/mmc_host/mmc3/
     mmc3:0001/block/mmcblk3/mmcblk3boot0
> echo 1 > ro_lock_until_next_power_on
[  178.645324] mmcblk3boot1: Locking boot partition ro until next power on
[  178.652221] mmcblk3boot0: Locking boot partition ro until next power on

Also tested this with a huge dd job in the background: it
is now possible to lock the boot partitions on the card even
under heavy I/O.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/block.c | 53 +++++++++++++++++++++++++++---------------------
 drivers/mmc/core/queue.h |  4 +++-
 2 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 75b1baacf28b..52635120a0a5 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -190,6 +190,8 @@ static ssize_t power_ro_lock_store(struct device *dev,
 	int ret;
 	struct mmc_blk_data *md, *part_md;
 	struct mmc_card *card;
+	struct mmc_queue *mq;
+	struct request *req;
 	unsigned long set;
 
 	if (kstrtoul(buf, 0, &set))
@@ -199,20 +201,14 @@ static ssize_t power_ro_lock_store(struct device *dev,
 		return count;
 
 	md = mmc_blk_get(dev_to_disk(dev));
+	mq = &md->queue;
 	card = md->queue.card;
 
-	mmc_get_card(card);
-
-	ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BOOT_WP,
-				card->ext_csd.boot_ro_lock |
-				EXT_CSD_BOOT_WP_B_PWR_WP_EN,
-				card->ext_csd.part_time);
-	if (ret)
-		pr_err("%s: Locking boot partition ro until next power on failed: %d\n", md->disk->disk_name, ret);
-	else
-		card->ext_csd.boot_ro_lock |= EXT_CSD_BOOT_WP_B_PWR_WP_EN;
-
-	mmc_put_card(card);
+	/* Dispatch locking to the block layer */
+	req = blk_get_request(mq->queue, REQ_OP_DRV_OUT, __GFP_RECLAIM);
+	req_to_mmc_queue_req(req)->drv_op = MMC_DRV_OP_BOOT_WP;
+	blk_execute_rq(mq->queue, NULL, req, 0);
+	ret = req_to_mmc_queue_req(req)->drv_op_result;
 
 	if (!ret) {
 		pr_info("%s: Locking boot partition ro until next power on\n",
@@ -606,7 +602,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 	req_to_mmc_queue_req(req)->idata = idatas;
 	req_to_mmc_queue_req(req)->ioc_count = 1;
 	blk_execute_rq(mq->queue, NULL, req, 0);
-	ioc_err = req_to_mmc_queue_req(req)->ioc_result;
+	ioc_err = req_to_mmc_queue_req(req)->drv_op_result;
 	err = mmc_blk_ioctl_copy_to_user(ic_ptr, idata);
 	blk_put_request(req);
 
@@ -682,7 +678,7 @@ static int mmc_blk_ioctl_multi_cmd(struct block_device *bdev,
 	req_to_mmc_queue_req(req)->idata = idata;
 	req_to_mmc_queue_req(req)->ioc_count = num_of_cmds;
 	blk_execute_rq(mq->queue, NULL, req, 0);
-	ioc_err = req_to_mmc_queue_req(req)->ioc_result;
+	ioc_err = req_to_mmc_queue_req(req)->drv_op_result;
 
 	/* copy to user if data and response */
 	for (i = 0; i < num_of_cmds && !err; i++)
@@ -1195,7 +1191,7 @@ static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)
 	struct mmc_queue_req *mq_rq;
 	struct mmc_card *card = mq->card;
 	struct mmc_blk_data *md = mq->blkdata;
-	int ioc_err;
+	int ret;
 	int i;
 
 	mq_rq = req_to_mmc_queue_req(req);
@@ -1203,23 +1199,34 @@ static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)
 	switch (mq_rq->drv_op) {
 	case MMC_DRV_OP_IOCTL:
 		for (i = 0; i < mq_rq->ioc_count; i++) {
-			ioc_err =
-				__mmc_blk_ioctl_cmd(card, md, mq_rq->idata[i]);
-			if (ioc_err)
+			ret = __mmc_blk_ioctl_cmd(card, md, mq_rq->idata[i]);
+			if (ret)
 				break;
 		}
-		mq_rq->ioc_result = ioc_err;
-
 		/* Always switch back to main area after RPMB access */
 		if (md->area_type & MMC_BLK_DATA_AREA_RPMB)
 			mmc_blk_part_switch(card, dev_get_drvdata(&card->dev));
-
-		blk_end_request_all(req, ioc_err);
+		break;
+	case MMC_DRV_OP_BOOT_WP:
+		ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BOOT_WP,
+				 card->ext_csd.boot_ro_lock |
+				 EXT_CSD_BOOT_WP_B_PWR_WP_EN,
+				 card->ext_csd.part_time);
+		if (ret)
+			pr_err("%s: Locking boot partition ro until next power on failed: %d\n",
+			       md->disk->disk_name, ret);
+		else
+			card->ext_csd.boot_ro_lock |=
+				EXT_CSD_BOOT_WP_B_PWR_WP_EN;
 		break;
 	default:
-		/* Unknown operation */
+		pr_err("%s: unknown driver specific operation\n",
+		       md->disk->disk_name);
+		ret = -EINVAL;
 		break;
 	}
+	mq_rq->drv_op_result = ret;
+	blk_end_request_all(req, ret);
 }
 
 static void mmc_blk_issue_discard_rq(struct mmc_queue *mq, struct request *req)
diff --git a/drivers/mmc/core/queue.h b/drivers/mmc/core/queue.h
index 1e6062eb3e07..361b46408e0f 100644
--- a/drivers/mmc/core/queue.h
+++ b/drivers/mmc/core/queue.h
@@ -35,9 +35,11 @@ struct mmc_blk_request {
 /**
  * enum mmc_drv_op - enumerates the operations in the mmc_queue_req
  * @MMC_DRV_OP_IOCTL: ioctl operation
+ * @MMC_DRV_OP_BOOT_WP: write protect boot partitions
  */
 enum mmc_drv_op {
 	MMC_DRV_OP_IOCTL,
+	MMC_DRV_OP_BOOT_WP,
 };
 
 struct mmc_queue_req {
@@ -48,7 +50,7 @@ struct mmc_queue_req {
 	unsigned int		bounce_sg_len;
 	struct mmc_async_req	areq;
 	enum mmc_drv_op		drv_op;
-	int			ioc_result;
+	int			drv_op_result;
 	struct mmc_blk_ioc_data	**idata;
 	unsigned int		ioc_count;
 };
-- 
2.9.3

[PATCH 5/6] mmc: debugfs: Move card status retrieveal into the block layer

[Linus Walleij]

The debugfs file "status" (in e.g. /debug/mmc3/mmc3:0001) is
only available if and only if the card used is an (e)MMC or
SD card, not for SDIO, as can be seen from this guard in
mmc_add_card_debugfs();

if (mmc_card_mmc(card) || mmc_card_sd(card))
  (...create debugfs "status" entry...)

Further this debugfs entry suffers from all the same starvation
issues as the other userspace things, under e.g. a heavy
dd operation.

It is therefore logical to move this over to the block layer
when it is enabled, using the new custom requests and issue
it using the block request queue.

This makes this debugfs card access land under the request
queue host lock instead of orthogonally taking the lock.

Tested during heavy dd load by cat:in the status file. We
add IS_ENABLED() guards and keep the code snippet just
issueing the card status as a static inline in the header
so we can still have card status working when the block
layer is compiled out.

Keeping two copies of mmc_dbg_card_status_get() around
seems to be a necessary evil to be able to have the MMC/SD
stack working with the block layer disabled: under these
circumstances, the code must simply take another path.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/block.c   | 28 ++++++++++++++++++++++++++++
 drivers/mmc/core/block.h   | 37 +++++++++++++++++++++++++++++++++++++
 drivers/mmc/core/debugfs.c | 15 ++-------------
 drivers/mmc/core/queue.h   |  2 ++
 4 files changed, 69 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 52635120a0a5..8858798d1349 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1191,6 +1191,7 @@ static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)
 	struct mmc_queue_req *mq_rq;
 	struct mmc_card *card = mq->card;
 	struct mmc_blk_data *md = mq->blkdata;
+	u32 status;
 	int ret;
 	int i;
 
@@ -1219,6 +1220,11 @@ static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)
 			card->ext_csd.boot_ro_lock |=
 				EXT_CSD_BOOT_WP_B_PWR_WP_EN;
 		break;
+	case MMC_DRV_OP_GET_CARD_STATUS:
+		ret = mmc_send_status(card, &status);
+		if (!ret)
+			ret = status;
+		break;
 	default:
 		pr_err("%s: unknown driver specific operation\n",
 		       md->disk->disk_name);
@@ -1968,6 +1974,28 @@ void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
 		mmc_put_card(card);
 }
 
+/* Called from debugfs for MMC/SD cards */
+int mmc_blk_card_status_get(struct mmc_card *card, u64 *val)
+{
+	struct mmc_blk_data *md = dev_get_drvdata(&card->dev);
+	struct mmc_queue *mq = &md->queue;
+	struct request *req;
+	int ret;
+
+	/* Ask the block layer about the card status */
+	req = blk_get_request(mq->queue, REQ_OP_DRV_IN, __GFP_RECLAIM);
+	req_to_mmc_queue_req(req)->drv_op = MMC_DRV_OP_GET_CARD_STATUS;
+	blk_execute_rq(mq->queue, NULL, req, 0);
+	ret = req_to_mmc_queue_req(req)->drv_op_result;
+	if (ret >= 0) {
+		*val = ret;
+		ret = 0;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(mmc_blk_card_status_get);
+
 static inline int mmc_blk_readonly(struct mmc_card *card)
 {
 	return mmc_card_readonly(card) ||
diff --git a/drivers/mmc/core/block.h b/drivers/mmc/core/block.h
index 860ca7c8df86..1e26755a864b 100644
--- a/drivers/mmc/core/block.h
+++ b/drivers/mmc/core/block.h
@@ -1,9 +1,46 @@
 #ifndef _MMC_CORE_BLOCK_H
 #define _MMC_CORE_BLOCK_H
 
+#include <linux/kconfig.h>
+#include "core.h"
+#include "mmc_ops.h"
+
 struct mmc_queue;
 struct request;
 
+#if IS_ENABLED(CONFIG_MMC_BLOCK)
+
 void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req);
+int mmc_blk_card_status_get(struct mmc_card *card, u64 *val);
+
+#else
+
+/*
+ * Small stub functions to be used when the block layer is not
+ * enabled, e.g. for pure SDIO without MMC/SD configurations.
+ */
+
+static inline void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
+{
+	return;
+}
+
+static inline int mmc_blk_card_status_get(struct mmc_card *card, u64 *val)
+{
+	u32 status;
+	int ret;
+
+	mmc_get_card(card);
+
+	ret = mmc_send_status(card, &status);
+	if (!ret)
+		*val = status;
+
+	mmc_put_card(card);
+
+	return ret;
+}
+
+#endif /* IS_ENABLED(CONFIG_MMC_BLOCK) */
 
 #endif
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index a1fba5732d66..ce5b921c7d96 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -19,6 +19,7 @@
 #include <linux/mmc/card.h>
 #include <linux/mmc/host.h>
 
+#include "block.h"
 #include "core.h"
 #include "card.h"
 #include "host.h"
@@ -283,19 +284,7 @@ void mmc_remove_host_debugfs(struct mmc_host *host)
 
 static int mmc_dbg_card_status_get(void *data, u64 *val)
 {
-	struct mmc_card	*card = data;
-	u32		status;
-	int		ret;
-
-	mmc_get_card(card);
-
-	ret = mmc_send_status(data, &status);
-	if (!ret)
-		*val = status;
-
-	mmc_put_card(card);
-
-	return ret;
+	return mmc_blk_card_status_get(data, val);
 }
 DEFINE_SIMPLE_ATTRIBUTE(mmc_dbg_card_status_fops, mmc_dbg_card_status_get,
 		NULL, "%08llx\n");
diff --git a/drivers/mmc/core/queue.h b/drivers/mmc/core/queue.h
index 361b46408e0f..2b39717453a5 100644
--- a/drivers/mmc/core/queue.h
+++ b/drivers/mmc/core/queue.h
@@ -36,10 +36,12 @@ struct mmc_blk_request {
  * enum mmc_drv_op - enumerates the operations in the mmc_queue_req
  * @MMC_DRV_OP_IOCTL: ioctl operation
  * @MMC_DRV_OP_BOOT_WP: write protect boot partitions
+ * @MMC_DRV_OP_GET_CARD_STATUS: get card status
  */
 enum mmc_drv_op {
 	MMC_DRV_OP_IOCTL,
 	MMC_DRV_OP_BOOT_WP,
+	MMC_DRV_OP_GET_CARD_STATUS,
 };
 
 struct mmc_queue_req {
-- 
2.9.3

[PATCH 6/6] mmc: debugfs: Move EXT CSD debugfs acces to block layer

[Linus Walleij]

Just like the previous commit moving status retriveal for
MMC and SD cards into the block layer (when active), this
moves the retrieveal of the EXT CSD from the card from
the special ext_csd file into the block stack as well.

Again special care is taken to make the debugfs work even
with the block layer disabled. Again this solves a
starvation issue during heavy block workloads.

It has been tested with and without the block layer and
during heavy load from dd.

Since we can't keep adding weirdo data pointers into
struct mmc_queue_req this converts the
struct mmc_blk_ioc_data **idata pointer to a simple
void *drv_op_data that gets casted into whatever data
the driver-specific command needs to pass, and then I
cast it to the right target type in the sending and
receiving functions.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/block.c   | 30 +++++++++++++++++++++++++++---
 drivers/mmc/core/block.h   | 12 ++++++++++++
 drivers/mmc/core/debugfs.c |  4 +---
 drivers/mmc/core/queue.h   |  4 +++-
 4 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 8858798d1349..5be7f06d4ecd 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -599,7 +599,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 		__GFP_RECLAIM);
 	idatas[0] = idata;
 	req_to_mmc_queue_req(req)->drv_op = MMC_DRV_OP_IOCTL;
-	req_to_mmc_queue_req(req)->idata = idatas;
+	req_to_mmc_queue_req(req)->drv_op_data = idatas;
 	req_to_mmc_queue_req(req)->ioc_count = 1;
 	blk_execute_rq(mq->queue, NULL, req, 0);
 	ioc_err = req_to_mmc_queue_req(req)->drv_op_result;
@@ -675,7 +675,7 @@ static int mmc_blk_ioctl_multi_cmd(struct block_device *bdev,
 		idata[0]->ic.write_flag ? REQ_OP_DRV_OUT : REQ_OP_DRV_IN,
 		__GFP_RECLAIM);
 	req_to_mmc_queue_req(req)->drv_op = MMC_DRV_OP_IOCTL;
-	req_to_mmc_queue_req(req)->idata = idata;
+	req_to_mmc_queue_req(req)->drv_op_data = idata;
 	req_to_mmc_queue_req(req)->ioc_count = num_of_cmds;
 	blk_execute_rq(mq->queue, NULL, req, 0);
 	ioc_err = req_to_mmc_queue_req(req)->drv_op_result;
@@ -1191,6 +1191,8 @@ static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)
 	struct mmc_queue_req *mq_rq;
 	struct mmc_card *card = mq->card;
 	struct mmc_blk_data *md = mq->blkdata;
+	struct mmc_blk_ioc_data	**idata;
+	u8 **ext_csd;
 	u32 status;
 	int ret;
 	int i;
@@ -1199,8 +1201,9 @@ static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)
 
 	switch (mq_rq->drv_op) {
 	case MMC_DRV_OP_IOCTL:
+		idata = mq_rq->drv_op_data;
 		for (i = 0; i < mq_rq->ioc_count; i++) {
-			ret = __mmc_blk_ioctl_cmd(card, md, mq_rq->idata[i]);
+			ret = __mmc_blk_ioctl_cmd(card, md, idata[i]);
 			if (ret)
 				break;
 		}
@@ -1225,6 +1228,10 @@ static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)
 		if (!ret)
 			ret = status;
 		break;
+	case MMC_DRV_OP_GET_EXT_CSD:
+		ext_csd = mq_rq->drv_op_data;
+		ret = mmc_get_ext_csd(card, ext_csd);
+		break;
 	default:
 		pr_err("%s: unknown driver specific operation\n",
 		       md->disk->disk_name);
@@ -1996,6 +2003,23 @@ int mmc_blk_card_status_get(struct mmc_card *card, u64 *val)
 }
 EXPORT_SYMBOL(mmc_blk_card_status_get);
 
+
+/* Called from debugfs for MMC cards */
+int mmc_blk_get_ext_csd(struct mmc_card *card, u8 **ext_csd)
+{
+	struct mmc_blk_data *md = dev_get_drvdata(&card->dev);
+	struct mmc_queue *mq = &md->queue;
+	struct request *req;
+
+	/* Ask the block layer about the EXT CSD */
+	req = blk_get_request(mq->queue, REQ_OP_DRV_IN, __GFP_RECLAIM);
+	req_to_mmc_queue_req(req)->drv_op = MMC_DRV_OP_GET_EXT_CSD;
+	req_to_mmc_queue_req(req)->drv_op_data = ext_csd;
+	blk_execute_rq(mq->queue, NULL, req, 0);
+	return req_to_mmc_queue_req(req)->drv_op_result;
+}
+EXPORT_SYMBOL(mmc_blk_get_ext_csd);
+
 static inline int mmc_blk_readonly(struct mmc_card *card)
 {
 	return mmc_card_readonly(card) ||
diff --git a/drivers/mmc/core/block.h b/drivers/mmc/core/block.h
index 1e26755a864b..c85c3b71dcad 100644
--- a/drivers/mmc/core/block.h
+++ b/drivers/mmc/core/block.h
@@ -12,6 +12,7 @@ struct request;
 
 void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req);
 int mmc_blk_card_status_get(struct mmc_card *card, u64 *val);
+int mmc_blk_get_ext_csd(struct mmc_card *card, u8 **ext_csd);
 
 #else
 
@@ -41,6 +42,17 @@ static inline int mmc_blk_card_status_get(struct mmc_card *card, u64 *val)
 	return ret;
 }
 
+static inline int mmc_blk_get_ext_csd(struct mmc_card *card, u8 **ext_csd)
+{
+	int ret;
+
+	mmc_get_card(card);
+	ret = mmc_get_ext_csd(card, ext_csd);
+	mmc_put_card(card);
+
+	return ret;
+}
+
 #endif /* IS_ENABLED(CONFIG_MMC_BLOCK) */
 
 #endif
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index ce5b921c7d96..85e058120e3b 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -303,9 +303,7 @@ static int mmc_ext_csd_open(struct inode *inode, struct file *filp)
 	if (!buf)
 		return -ENOMEM;
 
-	mmc_get_card(card);
-	err = mmc_get_ext_csd(card, &ext_csd);
-	mmc_put_card(card);
+	err = mmc_blk_get_ext_csd(card, &ext_csd);
 	if (err)
 		goto out_free;
 
diff --git a/drivers/mmc/core/queue.h b/drivers/mmc/core/queue.h
index 2b39717453a5..2f67856661da 100644
--- a/drivers/mmc/core/queue.h
+++ b/drivers/mmc/core/queue.h
@@ -37,11 +37,13 @@ struct mmc_blk_request {
  * @MMC_DRV_OP_IOCTL: ioctl operation
  * @MMC_DRV_OP_BOOT_WP: write protect boot partitions
  * @MMC_DRV_OP_GET_CARD_STATUS: get card status
+ * @MMC_DRV_OP_GET_EXT_CSD: get extended card descriptor
  */
 enum mmc_drv_op {
 	MMC_DRV_OP_IOCTL,
 	MMC_DRV_OP_BOOT_WP,
 	MMC_DRV_OP_GET_CARD_STATUS,
+	MMC_DRV_OP_GET_EXT_CSD,
 };
 
 struct mmc_queue_req {
@@ -53,7 +55,7 @@ struct mmc_queue_req {
 	struct mmc_async_req	areq;
 	enum mmc_drv_op		drv_op;
 	int			drv_op_result;
-	struct mmc_blk_ioc_data	**idata;
+	void			*drv_op_data;
 	unsigned int		ioc_count;
 };
 
-- 
2.9.3

Re: [PATCH 5/6] mmc: debugfs: Move card status retrieveal into the block layer

[Ulf Hansson]

On 19 May 2017 at 15:37, Linus Walleij <linus.walleij@linaro.org> wrote:
> The debugfs file "status" (in e.g. /debug/mmc3/mmc3:0001) is
> only available if and only if the card used is an (e)MMC or
> SD card, not for SDIO, as can be seen from this guard in
> mmc_add_card_debugfs();
>
> if (mmc_card_mmc(card) || mmc_card_sd(card))
>   (...create debugfs "status" entry...)
>
> Further this debugfs entry suffers from all the same starvation
> issues as the other userspace things, under e.g. a heavy
> dd operation.
>
> It is therefore logical to move this over to the block layer
> when it is enabled, using the new custom requests and issue
> it using the block request queue.
>
> This makes this debugfs card access land under the request
> queue host lock instead of orthogonally taking the lock.
>
> Tested during heavy dd load by cat:in the status file. We
> add IS_ENABLED() guards and keep the code snippet just
> issueing the card status as a static inline in the header
> so we can still have card status working when the block
> layer is compiled out.

Seems like a bit of re-factoring/cleanup could help here as
preparation step. I just posted a patch [1] cleaning up how the mmc
block layer fetches the card's status.

Perhaps that could at least simplify a bit for $subject patch,
especially since it also makes mmc_send_status() being exported.

>
> Keeping two copies of mmc_dbg_card_status_get() around
> seems to be a necessary evil to be able to have the MMC/SD
> stack working with the block layer disabled: under these
> circumstances, the code must simply take another path.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mmc/core/block.c   | 28 ++++++++++++++++++++++++++++
>  drivers/mmc/core/block.h   | 37 +++++++++++++++++++++++++++++++++++++
>  drivers/mmc/core/debugfs.c | 15 ++-------------
>  drivers/mmc/core/queue.h   |  2 ++
>  4 files changed, 69 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 52635120a0a5..8858798d1349 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -1191,6 +1191,7 @@ static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)
>         struct mmc_queue_req *mq_rq;
>         struct mmc_card *card = mq->card;
>         struct mmc_blk_data *md = mq->blkdata;
> +       u32 status;
>         int ret;
>         int i;
>
> @@ -1219,6 +1220,11 @@ static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)
>                         card->ext_csd.boot_ro_lock |=
>                                 EXT_CSD_BOOT_WP_B_PWR_WP_EN;
>                 break;
> +       case MMC_DRV_OP_GET_CARD_STATUS:
> +               ret = mmc_send_status(card, &status);
> +               if (!ret)
> +                       ret = status;
> +               break;
>         default:
>                 pr_err("%s: unknown driver specific operation\n",
>                        md->disk->disk_name);
> @@ -1968,6 +1974,28 @@ void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
>                 mmc_put_card(card);
>  }
>
> +/* Called from debugfs for MMC/SD cards */
> +int mmc_blk_card_status_get(struct mmc_card *card, u64 *val)
> +{
> +       struct mmc_blk_data *md = dev_get_drvdata(&card->dev);
> +       struct mmc_queue *mq = &md->queue;
> +       struct request *req;
> +       int ret;
> +
> +       /* Ask the block layer about the card status */
> +       req = blk_get_request(mq->queue, REQ_OP_DRV_IN, __GFP_RECLAIM);
> +       req_to_mmc_queue_req(req)->drv_op = MMC_DRV_OP_GET_CARD_STATUS;
> +       blk_execute_rq(mq->queue, NULL, req, 0);
> +       ret = req_to_mmc_queue_req(req)->drv_op_result;
> +       if (ret >= 0) {
> +               *val = ret;
> +               ret = 0;
> +       }
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL(mmc_blk_card_status_get);
> +
>  static inline int mmc_blk_readonly(struct mmc_card *card)
>  {
>         return mmc_card_readonly(card) ||
> diff --git a/drivers/mmc/core/block.h b/drivers/mmc/core/block.h
> index 860ca7c8df86..1e26755a864b 100644
> --- a/drivers/mmc/core/block.h
> +++ b/drivers/mmc/core/block.h
> @@ -1,9 +1,46 @@
>  #ifndef _MMC_CORE_BLOCK_H
>  #define _MMC_CORE_BLOCK_H
>
> +#include <linux/kconfig.h>
> +#include "core.h"
> +#include "mmc_ops.h"
> +
>  struct mmc_queue;
>  struct request;
>
> +#if IS_ENABLED(CONFIG_MMC_BLOCK)

What wrong with a regular ifdefs stubs? Why do you need IS_ENABLED()?

> +
>  void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req);

I don't get what mmc_blk_issue_rq() has to do with this change? Could
you please explain?

> +int mmc_blk_card_status_get(struct mmc_card *card, u64 *val);
> +
> +#else
> +
> +/*
> + * Small stub functions to be used when the block layer is not
> + * enabled, e.g. for pure SDIO without MMC/SD configurations.
> + */
> +
> +static inline void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
> +{
> +       return;
> +}
> +
> +static inline int mmc_blk_card_status_get(struct mmc_card *card, u64 *val)
> +{
> +       u32 status;
> +       int ret;
> +
> +       mmc_get_card(card);
> +
> +       ret = mmc_send_status(card, &status);
> +       if (!ret)
> +               *val = status;
> +
> +       mmc_put_card(card);
> +
> +       return ret;
> +}
> +
> +#endif /* IS_ENABLED(CONFIG_MMC_BLOCK) */
>
>  #endif
> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
> index a1fba5732d66..ce5b921c7d96 100644
> --- a/drivers/mmc/core/debugfs.c
> +++ b/drivers/mmc/core/debugfs.c
> @@ -19,6 +19,7 @@
>  #include <linux/mmc/card.h>
>  #include <linux/mmc/host.h>
>
> +#include "block.h"
>  #include "core.h"
>  #include "card.h"
>  #include "host.h"
> @@ -283,19 +284,7 @@ void mmc_remove_host_debugfs(struct mmc_host *host)
>
>  static int mmc_dbg_card_status_get(void *data, u64 *val)
>  {
> -       struct mmc_card *card = data;
> -       u32             status;
> -       int             ret;
> -
> -       mmc_get_card(card);
> -
> -       ret = mmc_send_status(data, &status);
> -       if (!ret)
> -               *val = status;
> -
> -       mmc_put_card(card);
> -
> -       return ret;
> +       return mmc_blk_card_status_get(data, val);
>  }
>  DEFINE_SIMPLE_ATTRIBUTE(mmc_dbg_card_status_fops, mmc_dbg_card_status_get,
>                 NULL, "%08llx\n");
> diff --git a/drivers/mmc/core/queue.h b/drivers/mmc/core/queue.h
> index 361b46408e0f..2b39717453a5 100644
> --- a/drivers/mmc/core/queue.h
> +++ b/drivers/mmc/core/queue.h
> @@ -36,10 +36,12 @@ struct mmc_blk_request {
>   * enum mmc_drv_op - enumerates the operations in the mmc_queue_req
>   * @MMC_DRV_OP_IOCTL: ioctl operation
>   * @MMC_DRV_OP_BOOT_WP: write protect boot partitions
> + * @MMC_DRV_OP_GET_CARD_STATUS: get card status
>   */
>  enum mmc_drv_op {
>         MMC_DRV_OP_IOCTL,
>         MMC_DRV_OP_BOOT_WP,
> +       MMC_DRV_OP_GET_CARD_STATUS,
>  };
>
>  struct mmc_queue_req {
> --
> 2.9.3
>

Hmm, this thing seems a bit upside-down.

Currently it's possible to build the mmc block device driver as a
module. In cases like this, accessing the card debugs node to request
the card's status, would trigger a call to mmc_blk_card_status_get().
How would that work when the mmc block device driver isn't loaded and
probed?

It seems like the life cycle of the card debugfs node is now being
controlled as when the mmc block device driver has been successfully
probed. We need to deal with that somehow.

Kind regards
Uffe

[1]
https://patchwork.kernel.org/patch/9739663/

Re: [PATCH 0/6] More MMC block core refactorings

[Ulf Hansson]

On 19 May 2017 at 15:37, Linus Walleij <linus.walleij@linaro.org> wrote:
> This series builds on top of the previous series that created
> custom DRV_OP requests for ioctl() operations in MMC.
>
> The first patch is a suggestion from Christoph, the second
> builds infrastructure for issuing more, currently orthogonal
> custom operations through the block layer.
>
> The first operation we move over is pretty uncontroversial
> and straight-forward: it is the operation that
> write-protect-locks the boot partitions from sysfs. This
> is now done through the block layer so we do not need
> to congest and starve in the big MMC lock.
>
> The last two patches are more contoversial: they move the
> two debugfs accesses for reading card status and EXT CSD
> over to using the block layer funnel *if* *present*.
>
> So if the block layer is configured out, these will still
> issue operations directly and take the big MMC lock.
>
> The patch series is fully ABI safe: any scripts or code
> using the debugfs with or without the block layer will
> still work.
>
> However this leaves the mmc_card_get() locks in the block.h
> header for the !CONFIG_MMC_BLOCK case and I'm not really happy
> to keep them around, the idea is to terminate them.
>
> Ways forward after these patches:
>
> - Simply remove the debugfs files for status and ext_csd if
>   the block layer is not there. The debugfs is not ABI after
>   all, and there is an ioctl() to do the same job, and
>   that is what mmc-utils is using.
>
> - Simply remove the debugfs files for status and ext_csd
>   completely - and require users to switch to using the
>   ioctl() mmc-utils way of doing things if they want to
>   inspect their MMC/SD cards.

I like this approach.

mmc-utils is also far better in parsing the results from ext csd and
presents the output in a more human readable format.

Moreover, people that needs to debug the behavior of a card should
likely not have a problem with using mmc-utils.

>
> - Wait and see: when I get to removing the big MMC lock from
>   SDIO I will anyway have to deal with this mess since
>   the big lock is no more a block layer problem, but a
>   problem with the entire MMC/SD/SDIO stack.
>
> In any case: these patches fixes the starvation of the
> boot partition locking and the debugfs access when using
> the block layer heavily at the same time.
>
> Linus Walleij (6):
>   mmc: block: remove req back pointer
>   mmc: block: Tag DRV_OPs with a driver operation type
>   mmc: block: Move DRV OP issue function
>   mmc: block: Move boot partition locking into a driver op
>   mmc: debugfs: Move card status retrieveal into the block layer
>   mmc: debugfs: Move EXT CSD debugfs acces to block layer
>
>  drivers/mmc/core/block.c   | 168 ++++++++++++++++++++++++++++++++-------------
>  drivers/mmc/core/block.h   |  49 +++++++++++++
>  drivers/mmc/core/debugfs.c |  19 +----
>  drivers/mmc/core/queue.c   |  13 ++--
>  drivers/mmc/core/queue.h   |  27 +++++++-
>  5 files changed, 200 insertions(+), 76 deletions(-)
>
> --
> 2.9.3
>

I decided to pick patch 1->4 at this stage as those looks nice and
clean. The rest will need a re-spin.

Thanks and kind regards
Uffe

RE: [PATCH 0/6] More MMC block core refactorings

[Avri Altman]

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Monday, May 22, 2017 3:05 PM
> To: Linus Walleij <linus.walleij@linaro.org>
> Cc: linux-mmc@vger.kernel.org; Adrian Hunter <adrian.hunter@intel.com>;
> linux-block@vger.kernel.org; Jens Axboe <axboe@kernel.dk>; Christoph
> Hellwig <hch@lst.de>; Arnd Bergmann <arnd@arndb.de>; Bartlomiej
> Zolnierkiewicz <b.zolnierkie@samsung.com>; Paolo Valente
> <paolo.valente@linaro.org>; Avri Altman <Avri.Altman@sandisk.com>
> Subject: Re: [PATCH 0/6] More MMC block core refactorings
> 
> On 19 May 2017 at 15:37, Linus Walleij <linus.walleij@linaro.org> wrote:
> > This series builds on top of the previous series that created custom
> > DRV_OP requests for ioctl() operations in MMC.
> >
> > The first patch is a suggestion from Christoph, the second builds
> > infrastructure for issuing more, currently orthogonal custom
> > operations through the block layer.
> >
> > The first operation we move over is pretty uncontroversial and
> > straight-forward: it is the operation that write-protect-locks the
> > boot partitions from sysfs. This is now done through the block layer
> > so we do not need to congest and starve in the big MMC lock.
> >
> > The last two patches are more contoversial: they move the two debugfs
> > accesses for reading card status and EXT CSD over to using the block
> > layer funnel *if* *present*.
> >
> > So if the block layer is configured out, these will still issue
> > operations directly and take the big MMC lock.
> >
> > The patch series is fully ABI safe: any scripts or code using the
> > debugfs with or without the block layer will still work.
> >
> > However this leaves the mmc_card_get() locks in the block.h header for
> > the !CONFIG_MMC_BLOCK case and I'm not really happy to keep them
> > around, the idea is to terminate them.
> >
> > Ways forward after these patches:
> >
> > - Simply remove the debugfs files for status and ext_csd if
> >   the block layer is not there. The debugfs is not ABI after
> >   all, and there is an ioctl() to do the same job, and
> >   that is what mmc-utils is using.
> >
> > - Simply remove the debugfs files for status and ext_csd
> >   completely - and require users to switch to using the
> >   ioctl() mmc-utils way of doing things if they want to
> >   inspect their MMC/SD cards.
> 
> I like this approach.
> 
> mmc-utils is also far better in parsing the results from ext csd and presents
> the output in a more human readable format.
> 
> Moreover, people that needs to debug the behavior of a card should likely
> not have a problem with using mmc-utils.

Those debugfs entries are there for ages, and there are some user-space utilities,
 e.g. for benchmarking etc. that count on it being there, 
 as the key identification criteria if a mmc device is present.


> 
> >
> > - Wait and see: when I get to removing the big MMC lock from
> >   SDIO I will anyway have to deal with this mess since
> >   the big lock is no more a block layer problem, but a
> >   problem with the entire MMC/SD/SDIO stack.
> >
> > In any case: these patches fixes the starvation of the boot partition
> > locking and the debugfs access when using the block layer heavily at
> > the same time.
> >
> > Linus Walleij (6):
> >   mmc: block: remove req back pointer
> >   mmc: block: Tag DRV_OPs with a driver operation type
> >   mmc: block: Move DRV OP issue function
> >   mmc: block: Move boot partition locking into a driver op
> >   mmc: debugfs: Move card status retrieveal into the block layer
> >   mmc: debugfs: Move EXT CSD debugfs acces to block layer
> >
> >  drivers/mmc/core/block.c   | 168
> ++++++++++++++++++++++++++++++++-------------
> >  drivers/mmc/core/block.h   |  49 +++++++++++++
> >  drivers/mmc/core/debugfs.c |  19 +----
> >  drivers/mmc/core/queue.c   |  13 ++--
> >  drivers/mmc/core/queue.h   |  27 +++++++-
> >  5 files changed, 200 insertions(+), 76 deletions(-)
> >
> > --
> > 2.9.3
> >
> 
> I decided to pick patch 1->4 at this stage as those looks nice and clean. The
> rest will need a re-spin.
> 
> Thanks and kind regards
> Uffe

Re: [PATCH 0/6] More MMC block core refactorings

[Christoph Hellwig]

On Mon, May 22, 2017 at 12:44:14PM +0000, Avri Altman wrote:
> Those debugfs entries are there for ages, and there are some user-space utilities,
>  e.g. for benchmarking etc. that count on it being there, 
>  as the key identification criteria if a mmc device is present.

debugfs is an optional feature not present on many systems, and
explicitly not considered a kernel ABI.

Re: [PATCH 5/6] mmc: debugfs: Move card status retrieveal into the block layer

[Linus Walleij]

On Mon, May 22, 2017 at 9:42 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 19 May 2017 at 15:37, Linus Walleij <linus.walleij@linaro.org> wrote:

>> The debugfs file "status" (in e.g. /debug/mmc3/mmc3:0001) is
>> only available if and only if the card used is an (e)MMC or
>> SD card, not for SDIO, as can be seen from this guard in
>> mmc_add_card_debugfs();
>>
>> if (mmc_card_mmc(card) || mmc_card_sd(card))
>>   (...create debugfs "status" entry...)
>>
>> Further this debugfs entry suffers from all the same starvation
>> issues as the other userspace things, under e.g. a heavy
>> dd operation.
>>
>> It is therefore logical to move this over to the block layer
>> when it is enabled, using the new custom requests and issue
>> it using the block request queue.
>>
>> This makes this debugfs card access land under the request
>> queue host lock instead of orthogonally taking the lock.
>>
>> Tested during heavy dd load by cat:in the status file. We
>> add IS_ENABLED() guards and keep the code snippet just
>> issueing the card status as a static inline in the header
>> so we can still have card status working when the block
>> layer is compiled out.
>
> Seems like a bit of re-factoring/cleanup could help here as
> preparation step. I just posted a patch [1] cleaning up how the mmc
> block layer fetches the card's status.
>
> Perhaps that could at least simplify a bit for $subject patch,
> especially since it also makes mmc_send_status() being exported.

OK if you merge your stuff I can iterate my patches on top,
no problem.

>> +#if IS_ENABLED(CONFIG_MMC_BLOCK)
>
> What wrong with a regular ifdefs stubs? Why do you need IS_ENABLED()?

This is because the entire block layer can be compiled as a
module.

In that case CONFIG_MMC_BLOCK contains 'm' instead of 'y'
which confusingly does not evaluate to true in the preprocessor
(it assumes it is a misspelled 'n' I guess).

And then the autobuilders wreak havoc.

And that is why the IS_ENABLED() defines exist in the first
place IIUC.

I'm all for making CONFIG_MMC_BLOCK into a bool... but
don't know how people (Intel laptops) feel about that extra
code in their kernel at all times.

>>  void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req);
>
> I don't get what mmc_blk_issue_rq() has to do with this change? Could
> you please explain?

If we start doing stubs we should stub everything IMO, but if you
prefer I can make that a separate patch. Seems a bit overdone
though.

> Hmm, this thing seems a bit upside-down.
>
> Currently it's possible to build the mmc block device driver as a
> module. In cases like this, accessing the card debugs node to request
> the card's status, would trigger a call to mmc_blk_card_status_get().
> How would that work when the mmc block device driver isn't loaded and
> probed?

Module symbol resolution and driver loading is always necessary,
it is no different for e.g. a wifi driver using the 802.11 library.
It is definately possible to shoot oneself in the foot, but I think
udev & friends usually load things in the right order?

> It seems like the life cycle of the card debugfs node is now being
> controlled as when the mmc block device driver has been successfully
> probed. We need to deal with that somehow.

Only for these two files but yes.

ext_csd is a bit dubious as it is only available on storage devices
(eMMC) that can not be SD, SDIO or combo cards, and we could make
it only appear if using the block layer.

The card status however we need to keep if people want it.

We *COULD* consider just thrashing these debugfs files. It is not
technically ABI and I wonder who is actually using them.

Yours,
Linus Walleij

Re: [PATCH 5/6] mmc: debugfs: Move card status retrieveal into the block layer

[Christoph Hellwig]

On Tue, May 23, 2017 at 11:49:48AM +0200, Linus Walleij wrote:
> In that case CONFIG_MMC_BLOCK contains 'm' instead of 'y'
> which confusingly does not evaluate to true in the preprocessor
> (it assumes it is a misspelled 'n' I guess).
> 
> And then the autobuilders wreak havoc.
> 
> And that is why the IS_ENABLED() defines exist in the first
> place IIUC.
> 
> I'm all for making CONFIG_MMC_BLOCK into a bool... but
> don't know how people (Intel laptops) feel about that extra
> code in their kernel at all times.

All the time?  At least only if any mmc/sd host driver is loaded,
right?  I don't think those additional 20k fatten the cow
(as we say in German).

Re: [PATCH 5/6] mmc: debugfs: Move card status retrieveal into the block layer

[Arnd Bergmann]

On Tue, May 23, 2017 at 11:49 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Mon, May 22, 2017 at 9:42 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 19 May 2017 at 15:37, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>>> The debugfs file "status" (in e.g. /debug/mmc3/mmc3:0001) is
>>> only available if and only if the card used is an (e)MMC or
>>> SD card, not for SDIO, as can be seen from this guard in
>>> mmc_add_card_debugfs();
>>>
>>> if (mmc_card_mmc(card) || mmc_card_sd(card))
>>>   (...create debugfs "status" entry...)
>>>
>>> Further this debugfs entry suffers from all the same starvation
>>> issues as the other userspace things, under e.g. a heavy
>>> dd operation.
>>>
>>> It is therefore logical to move this over to the block layer
>>> when it is enabled, using the new custom requests and issue
>>> it using the block request queue.
>>>
>>> This makes this debugfs card access land under the request
>>> queue host lock instead of orthogonally taking the lock.
>>>
>>> Tested during heavy dd load by cat:in the status file. We
>>> add IS_ENABLED() guards and keep the code snippet just
>>> issueing the card status as a static inline in the header
>>> so we can still have card status working when the block
>>> layer is compiled out.
>>
>> Seems like a bit of re-factoring/cleanup could help here as
>> preparation step. I just posted a patch [1] cleaning up how the mmc
>> block layer fetches the card's status.
>>
>> Perhaps that could at least simplify a bit for $subject patch,
>> especially since it also makes mmc_send_status() being exported.
>
> OK if you merge your stuff I can iterate my patches on top,
> no problem.
>
>>> +#if IS_ENABLED(CONFIG_MMC_BLOCK)
>>
>> What wrong with a regular ifdefs stubs? Why do you need IS_ENABLED()?
>
> This is because the entire block layer can be compiled as a
> module.

The block layer cannot be a module, but the MMC layer can.

> I'm all for making CONFIG_MMC_BLOCK into a bool... but
> don't know how people (Intel laptops) feel about that extra
> code in their kernel at all times.

I think CONFIG_MMC should remain tristate. However, we could
consider making CONFIG_MMC_BLOCK a 'bool' symbol and
linking it into the mmc-core module when enabled.

>> It seems like the life cycle of the card debugfs node is now being
>> controlled as when the mmc block device driver has been successfully
>> probed. We need to deal with that somehow.
>
> Only for these two files but yes.
>
> ext_csd is a bit dubious as it is only available on storage devices
> (eMMC) that can not be SD, SDIO or combo cards, and we could make
> it only appear if using the block layer.
>
> The card status however we need to keep if people want it.
>
> We *COULD* consider just thrashing these debugfs files. It is not
> technically ABI and I wonder who is actually using them.

I think the ext_csd is extremely useful on storage devices to have
exposed to user space in some form. There is a comment saying
that we don't store it in RAM at the moment as it is rarely used,
but we could change that, as it's only 512 bytes per eMMC device
and there is rarely more than one of them. IIRC, the ext_csd is
entirely readonly and doesn't change during runtime, so we
gain nothing by issuing the commands every time someone reads
the debugfs file.

       Arnd

Re: [PATCH 5/6] mmc: debugfs: Move card status retrieveal into the block layer

[Ulf Hansson]

[...]

>> Seems like a bit of re-factoring/cleanup could help here as
>> preparation step. I just posted a patch [1] cleaning up how the mmc
>> block layer fetches the card's status.
>>
>> Perhaps that could at least simplify a bit for $subject patch,
>> especially since it also makes mmc_send_status() being exported.
>
> OK if you merge your stuff I can iterate my patches on top,
> no problem.

Done!

>
>>> +#if IS_ENABLED(CONFIG_MMC_BLOCK)
>>
>> What wrong with a regular ifdefs stubs? Why do you need IS_ENABLED()?
>
> This is because the entire block layer can be compiled as a
> module.
>
> In that case CONFIG_MMC_BLOCK contains 'm' instead of 'y'
> which confusingly does not evaluate to true in the preprocessor
> (it assumes it is a misspelled 'n' I guess).
>
> And then the autobuilders wreak havoc.
>
> And that is why the IS_ENABLED() defines exist in the first
> place IIUC.

Thanks for explanation! It makes more sense to me now!

>
> I'm all for making CONFIG_MMC_BLOCK into a bool... but
> don't know how people (Intel laptops) feel about that extra
> code in their kernel at all times.
>
>>>  void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req);
>>
>> I don't get what mmc_blk_issue_rq() has to do with this change? Could
>> you please explain?
>
> If we start doing stubs we should stub everything IMO, but if you
> prefer I can make that a separate patch. Seems a bit overdone
> though.
>
>> Hmm, this thing seems a bit upside-down.
>>
>> Currently it's possible to build the mmc block device driver as a
>> module. In cases like this, accessing the card debugs node to request
>> the card's status, would trigger a call to mmc_blk_card_status_get().
>> How would that work when the mmc block device driver isn't loaded and
>> probed?
>
> Module symbol resolution and driver loading is always necessary,
> it is no different for e.g. a wifi driver using the 802.11 library.
> It is definately possible to shoot oneself in the foot, but I think
> udev & friends usually load things in the right order?

My my point is that *if* the card debugfs nodes shows up to the user -
they should work (no matter if the mmc block device has been probed or
not).

Can we guarantee that, is this approach?

>
>> It seems like the life cycle of the card debugfs node is now being
>> controlled as when the mmc block device driver has been successfully
>> probed. We need to deal with that somehow.
>
> Only for these two files but yes.
>
> ext_csd is a bit dubious as it is only available on storage devices
> (eMMC) that can not be SD, SDIO or combo cards, and we could make
> it only appear if using the block layer.

Yes, that's a nice option.

>
> The card status however we need to keep if people want it.

Again, this is for debug purpose. I don't see an issue moving also
this one within CONFIG_MMC_BLOCK.

>
> We *COULD* consider just thrashing these debugfs files. It is not
> technically ABI and I wonder who is actually using them.

Right.

According to Avri, apparently some userspace tools measuring
performance. I would be interested to know a bit more about those
tools.

Kind regards
Uffe

Re: [PATCH 5/6] mmc: debugfs: Move card status retrieveal into the block layer

[Ulf Hansson]

[...]

>>>> +#if IS_ENABLED(CONFIG_MMC_BLOCK)
>>>
>>> What wrong with a regular ifdefs stubs? Why do you need IS_ENABLED()?
>>
>> This is because the entire block layer can be compiled as a
>> module.
>
> The block layer cannot be a module, but the MMC layer can.
>
>> I'm all for making CONFIG_MMC_BLOCK into a bool... but
>> don't know how people (Intel laptops) feel about that extra
>> code in their kernel at all times.
>
> I think CONFIG_MMC should remain tristate. However, we could
> consider making CONFIG_MMC_BLOCK a 'bool' symbol and
> linking it into the mmc-core module when enabled.

That is a good idea, no matter what. We should do the same thing for
CONFIG_MMC_TEST. There are really no need to have to separate modules,
one is more than enough.

However, I don't think that by itself, solves the changed life-cycle
issue of the card debugfs node.

Some more protection is needed, to make sure the mmc block device is
loaded/probed before calling mmc_blk_card_status_get().

>
>>> It seems like the life cycle of the card debugfs node is now being
>>> controlled as when the mmc block device driver has been successfully
>>> probed. We need to deal with that somehow.
>>
>> Only for these two files but yes.
>>
>> ext_csd is a bit dubious as it is only available on storage devices
>> (eMMC) that can not be SD, SDIO or combo cards, and we could make
>> it only appear if using the block layer.
>>
>> The card status however we need to keep if people want it.
>>
>> We *COULD* consider just thrashing these debugfs files. It is not
>> technically ABI and I wonder who is actually using them.
>
> I think the ext_csd is extremely useful on storage devices to have
> exposed to user space in some form. There is a comment saying
> that we don't store it in RAM at the moment as it is rarely used,
> but we could change that, as it's only 512 bytes per eMMC device
> and there is rarely more than one of them. IIRC, the ext_csd is
> entirely readonly and doesn't change during runtime, so we
> gain nothing by issuing the commands every time someone reads
> the debugfs file.

First, we have mmc utils (going via mmc ioctl), which purpose are
exactly for these cases. As a matter of fact it does a better job,
since it even present the data in a more human readable format.

Second, ext csd gets updated very often to control/change some
behavior of the device. You must be mixing this up with some of the
other card registers.

Kind regards
Uffe

Re: [PATCH 5/6] mmc: debugfs: Move card status retrieveal into the block layer

[Arnd Bergmann]

On Tue, May 23, 2017 at 1:22 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> [...]
>
>>>>> +#if IS_ENABLED(CONFIG_MMC_BLOCK)
>>>>
>>>> What wrong with a regular ifdefs stubs? Why do you need IS_ENABLED()?
>>>
>>> This is because the entire block layer can be compiled as a
>>> module.
>>
>> The block layer cannot be a module, but the MMC layer can.
>>
>>> I'm all for making CONFIG_MMC_BLOCK into a bool... but
>>> don't know how people (Intel laptops) feel about that extra
>>> code in their kernel at all times.
>>
>> I think CONFIG_MMC should remain tristate. However, we could
>> consider making CONFIG_MMC_BLOCK a 'bool' symbol and
>> linking it into the mmc-core module when enabled.
>
> That is a good idea, no matter what. We should do the same thing for
> CONFIG_MMC_TEST. There are really no need to have to separate modules,
> one is more than enough.

I think for the test module it conceptually makes more sense to
be separate, though I can see that this requires to be careful
with the debugfs file lifetime rules.

>>>> It seems like the life cycle of the card debugfs node is now being
>>>> controlled as when the mmc block device driver has been successfully
>>>> probed. We need to deal with that somehow.
>>>
>>> Only for these two files but yes.
>>>
>>> ext_csd is a bit dubious as it is only available on storage devices
>>> (eMMC) that can not be SD, SDIO or combo cards, and we could make
>>> it only appear if using the block layer.
>>>
>>> The card status however we need to keep if people want it.
>>>
>>> We *COULD* consider just thrashing these debugfs files. It is not
>>> technically ABI and I wonder who is actually using them.
>>
>> I think the ext_csd is extremely useful on storage devices to have
>> exposed to user space in some form. There is a comment saying
>> that we don't store it in RAM at the moment as it is rarely used,
>> but we could change that, as it's only 512 bytes per eMMC device
>> and there is rarely more than one of them. IIRC, the ext_csd is
>> entirely readonly and doesn't change during runtime, so we
>> gain nothing by issuing the commands every time someone reads
>> the debugfs file.
>
> First, we have mmc utils (going via mmc ioctl), which purpose are
> exactly for these cases. As a matter of fact it does a better job,
> since it even present the data in a more human readable format.
>
> Second, ext csd gets updated very often to control/change some
> behavior of the device. You must be mixing this up with some of the
> other card registers.

Ok, it's been a while since I looked at the details, thanks for the
clarification.

     Arnd