2012-05-16 15:31:12

by Saugata Das

[permalink] [raw]
Subject: [RFC 1/3] block: Context support

From: Saugata Das <[email protected]>

On eMMC and UFS devices there is a new feature of setting context with each
read or write. The idea is to classify the data from different files and
apply the realibility on the complete file instead of individual writes,
which helps in performance. A new address space operation has been a added
to get the context from file system and set up the bi_context field in bio.
Then we need to ensure that bio from different contexts are not merged. The
context is then passed to the underlying driver as part of the read or write
request. Since the number of MMC contexts is limited, multiple file system
contexts are mapped to single MMC context.

Signed-off-by: Saugata Das <[email protected]>
---
block/blk-core.c | 1 +
block/blk-merge.c | 3 +++
fs/mpage.c | 12 ++++++++++++
include/linux/blk_types.h | 1 +
include/linux/blkdev.h | 1 +
include/linux/buffer_head.h | 2 ++
include/linux/fs.h | 1 +
7 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 1f61b74..274e05d 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1309,6 +1309,7 @@ void init_request_from_bio(struct request *req, struct bio *bio)
req->errors = 0;
req->__sector = bio->bi_sector;
req->ioprio = bio_prio(bio);
+ req->context = bio->bi_context;
blk_rq_bio_prep(req->q, req, bio);
}

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 160035f..ed70d56 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -497,6 +497,9 @@ bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
if (bio_integrity(bio) != blk_integrity_rq(rq))
return false;

+ if (bio->bi_context != rq->bio->bi_context)
+ return false;
+
return true;
}

diff --git a/fs/mpage.c b/fs/mpage.c
index 0face1c..4889842 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -293,6 +293,12 @@ alloc_new:
goto confused;
}

+ if (page && page->mapping && page->mapping->a_ops &&
+ page->mapping->a_ops->get_context)
+ bio->bi_context = page->mapping->a_ops->get_context(page);
+ else
+ bio->bi_context = 0;
+
length = first_hole << blkbits;
if (bio_add_page(bio, page, length, 0) < length) {
bio = mpage_bio_submit(READ, bio);
@@ -581,6 +587,12 @@ alloc_new:
goto confused;
}

+ if (page && page->mapping && page->mapping->a_ops &&
+ page->mapping->a_ops->get_context)
+ bio->bi_context = page->mapping->a_ops->get_context(page);
+ else
+ bio->bi_context = 0;
+
/*
* Must try to add the page before marking the buffer clean or
* the confused fail path above (OOM) will be very confused when
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 4053cbd..f3ac448 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -42,6 +42,7 @@ struct bio {

unsigned short bi_vcnt; /* how many bio_vec's */
unsigned short bi_idx; /* current index into bvl_vec */
+ unsigned long bi_context; /* context of this bio */

/* Number of segments in this BIO after
* physical address coalescing is performed.
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 2aa2466..0dd9a08 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -167,6 +167,7 @@ struct request {
struct list_head timeout_list;
unsigned int timeout;
int retries;
+ unsigned long context; /* context of this request */

/*
* completion callback.
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 13bba17..0776564 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -72,6 +72,8 @@ struct buffer_head {
struct list_head b_assoc_buffers; /* associated with another mapping */
struct address_space *b_assoc_map; /* mapping this buffer is
associated with */
+ unsigned long b_context; /* context for this buffer within the
+ storage device */
atomic_t b_count; /* users using this buffer_head */
};

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 8de6755..4b379d8 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -626,6 +626,7 @@ struct address_space_operations {
int (*is_partially_uptodate) (struct page *, read_descriptor_t *,
unsigned long);
int (*error_remove_page)(struct address_space *, struct page *);
+ int (*get_context)(struct page *);
};

extern const struct address_space_operations empty_aops;
--
1.7.4.3



2012-05-16 15:30:29

by Saugata Das

[permalink] [raw]
Subject: [RFC 2/3] ext4: Context support

From: Saugata Das <[email protected]>

On eMMC and UFS devices there is a new feature of setting context with
each read or write. The idea is to classify the data from different files
and apply the realibility on the complete file instead of individual writes.
On ext4 file system, the inode number of the file is passed as b_context
in the bh structure during write and via the get_context callback function
during read. Since number of MMC contexts is limited, multiple file system
contexts are mapped to single MMC context.

Signed-off-by: Saugata Das <[email protected]>
---
fs/ext4/inode.c | 33 +++++++++++++++++++++++++++++++++
fs/ext4/page-io.c | 1 +
2 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 754fe77..2667396 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -790,6 +790,21 @@ static int do_journal_get_write_access(handle_t *handle,
return ret;
}

+/* Get the context of the buffer within the underlying storage device */
+static int ext4_get_context(struct page *page)
+{
+ if (page && page->mapping && page->mapping->host)
+ return page->mapping->host->i_ino;
+ else
+ return 0;
+}
+
+static int ext4_set_buffer_context(handle_t *handle, struct buffer_head *bh)
+{
+ bh->b_context = ext4_get_context(bh->b_page);
+ return 0;
+}
+
static int ext4_get_block_write(struct inode *inode, sector_t iblock,
struct buffer_head *bh_result, int create);
static int ext4_write_begin(struct file *file, struct address_space *mapping,
@@ -843,6 +858,11 @@ retry:
from, to, NULL, do_journal_get_write_access);
}

+ if (!ret && walk_page_buffers(NULL, page_buffers(page),
+ from, to, NULL, ext4_set_buffer_context)) {
+ ext4_warning(inode->i_sb, "Couldn't set context\n");
+ }
+
if (ret) {
unlock_page(page);
page_cache_release(page);
@@ -2394,8 +2414,11 @@ static int ext4_da_write_begin(struct file *file, struct address_space *mapping,
pgoff_t index;
struct inode *inode = mapping->host;
handle_t *handle;
+ unsigned from, to;

index = pos >> PAGE_CACHE_SHIFT;
+ from = pos & (PAGE_CACHE_SIZE - 1);
+ to = from + len;

if (ext4_nonda_switch(inode->i_sb)) {
*fsdata = (void *)FALL_BACK_TO_NONDELALLOC;
@@ -2444,6 +2467,12 @@ retry:

if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
goto retry;
+
+ if (walk_page_buffers(NULL, page_buffers(page),
+ from, to, NULL, ext4_set_buffer_context)) {
+ ext4_warning(inode->i_sb, "Couldn't set context\n");
+ }
+
out:
return ret;
}
@@ -3040,6 +3069,7 @@ static const struct address_space_operations ext4_ordered_aops = {
.migratepage = buffer_migrate_page,
.is_partially_uptodate = block_is_partially_uptodate,
.error_remove_page = generic_error_remove_page,
+ .get_context = ext4_get_context,
};

static const struct address_space_operations ext4_writeback_aops = {
@@ -3055,6 +3085,7 @@ static const struct address_space_operations ext4_writeback_aops = {
.migratepage = buffer_migrate_page,
.is_partially_uptodate = block_is_partially_uptodate,
.error_remove_page = generic_error_remove_page,
+ .get_context = ext4_get_context,
};

static const struct address_space_operations ext4_journalled_aops = {
@@ -3070,6 +3101,7 @@ static const struct address_space_operations ext4_journalled_aops = {
.direct_IO = ext4_direct_IO,
.is_partially_uptodate = block_is_partially_uptodate,
.error_remove_page = generic_error_remove_page,
+ .get_context = ext4_get_context,
};

static const struct address_space_operations ext4_da_aops = {
@@ -3086,6 +3118,7 @@ static const struct address_space_operations ext4_da_aops = {
.migratepage = buffer_migrate_page,
.is_partially_uptodate = block_is_partially_uptodate,
.error_remove_page = generic_error_remove_page,
+ .get_context = ext4_get_context,
};

void ext4_set_aops(struct inode *inode)
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index dcdeef1..bf1381e 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -296,6 +296,7 @@ static int io_submit_init(struct ext4_io_submit *io,
bio = bio_alloc(GFP_NOIO, min(nvecs, BIO_MAX_PAGES));
bio->bi_sector = bh->b_blocknr * (bh->b_size >> 9);
bio->bi_bdev = bh->b_bdev;
+ bio->bi_context = bh->b_context;
bio->bi_private = io->io_end = io_end;
bio->bi_end_io = ext4_end_bio;

--
1.7.4.3


2012-05-16 15:30:30

by Saugata Das

[permalink] [raw]
Subject: [RFC 3/3] mmc: Context support

From: Saugata Das <[email protected]>

This patch implements the context ID support at MMC layer. From file system
(ext4), the context is passed in the request structure. At MMC layer the
context is retrieved from the request structure and then used in the CMD23
argument. Since number of MMC contexts is limited, multiple file system
contexts are mapped to single MMC contexts. When the REQ_SYNC or REQ_FLUSH
flag is set, the context is flushed or sync'ed, in which the context is
closed so that the data blocks are safely written out to non-volatile memory
and then the context is opened again.

Signed-off-by: Saugata Das <[email protected]>
---
drivers/mmc/card/block.c | 35 +++++++++++++++++++++-
drivers/mmc/core/core.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++
drivers/mmc/core/mmc.c | 28 ++++++++++++++++++
include/linux/mmc/card.h | 4 ++
include/linux/mmc/core.h | 4 ++
include/linux/mmc/host.h | 1 +
include/linux/mmc/mmc.h | 3 ++
7 files changed, 145 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index dabec55..914f942 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -958,6 +958,15 @@ static int mmc_blk_issue_flush(struct mmc_queue *mq, struct request *req)
struct mmc_card *card = md->queue.card;
int ret = 0;

+ /*
+ * The flush command is a synchronization point from file system.
+ * The contexts are flushed here to ensure that the data written
+ * in the open contexts are saved reliably in non-volatile media
+ */
+ ret = mmc_flush_contexts(card);
+ if (ret)
+ ret = -EIO;
+
ret = mmc_flush_cache(card);
if (ret)
ret = -EIO;
@@ -1207,11 +1216,16 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
*/
if ((md->flags & MMC_BLK_CMD23) && mmc_op_multi(brq->cmd.opcode) &&
(do_rel_wr || !(card->quirks & MMC_QUIRK_BLK_NO_CMD23) ||
- do_data_tag)) {
+ do_data_tag || (card->ext_csd.max_context_id > 0))) {
+ int context_id = (req->context &&
+ card->ext_csd.max_context_id) ?
+ (req->context % card->ext_csd.max_context_id + 1) :
+ 0;
brq->sbc.opcode = MMC_SET_BLOCK_COUNT;
brq->sbc.arg = brq->data.blocks |
(do_rel_wr ? (1 << 31) : 0) |
- (do_data_tag ? (1 << 29) : 0);
+ (do_data_tag ? (1 << 29) : 0) |
+ (!do_data_tag ? (context_id << 25) : 0);
brq->sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
brq->mrq.sbc = &brq->sbc;
}
@@ -1440,6 +1454,23 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
mmc_blk_issue_rw_rq(mq, NULL);
ret = mmc_blk_issue_flush(mq, req);
} else {
+ if (req && (req->cmd_flags & REQ_SYNC) &&
+ req->context && card->ext_csd.max_context_id) {
+ int context_cfg_id = (req->context) ?
+ req->context % card->ext_csd.max_context_id : 0;
+ /*
+ * The SYNC command is a synchronization point from
+ * file system. The relevent context is sync'ed here
+ * to ensure that the data written in the open context
+ * are saved reliably in non-volatile media
+ */
+ if (card->host->areq)
+ mmc_blk_issue_rw_rq(mq, NULL);
+ mmc_sync_context(card, context_cfg_id);
+ /* this write will go without context to ensure
+ that it is reliably written */
+ req->context = 0;
+ }
ret = mmc_blk_issue_rw_rq(mq, req);
}

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index ba821fe..728145a 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2262,6 +2262,78 @@ int mmc_cache_ctrl(struct mmc_host *host, u8 enable)
}
EXPORT_SYMBOL(mmc_cache_ctrl);

+/*
+ * Synchronize a context by first closing the context and then
+ * opening it
+ */
+int mmc_sync_context(struct mmc_card *card, int context_id)
+{
+ int err = 0;
+
+ err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+ EXT_CSD_CONTEXT_CONF + context_id,
+ 0x0, card->ext_csd.generic_cmd6_time);
+
+ if (err)
+ return err;
+
+ err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+ EXT_CSD_CONTEXT_CONF + context_id,
+ 0x3, card->ext_csd.generic_cmd6_time);
+
+ return err;
+}
+EXPORT_SYMBOL(mmc_sync_context);
+
+int mmc_flush_contexts(struct mmc_card *card)
+{
+ int i;
+ for (i = 1; i <= card->ext_csd.max_context_id; i++) {
+ int err = mmc_sync_context(card, i);
+ if (err)
+ return err;
+ }
+ return 0;
+}
+EXPORT_SYMBOL(mmc_flush_contexts);
+
+/*
+ * Initialize all the MMC contexts in read-write and non-LU mode
+ */
+int mmc_init_context(struct mmc_card *card)
+{
+ int i, err = 0;
+
+ for (i = 0; i < card->ext_csd.max_context_id; i++) {
+ err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+ EXT_CSD_CONTEXT_CONF + i,
+ 0x3, card->ext_csd.generic_cmd6_time);
+ if (err) {
+ pr_warning("%s: Activating of context %d failed [%x]\n",
+ mmc_hostname(card->host), i, err);
+ break;
+ }
+ }
+
+ if (!err)
+ return 0;
+
+ /*
+ * Close the opened contexts
+ */
+ for (i = i-1; i >= 0; i--) {
+ int err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+ EXT_CSD_CONTEXT_CONF + i,
+ 0x0, card->ext_csd.generic_cmd6_time);
+ if (err)
+ pr_warning("%s: Closing of context %d failed [%x]\n",
+ mmc_hostname(card->host), i, err);
+ }
+
+ return err;
+}
+EXPORT_SYMBOL(mmc_init_context);
+
#ifdef CONFIG_PM

/**
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 54df5ad..84bec43 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -533,6 +533,20 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
} else {
card->ext_csd.data_tag_unit_size = 0;
}
+
+ /* LU size in 512 byt sectors */
+ card->ext_csd.lu_size =
+ (ext_csd[EXT_CSD_LARGE_UNIT_SIZE_M1] + 1) * 0x800;
+
+ card->ext_csd.max_context_id =
+ ext_csd[EXT_CSD_CONTEXT_CAPABILITIES] & 0x0f;
+
+ if (card->ext_csd.max_context_id > MAX_MMC_CONTEXT_ID) {
+ pr_warning("%s: card has invalid number of contexts [%d]\n",
+ mmc_hostname(card->host),
+ card->ext_csd.max_context_id);
+ card->ext_csd.max_context_id = 0;
+ }
}

out:
@@ -1267,6 +1281,20 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
}
}

+ if (host->caps2 & MMC_CAP2_CONTEXT) {
+ if (card->ext_csd.max_context_id > 0) {
+ err = mmc_init_context(card);
+ if (err && err != -EBADMSG)
+ goto free_card;
+ if (err) {
+ pr_warning("%s: failed to activate context (%x)\n",
+ mmc_hostname(card->host), err);
+ card->ext_csd.max_context_id = 0;
+ err = 0;
+ }
+ }
+ }
+
if (!oldcard)
host->card = card;

diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 629b823..adf2c84 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -74,6 +74,8 @@ struct mmc_ext_csd {
unsigned int hpi_cmd; /* cmd used as HPI */
unsigned int data_sector_size; /* 512 bytes or 4KB */
unsigned int data_tag_unit_size; /* DATA TAG UNIT size */
+ unsigned int lu_size;
+ unsigned int max_context_id;
unsigned int boot_ro_lock; /* ro lock support */
bool boot_ro_lockable;
u8 raw_partition_support; /* 160 */
@@ -184,6 +186,8 @@ struct sdio_func_tuple;
#define MMC_NUM_PHY_PARTITION 6
#define MAX_MMC_PART_NAME_LEN 20

+#define MAX_MMC_CONTEXT_ID 15
+
/*
* MMC Physical partitions
*/
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 1b431c7..a4e6bc9 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -179,6 +179,10 @@ extern int mmc_try_claim_host(struct mmc_host *host);

extern int mmc_flush_cache(struct mmc_card *);

+extern int mmc_sync_context(struct mmc_card *card, int context_id);
+extern int mmc_flush_contexts(struct mmc_card *card);
+extern int mmc_init_context(struct mmc_card *card);
+
extern int mmc_detect_card_removed(struct mmc_host *host);

/**
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0707d22..688348f 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -233,6 +233,7 @@ struct mmc_host {
#define MMC_CAP2_NO_SLEEP_CMD (1 << 4) /* Don't allow sleep command */
#define MMC_CAP2_HS200_1_8V_SDR (1 << 5) /* can support */
#define MMC_CAP2_HS200_1_2V_SDR (1 << 6) /* can support */
+#define MMC_CAP2_CONTEXT (1<<7) /* Context ID supported */
#define MMC_CAP2_HS200 (MMC_CAP2_HS200_1_8V_SDR | \
MMC_CAP2_HS200_1_2V_SDR)
#define MMC_CAP2_BROKEN_VOLTAGE (1 << 7) /* Use the broken voltage */
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index b822a2c..8f8d958 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -274,6 +274,7 @@ struct _mmc_csd {
#define EXT_CSD_FLUSH_CACHE 32 /* W */
#define EXT_CSD_CACHE_CTRL 33 /* R/W */
#define EXT_CSD_POWER_OFF_NOTIFICATION 34 /* R/W */
+#define EXT_CSD_CONTEXT_CONF 37 /* R/W */
#define EXT_CSD_DATA_SECTOR_SIZE 61 /* R */
#define EXT_CSD_GP_SIZE_MULT 143 /* R/W */
#define EXT_CSD_PARTITION_ATTRIBUTE 156 /* R/W */
@@ -316,6 +317,8 @@ struct _mmc_csd {
#define EXT_CSD_POWER_OFF_LONG_TIME 247 /* RO */
#define EXT_CSD_GENERIC_CMD6_TIME 248 /* RO */
#define EXT_CSD_CACHE_SIZE 249 /* RO, 4 bytes */
+#define EXT_CSD_LARGE_UNIT_SIZE_M1 495 /* RO */
+#define EXT_CSD_CONTEXT_CAPABILITIES 496 /* RO */
#define EXT_CSD_TAG_UNIT_SIZE 498 /* RO */
#define EXT_CSD_DATA_TAG_SUPPORT 499 /* RO */
#define EXT_CSD_HPI_FEATURES 503 /* RO */
--
1.7.4.3


2012-05-18 13:26:52

by Subhash Jadavani

[permalink] [raw]
Subject: RE: [RFC 3/3] mmc: Context support

Hi Sougata,

Please find few comments inline below.

Regards,
Subhash

> -----Original Message-----
> From: [email protected] [mailto:linux-mmc-
> [email protected]] On Behalf Of Saugata Das
> Sent: Wednesday, May 16, 2012 9:01 PM
> To: [email protected]; [email protected]; linux-
> [email protected]
> Cc: [email protected]; [email protected]; [email protected]
> Subject: [RFC 3/3] mmc: Context support
>
> From: Saugata Das <[email protected]>
>
> This patch implements the context ID support at MMC layer. From file
> system (ext4), the context is passed in the request structure. At MMC
layer
> the context is retrieved from the request structure and then used in the
> CMD23 argument. Since number of MMC contexts is limited, multiple file
> system contexts are mapped to single MMC contexts. When the REQ_SYNC
> or REQ_FLUSH flag is set, the context is flushed or sync'ed, in which the
> context is closed so that the data blocks are safely written out to non-
> volatile memory and then the context is opened again.
>
> Signed-off-by: Saugata Das <[email protected]>
> ---
> drivers/mmc/card/block.c | 35 +++++++++++++++++++++-
> drivers/mmc/core/core.c | 72
> ++++++++++++++++++++++++++++++++++++++++++++++
> drivers/mmc/core/mmc.c | 28 ++++++++++++++++++
> include/linux/mmc/card.h | 4 ++
> include/linux/mmc/core.h | 4 ++
> include/linux/mmc/host.h | 1 +
> include/linux/mmc/mmc.h | 3 ++
> 7 files changed, 145 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c index
> dabec55..914f942 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -958,6 +958,15 @@ static int mmc_blk_issue_flush(struct mmc_queue
> *mq, struct request *req)
> struct mmc_card *card = md->queue.card;
> int ret = 0;
>
> + /*
> + * The flush command is a synchronization point from file system.
> + * The contexts are flushed here to ensure that the data written
> + * in the open contexts are saved reliably in non-volatile media
> + */
> + ret = mmc_flush_contexts(card);

This is called unconditionally. Shouldn't we check if context is really
enabled or not and host have asked (by defining MMC_CAP2_CONTEXT) for it or
not.

Also shouln't this mmc_flush_contexts() be called after the
__blk_end_request_all() in this same function?

> + if (ret)
> + ret = -EIO;
> +
> ret = mmc_flush_cache(card);
> if (ret)
> ret = -EIO;
> @@ -1207,11 +1216,16 @@ static void mmc_blk_rw_rq_prep(struct
> mmc_queue_req *mqrq,
> */
> if ((md->flags & MMC_BLK_CMD23) && mmc_op_multi(brq-
> >cmd.opcode) &&
> (do_rel_wr || !(card->quirks & MMC_QUIRK_BLK_NO_CMD23) ||
> - do_data_tag)) {
> + do_data_tag || (card->ext_csd.max_context_id > 0))) {

card->ext_csd.max_context_id can be non-zero even if host has not enabled
MMC_CAP2_CONTEXT cap. So you should add proper check here before tagging
the context id.

> + int context_id = (req->context &&
> + card->ext_csd.max_context_id) ?
> + (req->context % card->ext_csd.max_context_id + 1) :
> + 0;
> brq->sbc.opcode = MMC_SET_BLOCK_COUNT;
> brq->sbc.arg = brq->data.blocks |
> (do_rel_wr ? (1 << 31) : 0) |
> - (do_data_tag ? (1 << 29) : 0);
> + (do_data_tag ? (1 << 29) : 0) |
> + (!do_data_tag ? (context_id << 25) : 0);
> brq->sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
> brq->mrq.sbc = &brq->sbc;
> }
> @@ -1440,6 +1454,23 @@ static int mmc_blk_issue_rq(struct mmc_queue
> *mq, struct request *req)
> mmc_blk_issue_rw_rq(mq, NULL);
> ret = mmc_blk_issue_flush(mq, req);
> } else {
> + if (req && (req->cmd_flags & REQ_SYNC) &&
> + req->context && card->ext_csd.max_context_id) {
> + int context_cfg_id = (req->context) ?
> + req->context % card-
> >ext_csd.max_context_id : 0;
> + /*
> + * The SYNC command is a synchronization point
> from
> + * file system. The relevent context is sync'ed here
> + * to ensure that the data written in the open
> context
> + * are saved reliably in non-volatile media
> + */
> + if (card->host->areq)
> + mmc_blk_issue_rw_rq(mq, NULL);
> + mmc_sync_context(card, context_cfg_id);
> + /* this write will go without context to ensure
> + that it is reliably written */

Use multiline comments format.

> + req->context = 0;
> + }
> ret = mmc_blk_issue_rw_rq(mq, req);
> }
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
> ba821fe..728145a 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2262,6 +2262,78 @@ int mmc_cache_ctrl(struct mmc_host *host, u8
> enable) } EXPORT_SYMBOL(mmc_cache_ctrl);
>
> +/*
> + * Synchronize a context by first closing the context and then
> + * opening it
> + */
> +int mmc_sync_context(struct mmc_card *card, int context_id) {
> + int err = 0;
> +
> + err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> + EXT_CSD_CONTEXT_CONF + context_id,
> + 0x0, card->ext_csd.generic_cmd6_time);
> +
> + if (err)
> + return err;
> +
> + err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> + EXT_CSD_CONTEXT_CONF + context_id,

Value of context_id passed to this function can range from 1 to 15. If it's
15 then (EXT_CSD_CONTEXT_CONF + context_id) would be 52 which is beyond the
CONTEXT_CONF [51:37].
So basically the index value passed to mmc_switch() should be
(EXT_CSD_CONTEXT_CONF + context_id - 1).

> + 0x3, card->ext_csd.generic_cmd6_time);
> +
> + return err;
> +}
> +EXPORT_SYMBOL(mmc_sync_context);
> +
> +int mmc_flush_contexts(struct mmc_card *card) {
> + int i;

One line space after the declaration missing.

> + for (i = 1; i <= card->ext_csd.max_context_id; i++) {
> + int err = mmc_sync_context(card, i);
> + if (err)
> + return err;

Shouldn't we try to flush other contexts even if flush for other context
fails?

> + }
> + return 0;
> +}
> +EXPORT_SYMBOL(mmc_flush_contexts);
> +
> +/*
> + * Initialize all the MMC contexts in read-write and non-LU mode */
> +int mmc_init_context(struct mmc_card *card) {
> + int i, err = 0;
> +
> + for (i = 0; i < card->ext_csd.max_context_id; i++) {
> + err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> + EXT_CSD_CONTEXT_CONF + i,
> + 0x3, card-
> >ext_csd.generic_cmd6_time);

I would suggest a separate function which takes the context index,
activation mode, large unit context as it's arguments and writes the
activation mode in appropriate context configuration offset. So in future if
we want to add enable other contexts, we can use the same function.

Also, let's have macro for 0x03 to indicate what it really means (read/write
context).

> + if (err) {
> + pr_warning("%s: Activating of context %d failed
> [%x]\n",
> + mmc_hostname(card->host), i, err);
> + break;
> + }
> + }
> +
> + if (!err)
> + return 0;
> +
> + /*
> + * Close the opened contexts
> + */

Why should we close the already opened contexts? Rather we can still use the
opened contexts. Right?

> + for (i = i-1; i >= 0; i--) {
> + int err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> + EXT_CSD_CONTEXT_CONF + i,
> + 0x0, card-
> >ext_csd.generic_cmd6_time);
> + if (err)
> + pr_warning("%s: Closing of context %d failed
> [%x]\n",
> + mmc_hostname(card->host), i, err);
> + }
> +
> + return err;
> +}
> +EXPORT_SYMBOL(mmc_init_context);
> +
> #ifdef CONFIG_PM
>
> /**
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index
> 54df5ad..84bec43 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -533,6 +533,20 @@ static int mmc_read_ext_csd(struct mmc_card *card,
> u8 *ext_csd)
> } else {
> card->ext_csd.data_tag_unit_size = 0;
> }
> +
> + /* LU size in 512 byt sectors */

Typo here. s/byt/byte

> + card->ext_csd.lu_size =
> + (ext_csd[EXT_CSD_LARGE_UNIT_SIZE_M1] + 1) *
> 0x800;

In this patch you are not using the large unit context anywhere then better
not to read the large unit size as part of this patch. Let it be completely
handled separately when someone adds the large unit context in future.

> +
> + card->ext_csd.max_context_id =
> + ext_csd[EXT_CSD_CONTEXT_CAPABILITIES] & 0x0f;

This is what spec says: " The mandatory minimum value for this field is 5
plus the default ID #0 (MAX_CONTEXT_ID shall be 5 or higher) ". Can you add
check for this? And if card violates this, we at least should print a
warning.

> +
> + if (card->ext_csd.max_context_id >
> MAX_MMC_CONTEXT_ID) {

Is this check required? You are already &ing with 0x0f which means the
max_context_id will never be more than MAX_MMC_CONTEXT_ID (which is 15).

> + pr_warning("%s: card has invalid number of contexts
> [%d]\n",
> + mmc_hostname(card->host),
> + card->ext_csd.max_context_id);
> + card->ext_csd.max_context_id = 0;
> + }
> }
>
> out:
> @@ -1267,6 +1281,20 @@ static int mmc_init_card(struct mmc_host *host,
> u32 ocr,
> }
> }
>
> + if (host->caps2 & MMC_CAP2_CONTEXT) {
> + if (card->ext_csd.max_context_id > 0) {
> + err = mmc_init_context(card);
> + if (err && err != -EBADMSG)
> + goto free_card;
> + if (err) {
> + pr_warning("%s: failed to activate context
> (%x)\n",
> + mmc_hostname(card->host),
> err);
> + card->ext_csd.max_context_id = 0;
> + err = 0;
> + }
> + }
> + }
> +
> if (!oldcard)
> host->card = card;
>
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h index
> 629b823..adf2c84 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -74,6 +74,8 @@ struct mmc_ext_csd {
> unsigned int hpi_cmd; /* cmd used as HPI
*/
> unsigned int data_sector_size; /* 512 bytes or 4KB
*/
> unsigned int data_tag_unit_size; /* DATA TAG UNIT
size */
> + unsigned int lu_size;
> + unsigned int max_context_id;
> unsigned int boot_ro_lock; /* ro lock support
*/
> bool boot_ro_lockable;
> u8 raw_partition_support; /* 160 */
> @@ -184,6 +186,8 @@ struct sdio_func_tuple;
> #define MMC_NUM_PHY_PARTITION 6
> #define MAX_MMC_PART_NAME_LEN 20
>
> +#define MAX_MMC_CONTEXT_ID 15
> +
> /*
> * MMC Physical partitions
> */
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h index
> 1b431c7..a4e6bc9 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -179,6 +179,10 @@ extern int mmc_try_claim_host(struct mmc_host
> *host);
>
> extern int mmc_flush_cache(struct mmc_card *);
>
> +extern int mmc_sync_context(struct mmc_card *card, int context_id);
> +extern int mmc_flush_contexts(struct mmc_card *card); extern int
> +mmc_init_context(struct mmc_card *card);
> +
> extern int mmc_detect_card_removed(struct mmc_host *host);
>
> /**
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index
> 0707d22..688348f 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -233,6 +233,7 @@ struct mmc_host {
> #define MMC_CAP2_NO_SLEEP_CMD (1 << 4) /* Don't allow sleep
> command */
> #define MMC_CAP2_HS200_1_8V_SDR (1 << 5) /* can support */
> #define MMC_CAP2_HS200_1_2V_SDR (1 << 6) /* can support */
> +#define MMC_CAP2_CONTEXT (1<<7) /* Context ID supported */
> #define MMC_CAP2_HS200 (MMC_CAP2_HS200_1_8V_SDR | \
> MMC_CAP2_HS200_1_2V_SDR)
> #define MMC_CAP2_BROKEN_VOLTAGE (1 << 7) /* Use the broken
> voltage */
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h index
> b822a2c..8f8d958 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -274,6 +274,7 @@ struct _mmc_csd {
> #define EXT_CSD_FLUSH_CACHE 32 /* W */
> #define EXT_CSD_CACHE_CTRL 33 /* R/W */
> #define EXT_CSD_POWER_OFF_NOTIFICATION 34 /* R/W */
> +#define EXT_CSD_CONTEXT_CONF 37 /* R/W */
> #define EXT_CSD_DATA_SECTOR_SIZE 61 /* R */
> #define EXT_CSD_GP_SIZE_MULT 143 /* R/W */
> #define EXT_CSD_PARTITION_ATTRIBUTE 156 /* R/W */
> @@ -316,6 +317,8 @@ struct _mmc_csd {
> #define EXT_CSD_POWER_OFF_LONG_TIME 247 /* RO */
> #define EXT_CSD_GENERIC_CMD6_TIME 248 /* RO */
> #define EXT_CSD_CACHE_SIZE 249 /* RO, 4 bytes */
> +#define EXT_CSD_LARGE_UNIT_SIZE_M1 495 /* RO */
> +#define EXT_CSD_CONTEXT_CAPABILITIES 496 /* RO */
> #define EXT_CSD_TAG_UNIT_SIZE 498 /* RO */
> #define EXT_CSD_DATA_TAG_SUPPORT 499 /* RO */
> #define EXT_CSD_HPI_FEATURES 503 /* RO */
> --
> 1.7.4.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the
> body of a message to [email protected] More majordomo info at
> http://vger.kernel.org/majordomo-info.html


2012-05-19 05:30:00

by Saugata Das

[permalink] [raw]
Subject: Re: [RFC 3/3] mmc: Context support

On 18 May 2012 18:56, Subhash Jadavani <[email protected]> wrote:
> Hi Sougata,
>
> Please find few comments inline below.
>

Many thanks for your time and comments,


> Regards,
> Subhash
>
>> -----Original Message-----
>> From: [email protected] [mailto:linux-mmc-
>> [email protected]] On Behalf Of Saugata Das
>> Sent: Wednesday, May 16, 2012 9:01 PM
>> To: [email protected]; [email protected]; linux-
>> [email protected]
>> Cc: [email protected]; [email protected]; [email protected]
>> Subject: [RFC 3/3] mmc: Context support
>>
>> From: Saugata Das <[email protected]>
>>
>> This patch implements the context ID support at MMC layer. From file
>> system (ext4), the context is passed in the request structure. At MMC
> layer
>> the context is retrieved from the request structure and then used in the
>> CMD23 argument. Since number of MMC contexts is limited, multiple file
>> system contexts are mapped to single MMC contexts. When the REQ_SYNC
>> or REQ_FLUSH flag is set, the context is flushed or sync'ed, in which the
>> context is closed so that the data blocks are safely written out to non-
>> volatile memory and then the context is opened again.
>>
>> Signed-off-by: Saugata Das <[email protected]>
>> ---
>> ?drivers/mmc/card/block.c | ? 35 +++++++++++++++++++++-
>> ?drivers/mmc/core/core.c ?| ? 72
>> ++++++++++++++++++++++++++++++++++++++++++++++
>> ?drivers/mmc/core/mmc.c ? | ? 28 ++++++++++++++++++
>> ?include/linux/mmc/card.h | ? ?4 ++
>> ?include/linux/mmc/core.h | ? ?4 ++
>> ?include/linux/mmc/host.h | ? ?1 +
>> ?include/linux/mmc/mmc.h ?| ? ?3 ++
>> ?7 files changed, 145 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c index
>> dabec55..914f942 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -958,6 +958,15 @@ static int mmc_blk_issue_flush(struct mmc_queue
>> *mq, struct request *req)
>> ? ? ? struct mmc_card *card = md->queue.card;
>> ? ? ? int ret = 0;
>>
>> + ? ? /*
>> + ? ? ?* The flush command is a synchronization point from file system.
>> + ? ? ?* The contexts are flushed here to ensure that the data written
>> + ? ? ?* in the open contexts are saved reliably in non-volatile media
>> + ? ? ?*/
>> + ? ? ret = mmc_flush_contexts(card);
>
> This is called unconditionally. Shouldn't we check if context is really
> enabled or not and host have asked (by defining MMC_CAP2_CONTEXT) for it or
> not.

I think the best approach will be to set max_context_id to 0 in
mmc_read_ext_csd, if MMC_CAP2_CONTEXT is not enabled. Then, we do not
have to add a check for MMC_CAP2_CONTEXT, wherever we want to use the
context.

>
> Also shouln't this mmc_flush_contexts() be called after the
> __blk_end_request_all() in this same function?
>

I do not think so. This is in line with how mmc_flush_cache is done
today, i.e. we complete the closing/flushing all pending operations on
the device before __blk_end_request_all.


>> + ? ? if (ret)
>> + ? ? ? ? ? ? ret = -EIO;
>> +
>> ? ? ? ret = mmc_flush_cache(card);
>> ? ? ? if (ret)
>> ? ? ? ? ? ? ? ret = -EIO;
>> @@ -1207,11 +1216,16 @@ static void mmc_blk_rw_rq_prep(struct
>> mmc_queue_req *mqrq,
>> ? ? ? ?*/
>> ? ? ? if ((md->flags & MMC_BLK_CMD23) && mmc_op_multi(brq-
>> >cmd.opcode) &&
>> ? ? ? ? ? (do_rel_wr || !(card->quirks & MMC_QUIRK_BLK_NO_CMD23) ||
>> - ? ? ? ? ?do_data_tag)) {
>> + ? ? ? ? ?do_data_tag || (card->ext_csd.max_context_id > 0))) {
>
> card->ext_csd.max_context_id can be non-zero even if host has not enabled
> MMC_CAP2_CONTEXT cap. ?So you should add proper check here before tagging
> the context id.
>

I will set max_context_id to 0 in mmc_read_ext_csd if MMC_CAP2_CONTEXT
is not set as mentioned above.

>> + ? ? ? ? ? ? int context_id = (req->context &&
>> + ? ? ? ? ? ? ? ? ? ? card->ext_csd.max_context_id) ?
>> + ? ? ? ? ? ? ? ? ? ? (req->context % card->ext_csd.max_context_id + 1) :
>> + ? ? ? ? ? ? ? ? ? ? 0;
>> ? ? ? ? ? ? ? brq->sbc.opcode = MMC_SET_BLOCK_COUNT;
>> ? ? ? ? ? ? ? brq->sbc.arg = brq->data.blocks |
>> ? ? ? ? ? ? ? ? ? ? ? (do_rel_wr ? (1 << 31) : 0) |
>> - ? ? ? ? ? ? ? ? ? ? (do_data_tag ? (1 << 29) : 0);
>> + ? ? ? ? ? ? ? ? ? ? (do_data_tag ? (1 << 29) : 0) |
>> + ? ? ? ? ? ? ? ? ? ? (!do_data_tag ? (context_id << 25) : 0);
>> ? ? ? ? ? ? ? brq->sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
>> ? ? ? ? ? ? ? brq->mrq.sbc = &brq->sbc;
>> ? ? ? }
>> @@ -1440,6 +1454,23 @@ static int mmc_blk_issue_rq(struct mmc_queue
>> *mq, struct request *req)
>> ? ? ? ? ? ? ? ? ? ? ? mmc_blk_issue_rw_rq(mq, NULL);
>> ? ? ? ? ? ? ? ret = mmc_blk_issue_flush(mq, req);
>> ? ? ? } else {
>> + ? ? ? ? ? ? if (req && (req->cmd_flags & REQ_SYNC) &&
>> + ? ? ? ? ? ? ? ? ? ? req->context && card->ext_csd.max_context_id) {
>> + ? ? ? ? ? ? ? ? ? ? int context_cfg_id = (req->context) ?
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? req->context % card-
>> >ext_csd.max_context_id : 0;
>> + ? ? ? ? ? ? ? ? ? ? /*
>> + ? ? ? ? ? ? ? ? ? ? ?* The SYNC command is a synchronization point
>> from
>> + ? ? ? ? ? ? ? ? ? ? ?* file system. The relevent context is sync'ed here
>> + ? ? ? ? ? ? ? ? ? ? ?* to ensure that the data written in the open
>> context
>> + ? ? ? ? ? ? ? ? ? ? ?* are saved reliably in non-volatile media
>> + ? ? ? ? ? ? ? ? ? ? ?*/
>> + ? ? ? ? ? ? ? ? ? ? if (card->host->areq)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? mmc_blk_issue_rw_rq(mq, NULL);
>> + ? ? ? ? ? ? ? ? ? ? mmc_sync_context(card, context_cfg_id);
>> + ? ? ? ? ? ? ? ? ? ? /* this write will go without context to ensure
>> + ? ? ? ? ? ? ? ? ? ? ? ?that it is reliably written */
>
> Use multiline comments format.

Ok

>
>> + ? ? ? ? ? ? ? ? ? ? req->context = 0;
>> + ? ? ? ? ? ? }
>> ? ? ? ? ? ? ? ret = mmc_blk_issue_rw_rq(mq, req);
>> ? ? ? }
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
>> ba821fe..728145a 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -2262,6 +2262,78 @@ int mmc_cache_ctrl(struct mmc_host *host, u8
>> enable) ?} ?EXPORT_SYMBOL(mmc_cache_ctrl);
>>
>> +/*
>> + * Synchronize a context by first closing the context and then
>> + * opening it
>> + */
>> +int mmc_sync_context(struct mmc_card *card, int context_id) {
>> + ? ? int err = 0;
>> +
>> + ? ? err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> + ? ? ? ? ? ? EXT_CSD_CONTEXT_CONF + context_id,
>> + ? ? ? ? ? ? 0x0, card->ext_csd.generic_cmd6_time);
>> +
>> + ? ? if (err)
>> + ? ? ? ? ? ? return err;
>> +
>> + ? ? err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> + ? ? ? ? ? ? EXT_CSD_CONTEXT_CONF + context_id,
>
> Value of context_id passed to this function can range from 1 to 15. If it's
> 15 then (EXT_CSD_CONTEXT_CONF + context_id) would be 52 which is beyond the
> CONTEXT_CONF [51:37].
> So basically the index value passed to mmc_switch() should be
> (EXT_CSD_CONTEXT_CONF + context_id - 1).
>

In mmc_blk_issue_rq, we do a "req->context %
card->ext_csd.max_context_id", which will set the context_cfg_id
between 0 and 14. We use this context_cfg_id for the mmc_sync_context.
It should fall within the range.

But in the call from mmc_flush_contexts, I should set the loop i from
0 to (max_context_id-1).

May be, it is good idea to call the parameter context_id as
context_cfg_id within mmc_sync_context to avoid confusion.

>> + ? ? ? ? ? ? 0x3, card->ext_csd.generic_cmd6_time);
>> +
>> + ? ? return err;
>> +}
>> +EXPORT_SYMBOL(mmc_sync_context);
>> +
>> +int mmc_flush_contexts(struct mmc_card *card) {
>> + ? ? int i;
>
> One line space after the declaration missing.
>

Ok

>> + ? ? for (i = 1; i <= card->ext_csd.max_context_id; i++) {
>> + ? ? ? ? ? ? int err = mmc_sync_context(card, i);
>> + ? ? ? ? ? ? if (err)
>> + ? ? ? ? ? ? ? ? ? ? return err;
>
> Shouldn't we try to flush other contexts even if flush for other context
> fails?
>

Ok. Will do. But this function will still return an error.

>> + ? ? }
>> + ? ? return 0;
>> +}
>> +EXPORT_SYMBOL(mmc_flush_contexts);
>> +
>> +/*
>> + * Initialize all the MMC contexts in read-write and non-LU mode ?*/
>> +int mmc_init_context(struct mmc_card *card) {
>> + ? ? int i, err = 0;
>> +
>> + ? ? for (i = 0; i < card->ext_csd.max_context_id; i++) {
>> + ? ? ? ? ? ? err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? EXT_CSD_CONTEXT_CONF + i,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x3, card-
>> >ext_csd.generic_cmd6_time);
>
> I would suggest a separate function which takes the context index,
> activation mode, large unit context as it's arguments and writes the
> activation mode in appropriate context configuration offset. So in future if
> we want to add enable other contexts, we can use the same function.
>
> Also, let's have macro for 0x03 to indicate what it really means (read/write
> context).
>

Ok

>> + ? ? ? ? ? ? if (err) {
>> + ? ? ? ? ? ? ? ? ? ? pr_warning("%s: Activating of context %d failed
>> [%x]\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?mmc_hostname(card->host), i, err);
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? }
>> + ? ? }
>> +
>> + ? ? if (!err)
>> + ? ? ? ? ? ? return 0;
>> +
>> + ? ? /*
>> + ? ? ?* Close the opened contexts
>> + ? ? ?*/
>
> Why should we close the already opened contexts? Rather we can still use the
> opened contexts. Right?
>

Ok. We can set max_context_id to (1 + the context id we could
successfully opened).


>> + ? ? for (i = i-1; i >= 0; i--) {
>> + ? ? ? ? ? ? int err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? EXT_CSD_CONTEXT_CONF + i,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x0, card-
>> >ext_csd.generic_cmd6_time);
>> + ? ? ? ? ? ? if (err)
>> + ? ? ? ? ? ? ? ? ? ? pr_warning("%s: Closing of context %d failed
>> [%x]\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?mmc_hostname(card->host), i, err);
>> + ? ? }
>> +
>> + ? ? return err;
>> +}
>> +EXPORT_SYMBOL(mmc_init_context);
>> +
>> ?#ifdef CONFIG_PM
>>
>> ?/**
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index
>> 54df5ad..84bec43 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -533,6 +533,20 @@ static int mmc_read_ext_csd(struct mmc_card *card,
>> u8 *ext_csd)
>> ? ? ? ? ? ? ? } else {
>> ? ? ? ? ? ? ? ? ? ? ? card->ext_csd.data_tag_unit_size = 0;
>> ? ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? /* LU size in 512 byt sectors */
>
> Typo here. s/byt/byte
>
>> + ? ? ? ? ? ? card->ext_csd.lu_size =
>> + ? ? ? ? ? ? ? ? ? ? (ext_csd[EXT_CSD_LARGE_UNIT_SIZE_M1] + 1) *
>> 0x800;
>
> In this patch you are not using the large unit context anywhere then better
> not to read the large unit size as part of this patch. Let it be completely
> handled separately when someone adds the large unit context in future.
>

Ok

>> +
>> + ? ? ? ? ? ? card->ext_csd.max_context_id =
>> + ? ? ? ? ? ? ? ? ? ? ext_csd[EXT_CSD_CONTEXT_CAPABILITIES] & 0x0f;
>
> This is what spec says: " The mandatory minimum value for this field is 5
> plus the default ID #0 (MAX_CONTEXT_ID shall be 5 or higher) ". Can you add
> check for this? And if card violates this, we at least should print a
> warning.
>

Ok

>> +
>> + ? ? ? ? ? ? if (card->ext_csd.max_context_id >
>> MAX_MMC_CONTEXT_ID) {
>
> Is this check required? You are already &ing with 0x0f which means the
> max_context_id will never be more than MAX_MMC_CONTEXT_ID (which is 15).
>
>> + ? ? ? ? ? ? ? ? ? ? pr_warning("%s: card has invalid number of contexts
>> [%d]\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? mmc_hostname(card->host),
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? card->ext_csd.max_context_id);
>> + ? ? ? ? ? ? ? ? ? ? card->ext_csd.max_context_id = 0;
>> + ? ? ? ? ? ? }
>> ? ? ? }
>>
>> ?out:
>> @@ -1267,6 +1281,20 @@ static int mmc_init_card(struct mmc_host *host,
>> u32 ocr,
>> ? ? ? ? ? ? ? }
>> ? ? ? }
>>
>> + ? ? if (host->caps2 & MMC_CAP2_CONTEXT) {
>> + ? ? ? ? ? ? if (card->ext_csd.max_context_id > 0) {
>> + ? ? ? ? ? ? ? ? ? ? err = mmc_init_context(card);
>> + ? ? ? ? ? ? ? ? ? ? if (err && err != -EBADMSG)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? goto free_card;
>> + ? ? ? ? ? ? ? ? ? ? if (err) {
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? pr_warning("%s: failed to activate context
>> (%x)\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? mmc_hostname(card->host),
>> err);
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? card->ext_csd.max_context_id = 0;
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? err = 0;
>> + ? ? ? ? ? ? ? ? ? ? }
>> + ? ? ? ? ? ? }
>> + ? ? }
>> +
>> ? ? ? if (!oldcard)
>> ? ? ? ? ? ? ? host->card = card;
>>
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h index
>> 629b823..adf2c84 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -74,6 +74,8 @@ struct mmc_ext_csd {
>> ? ? ? unsigned int ? ? ? ? ? ?hpi_cmd; ? ? ? ? ? ? ? ?/* cmd used as HPI
> */
>> ? ? ? unsigned int ? ? ? ? ? ?data_sector_size; ? ? ? /* 512 bytes or 4KB
> */
>> ? ? ? unsigned int ? ? ? ? ? ?data_tag_unit_size; ? ? /* DATA TAG UNIT
> size */
>> + ? ? unsigned int ? ? ? ? ? ?lu_size;
>> + ? ? unsigned int ? ? ? ? ? ?max_context_id;
>> ? ? ? unsigned int ? ? ? ? ? ?boot_ro_lock; ? ? ? ? ? /* ro lock support
> */
>> ? ? ? bool ? ? ? ? ? ? ? ? ? ?boot_ro_lockable;
>> ? ? ? u8 ? ? ? ? ? ? ? ? ? ? ?raw_partition_support; ?/* 160 */
>> @@ -184,6 +186,8 @@ struct sdio_func_tuple;
>> ?#define MMC_NUM_PHY_PARTITION ? ? ? ?6
>> ?#define MAX_MMC_PART_NAME_LEN ? ? ? ?20
>>
>> +#define MAX_MMC_CONTEXT_ID ? ? ? ? ? 15
>> +
>> ?/*
>> ? * MMC Physical partitions
>> ? */
>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h index
>> 1b431c7..a4e6bc9 100644
>> --- a/include/linux/mmc/core.h
>> +++ b/include/linux/mmc/core.h
>> @@ -179,6 +179,10 @@ extern int mmc_try_claim_host(struct mmc_host
>> *host);
>>
>> ?extern int mmc_flush_cache(struct mmc_card *);
>>
>> +extern int mmc_sync_context(struct mmc_card *card, int context_id);
>> +extern int mmc_flush_contexts(struct mmc_card *card); extern int
>> +mmc_init_context(struct mmc_card *card);
>> +
>> ?extern int mmc_detect_card_removed(struct mmc_host *host);
>>
>> ?/**
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index
>> 0707d22..688348f 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -233,6 +233,7 @@ struct mmc_host {
>> ?#define MMC_CAP2_NO_SLEEP_CMD ? ? ? ?(1 << 4) ? ? ? ?/* Don't allow sleep
>> command */
>> ?#define MMC_CAP2_HS200_1_8V_SDR ? ? ?(1 << 5) ? ? ? ?/* can support */
>> ?#define MMC_CAP2_HS200_1_2V_SDR ? ? ?(1 << 6) ? ? ? ?/* can support */
>> +#define MMC_CAP2_CONTEXT ? ? (1<<7) ?/* Context ID supported */
>> ?#define MMC_CAP2_HS200 ? ? ? ? ? ? ? (MMC_CAP2_HS200_1_8V_SDR | \
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?MMC_CAP2_HS200_1_2V_SDR)
>> ?#define MMC_CAP2_BROKEN_VOLTAGE ? ? ?(1 << 7) ? ? ? ?/* Use the broken
>> voltage */
>> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h index
>> b822a2c..8f8d958 100644
>> --- a/include/linux/mmc/mmc.h
>> +++ b/include/linux/mmc/mmc.h
>> @@ -274,6 +274,7 @@ struct _mmc_csd {
>> ?#define EXT_CSD_FLUSH_CACHE ? ? ? ? ?32 ? ? ?/* W */
>> ?#define EXT_CSD_CACHE_CTRL ? ? ? ? ? 33 ? ? ?/* R/W */
>> ?#define EXT_CSD_POWER_OFF_NOTIFICATION ? ? ? 34 ? ? ?/* R/W */
>> +#define EXT_CSD_CONTEXT_CONF ? ? ? ? 37 ? ? ?/* R/W */
>> ?#define EXT_CSD_DATA_SECTOR_SIZE ? ? 61 ? ? ?/* R */
>> ?#define EXT_CSD_GP_SIZE_MULT ? ? ? ? 143 ? ? /* R/W */
>> ?#define EXT_CSD_PARTITION_ATTRIBUTE ?156 ? ? /* R/W */
>> @@ -316,6 +317,8 @@ struct _mmc_csd {
>> ?#define EXT_CSD_POWER_OFF_LONG_TIME ?247 ? ? /* RO */
>> ?#define EXT_CSD_GENERIC_CMD6_TIME ? ?248 ? ? /* RO */
>> ?#define EXT_CSD_CACHE_SIZE ? ? ? ? ? 249 ? ? /* RO, 4 bytes */
>> +#define EXT_CSD_LARGE_UNIT_SIZE_M1 ? 495 ? ? /* RO */
>> +#define EXT_CSD_CONTEXT_CAPABILITIES 496 ? ? /* RO */
>> ?#define EXT_CSD_TAG_UNIT_SIZE ? ? ? ? ? ? ? ?498 ? ? /* RO */
>> ?#define EXT_CSD_DATA_TAG_SUPPORT ? ? 499 ? ? /* RO */
>> ?#define EXT_CSD_HPI_FEATURES ? ? ? ? 503 ? ? /* RO */
>> --
>> 1.7.4.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the
>> body of a message to [email protected] More majordomo info at
>> http://vger.kernel.org/majordomo-info.html
>

2012-05-22 04:22:44

by Saugata Das

[permalink] [raw]
Subject: Re: [RFC 1/3] block: Context support

Hi Group,

Will you please provide some feedback on the following RFC patches,

[RFC 1/3] block: Context support
[RFC 2/3] ext4: Context support


Regards
Saugata


On 16 May 2012 21:00, Saugata Das <[email protected]> wrote:
> From: Saugata Das <[email protected]>
>
> On eMMC and UFS devices there is a new feature of setting context with each
> read or write. The idea is to classify the data from different files and
> apply the realibility on the complete file instead of individual writes,
> which helps in performance. A new address space operation has been a added
> to get the context from file system and set up the bi_context field in bio.
> Then we need to ensure that bio from different contexts are not merged. The
> context is then passed to the underlying driver as part of the read or write
> request. Since the number of MMC contexts is limited, multiple file system
> contexts are mapped to single MMC context.
>
> Signed-off-by: Saugata Das <[email protected]>
> ---
> ?block/blk-core.c ? ? ? ? ? ?| ? ?1 +
> ?block/blk-merge.c ? ? ? ? ? | ? ?3 +++
> ?fs/mpage.c ? ? ? ? ? ? ? ? ?| ? 12 ++++++++++++
> ?include/linux/blk_types.h ? | ? ?1 +
> ?include/linux/blkdev.h ? ? ?| ? ?1 +
> ?include/linux/buffer_head.h | ? ?2 ++
> ?include/linux/fs.h ? ? ? ? ?| ? ?1 +
> ?7 files changed, 21 insertions(+), 0 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 1f61b74..274e05d 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1309,6 +1309,7 @@ void init_request_from_bio(struct request *req, struct bio *bio)
> ? ? ? ?req->errors = 0;
> ? ? ? ?req->__sector = bio->bi_sector;
> ? ? ? ?req->ioprio = bio_prio(bio);
> + ? ? ? req->context = bio->bi_context;
> ? ? ? ?blk_rq_bio_prep(req->q, req, bio);
> ?}
>
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 160035f..ed70d56 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -497,6 +497,9 @@ bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
> ? ? ? ?if (bio_integrity(bio) != blk_integrity_rq(rq))
> ? ? ? ? ? ? ? ?return false;
>
> + ? ? ? if (bio->bi_context != rq->bio->bi_context)
> + ? ? ? ? ? ? ? return false;
> +
> ? ? ? ?return true;
> ?}
>
> diff --git a/fs/mpage.c b/fs/mpage.c
> index 0face1c..4889842 100644
> --- a/fs/mpage.c
> +++ b/fs/mpage.c
> @@ -293,6 +293,12 @@ alloc_new:
> ? ? ? ? ? ? ? ? ? ? ? ?goto confused;
> ? ? ? ?}
>
> + ? ? ? if (page && page->mapping && page->mapping->a_ops &&
> + ? ? ? ? ? ? ? page->mapping->a_ops->get_context)
> + ? ? ? ? ? ? ? bio->bi_context = page->mapping->a_ops->get_context(page);
> + ? ? ? else
> + ? ? ? ? ? ? ? bio->bi_context = 0;
> +
> ? ? ? ?length = first_hole << blkbits;
> ? ? ? ?if (bio_add_page(bio, page, length, 0) < length) {
> ? ? ? ? ? ? ? ?bio = mpage_bio_submit(READ, bio);
> @@ -581,6 +587,12 @@ alloc_new:
> ? ? ? ? ? ? ? ? ? ? ? ?goto confused;
> ? ? ? ?}
>
> + ? ? ? if (page && page->mapping && page->mapping->a_ops &&
> + ? ? ? ? ? ? ? page->mapping->a_ops->get_context)
> + ? ? ? ? ? ? ? bio->bi_context = page->mapping->a_ops->get_context(page);
> + ? ? ? else
> + ? ? ? ? ? ? ? bio->bi_context = 0;
> +
> ? ? ? ?/*
> ? ? ? ? * Must try to add the page before marking the buffer clean or
> ? ? ? ? * the confused fail path above (OOM) will be very confused when
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index 4053cbd..f3ac448 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -42,6 +42,7 @@ struct bio {
>
> ? ? ? ?unsigned short ? ? ? ? ?bi_vcnt; ? ? ? ?/* how many bio_vec's */
> ? ? ? ?unsigned short ? ? ? ? ?bi_idx; ? ? ? ? /* current index into bvl_vec */
> + ? ? ? unsigned long ? ? ? ? ? bi_context; ? ? /* context of this bio */
>
> ? ? ? ?/* Number of segments in this BIO after
> ? ? ? ? * physical address coalescing is performed.
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 2aa2466..0dd9a08 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -167,6 +167,7 @@ struct request {
> ? ? ? ?struct list_head timeout_list;
> ? ? ? ?unsigned int timeout;
> ? ? ? ?int retries;
> + ? ? ? unsigned long context; ?/* context of this request */
>
> ? ? ? ?/*
> ? ? ? ? * completion callback.
> diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
> index 13bba17..0776564 100644
> --- a/include/linux/buffer_head.h
> +++ b/include/linux/buffer_head.h
> @@ -72,6 +72,8 @@ struct buffer_head {
> ? ? ? ?struct list_head b_assoc_buffers; /* associated with another mapping */
> ? ? ? ?struct address_space *b_assoc_map; ? ? ?/* mapping this buffer is
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? associated with */
> + ? ? ? unsigned long ? b_context; /* context for this buffer within the
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? storage device */
> ? ? ? ?atomic_t b_count; ? ? ? ? ? ? ? /* users using this buffer_head */
> ?};
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 8de6755..4b379d8 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -626,6 +626,7 @@ struct address_space_operations {
> ? ? ? ?int (*is_partially_uptodate) (struct page *, read_descriptor_t *,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned long);
> ? ? ? ?int (*error_remove_page)(struct address_space *, struct page *);
> + ? ? ? int (*get_context)(struct page *);
> ?};
>
> ?extern const struct address_space_operations empty_aops;
> --
> 1.7.4.3
>