memstick: Alternative approach to proposed fixes

memstick: Alternative approach to proposed fixes

[Alex Dubov]

As I was not able to convince myself that profound changes proposed by Maxim
are really necessary, I propose to follow a much milder path with this update
round (as I outlined in my previous emails).

In this small patchset, I fix a couple of small omissions as well as introduce
support for "extended command" MSPro transfer method (currently enabled for
JMicron, but I'll clear the TI host for this functionality soon). I expect the
current solution to scale trivially for HG and XC transfer methods (I don't
have the spec for the later yet).

I'm still investigating possible issues with JMicron adapter. As JMicron is
known to follow a rather uneven path with their chip revisions, revision
dependent quirks may need to be introduced in the driver if I'm unable to
confirm Maxim's findings.

[PATCH 1/4] memstick: avert possible race condition between idr_pre_get and idr_get_new

[Alex Dubov]

From: Alex Dubov <oakad@yahoo.com>

Signed-off-by: Alex Dubov <oakad@yahoo.com>
---
 drivers/memstick/core/memstick.c    |   18 +++++++++++-------
 drivers/memstick/core/mspro_block.c |    6 ++++--
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
index c00fe82..17c556c 100644
--- a/drivers/memstick/core/memstick.c
+++ b/drivers/memstick/core/memstick.c
@@ -510,14 +510,18 @@ int memstick_add_host(struct memstick_host *host)
 {
 	int rc;
 
-	if (!idr_pre_get(&memstick_host_idr, GFP_KERNEL))
-		return -ENOMEM;
+	while (1) {
+		if (!idr_pre_get(&memstick_host_idr, GFP_KERNEL))
+			return -ENOMEM;
 
-	spin_lock(&memstick_host_lock);
-	rc = idr_get_new(&memstick_host_idr, host, &host->id);
-	spin_unlock(&memstick_host_lock);
-	if (rc)
-		return rc;
+		spin_lock(&memstick_host_lock);
+		rc = idr_get_new(&memstick_host_idr, host, &host->id);
+		spin_unlock(&memstick_host_lock);
+		if (!rc)
+			break;
+		else if (rc != -EAGAIN)
+			return rc;
+	}
 
 	dev_set_name(&host->dev, "memstick%u", host->id);
 
diff --git a/drivers/memstick/core/mspro_block.c b/drivers/memstick/core/mspro_block.c
index 02362ec..a167938 100644
--- a/drivers/memstick/core/mspro_block.c
+++ b/drivers/memstick/core/mspro_block.c
@@ -1206,10 +1206,12 @@ static int mspro_block_init_disk(struct memstick_dev *card)
 
 	msb->page_size = be16_to_cpu(sys_info->unit_size);
 
-	if (!idr_pre_get(&mspro_block_disk_idr, GFP_KERNEL))
+	mutex_lock(&mspro_block_disk_lock);
+	if (!idr_pre_get(&mspro_block_disk_idr, GFP_KERNEL)) {
+		mutex_unlock(&mspro_block_disk_lock);
 		return -ENOMEM;
+	}
 
-	mutex_lock(&mspro_block_disk_lock);
 	rc = idr_get_new(&mspro_block_disk_idr, card, &disk_id);
 	mutex_unlock(&mspro_block_disk_lock);
 
-- 
1.6.5.1

[PATCH 2/4] memstick: remove mspro_block_mutex

[Alex Dubov]

From: Alex Dubov <oakad@yahoo.com>

mspro_block_mutex is identical in scope to mspro_block_disk_lock and therefore
unnecessary.

Signed-off-by: Alex Dubov <oakad@yahoo.com>
---
 drivers/memstick/core/mspro_block.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/drivers/memstick/core/mspro_block.c b/drivers/memstick/core/mspro_block.c
index a167938..b11b2b8 100644
--- a/drivers/memstick/core/mspro_block.c
+++ b/drivers/memstick/core/mspro_block.c
@@ -23,7 +23,6 @@
 
 #define DRIVER_NAME "mspro_block"
 
-static DEFINE_MUTEX(mspro_block_mutex);
 static int major;
 module_param(major, int, 0644);
 
@@ -181,7 +180,6 @@ static int mspro_block_bd_open(struct block_device *bdev, fmode_t mode)
 	struct mspro_block_data *msb = disk->private_data;
 	int rc = -ENXIO;
 
-	mutex_lock(&mspro_block_mutex);
 	mutex_lock(&mspro_block_disk_lock);
 
 	if (msb && msb->card) {
@@ -193,7 +191,6 @@ static int mspro_block_bd_open(struct block_device *bdev, fmode_t mode)
 	}
 
 	mutex_unlock(&mspro_block_disk_lock);
-	mutex_unlock(&mspro_block_mutex);
 
 	return rc;
 }
@@ -225,11 +222,7 @@ static int mspro_block_disk_release(struct gendisk *disk)
 
 static int mspro_block_bd_release(struct gendisk *disk, fmode_t mode)
 {
-	int ret;
-	mutex_lock(&mspro_block_mutex);
-	ret = mspro_block_disk_release(disk);
-	mutex_unlock(&mspro_block_mutex);
-	return ret;
+	return mspro_block_disk_release(disk);
 }
 
 static int mspro_block_bd_getgeo(struct block_device *bdev,
-- 
1.6.5.1

[PATCH 3/4] memstick: factor out transfer initiating functionality in mspro_block.c

[Alex Dubov]

From: Alex Dubov <oakad@yahoo.com>

Apart from currently used standard memstick data transfer method, Sony
introduced several newer ones, to uncover full bandwidth/capacity of its
Pro, HG and XC media formats. This patch lays a foundation to enable
those methods as made possible by host/media capabilities.

As a side effect of this patch, mspro_block_read_attributes became more
streamlined and readable.

Signed-off-by: Alex Dubov <oakad@yahoo.com>
---
 drivers/memstick/core/mspro_block.c |  136 +++++++++++++++++++----------------
 1 files changed, 74 insertions(+), 62 deletions(-)

diff --git a/drivers/memstick/core/mspro_block.c b/drivers/memstick/core/mspro_block.c
index b11b2b8..ef7679b 100644
--- a/drivers/memstick/core/mspro_block.c
+++ b/drivers/memstick/core/mspro_block.c
@@ -159,6 +159,13 @@ struct mspro_block_data {
 	int                   (*mrq_handler)(struct memstick_dev *card,
 					     struct memstick_request **mrq);
 
+
+	/* Default request setup function for data access method preferred by
+	 * this host instance.
+	 */
+	void                  (*setup_transfer)(struct memstick_dev *card,
+						u64 offset, size_t length);
+
 	struct attribute_group attr_group;
 
 	struct scatterlist    req_sg[MSPRO_BLOCK_MAX_SEGS];
@@ -656,14 +663,43 @@ has_int_reg:
 	}
 }
 
+/*** Transfer setup functions for different access methods. ***/
+
+/** Setup data transfer request for SET_CMD TPC with arguments in card
+ *  registers.
+ *
+ *  @card    Current media instance
+ *  @offset  Target data offset in bytes
+ *  @length  Required transfer length in bytes.
+ */
+static void h_mspro_block_setup_cmd(struct memstick_dev *card, u64 offset,
+				    size_t length)
+{
+	struct mspro_block_data *msb = memstick_get_drvdata(card);
+	struct mspro_param_register param = {
+		.system = msb->system,
+		.data_count = cpu_to_be16((uint16_t)(length / msb->page_size)),
+		/* ISO C90 warning precludes direct initialization for now. */
+		.data_address = 0,
+		.tpc_param = 0
+	};
+
+	do_div(offset, msb->page_size);
+	param.data_address = cpu_to_be32((uint32_t)offset);
+
+	card->next_request = h_mspro_block_req_init;
+	msb->mrq_handler = h_mspro_block_transfer_data;
+	memstick_init_req(&card->current_mrq, MS_TPC_WRITE_REG,
+			  &param, sizeof(param));
+}
+
 /*** Data transfer ***/
 
 static int mspro_block_issue_req(struct memstick_dev *card, int chunk)
 {
 	struct mspro_block_data *msb = memstick_get_drvdata(card);
-	sector_t t_sec;
+	u64 t_off;
 	unsigned int count;
-	struct mspro_param_register param;
 
 try_again:
 	while (chunk) {
@@ -678,30 +714,17 @@ try_again:
 			continue;
 		}
 
-		t_sec = blk_rq_pos(msb->block_req) << 9;
-		sector_div(t_sec, msb->page_size);
-
+		t_off = blk_rq_pos(msb->block_req);
+		t_off <<= 9;
 		count = blk_rq_bytes(msb->block_req);
-		count /= msb->page_size;
 
-		param.system = msb->system;
-		param.data_count = cpu_to_be16(count);
-		param.data_address = cpu_to_be32((uint32_t)t_sec);
-		param.tpc_param = 0;
+		msb->setup_transfer(card, t_off, count);
 
 		msb->data_dir = rq_data_dir(msb->block_req);
 		msb->transfer_cmd = msb->data_dir == READ
 				    ? MSPRO_CMD_READ_DATA
 				    : MSPRO_CMD_WRITE_DATA;
 
-		dev_dbg(&card->dev, "data transfer: cmd %x, "
-			"lba %x, count %x\n", msb->transfer_cmd,
-			be32_to_cpu(param.data_address), count);
-
-		card->next_request = h_mspro_block_req_init;
-		msb->mrq_handler = h_mspro_block_transfer_data;
-		memstick_init_req(&card->current_mrq, MS_TPC_WRITE_REG,
-				  &param, sizeof(param));
 		memstick_new_req(card->host);
 		return 0;
 	}
@@ -956,18 +979,16 @@ try_again:
 static int mspro_block_read_attributes(struct memstick_dev *card)
 {
 	struct mspro_block_data *msb = memstick_get_drvdata(card);
-	struct mspro_param_register param = {
-		.system = msb->system,
-		.data_count = cpu_to_be16(1),
-		.data_address = 0,
-		.tpc_param = 0
-	};
 	struct mspro_attribute *attr = NULL;
 	struct mspro_sys_attr *s_attr = NULL;
 	unsigned char *buffer = NULL;
 	int cnt, rc, attr_count;
-	unsigned int addr;
-	unsigned short page_count;
+	/* While normally physical device offsets, represented here by
+	 * attr_offset and attr_len will be of large numeric types, we can be
+	 * sure, that attributes are close enough to the beginning of the
+	 * device, to save ourselves some trouble.
+	 */
+	unsigned int addr, attr_offset = 0, attr_len = msb->page_size;
 
 	attr = kmalloc(msb->page_size, GFP_KERNEL);
 	if (!attr)
@@ -980,10 +1001,8 @@ static int mspro_block_read_attributes(struct memstick_dev *card)
 	msb->data_dir = READ;
 	msb->transfer_cmd = MSPRO_CMD_READ_ATRB;
 
-	card->next_request = h_mspro_block_req_init;
-	msb->mrq_handler = h_mspro_block_transfer_data;
-	memstick_init_req(&card->current_mrq, MS_TPC_WRITE_REG, &param,
-			  sizeof(param));
+	msb->setup_transfer(card, attr_offset, attr_len);
+
 	memstick_new_req(card->host);
 	wait_for_completion(&card->mrq_complete);
 	if (card->current_mrq.error) {
@@ -1014,13 +1033,12 @@ static int mspro_block_read_attributes(struct memstick_dev *card)
 	}
 	msb->attr_group.name = "media_attributes";
 
-	buffer = kmalloc(msb->page_size, GFP_KERNEL);
+	buffer = kmalloc(attr_len, GFP_KERNEL);
 	if (!buffer) {
 		rc = -ENOMEM;
 		goto out_free_attr;
 	}
-	memcpy(buffer, (char *)attr, msb->page_size);
-	page_count = 1;
+	memcpy(buffer, (char *)attr, attr_len);
 
 	for (cnt = 0; cnt < attr_count; ++cnt) {
 		s_attr = kzalloc(sizeof(struct mspro_sys_attr), GFP_KERNEL);
@@ -1031,9 +1049,10 @@ static int mspro_block_read_attributes(struct memstick_dev *card)
 
 		msb->attr_group.attrs[cnt] = &s_attr->dev_attr.attr;
 		addr = be32_to_cpu(attr->entries[cnt].address);
-		rc = be32_to_cpu(attr->entries[cnt].size);
+		s_attr->size = be32_to_cpu(attr->entries[cnt].size);
 		dev_dbg(&card->dev, "adding attribute %d: id %x, address %x, "
-			"size %x\n", cnt, attr->entries[cnt].id, addr, rc);
+			"size %x\n", cnt, attr->entries[cnt].id, addr,
+			s_attr->size);
 		s_attr->id = attr->entries[cnt].id;
 		if (mspro_block_attr_name(s_attr->id))
 			snprintf(s_attr->name, sizeof(s_attr->name), "%s",
@@ -1047,57 +1066,47 @@ static int mspro_block_read_attributes(struct memstick_dev *card)
 		s_attr->dev_attr.attr.mode = S_IRUGO;
 		s_attr->dev_attr.show = mspro_block_attr_show(s_attr->id);
 
-		if (!rc)
+		if (!s_attr->size)
 			continue;
 
-		s_attr->size = rc;
-		s_attr->data = kmalloc(rc, GFP_KERNEL);
+		s_attr->data = kmalloc(s_attr->size, GFP_KERNEL);
 		if (!s_attr->data) {
 			rc = -ENOMEM;
 			goto out_free_buffer;
 		}
 
-		if (((addr / msb->page_size)
-		     == be32_to_cpu(param.data_address))
-		    && (((addr + rc - 1) / msb->page_size)
-			== be32_to_cpu(param.data_address))) {
+		if (((addr / msb->page_size) == (attr_offset / msb->page_size))
+		    && (((addr + s_attr->size - 1) / msb->page_size)
+			== (attr_offset / msb->page_size))) {
 			memcpy(s_attr->data, buffer + addr % msb->page_size,
-			       rc);
+			       s_attr->size);
 			continue;
 		}
 
-		if (page_count <= (rc / msb->page_size)) {
+		attr_offset = (addr / msb->page_size) * msb->page_size;
+
+		if ((attr_offset + attr_len) < (addr + s_attr->size)) {
 			kfree(buffer);
-			page_count = (rc / msb->page_size) + 1;
-			buffer = kmalloc(page_count * msb->page_size,
-					 GFP_KERNEL);
+			attr_len = (((addr + s_attr->size) / msb->page_size)
+				    + 1 ) * msb->page_size - attr_offset;
+			buffer = kmalloc(attr_len, GFP_KERNEL);
 			if (!buffer) {
 				rc = -ENOMEM;
 				goto out_free_attr;
 			}
 		}
 
-		param.system = msb->system;
-		param.data_count = cpu_to_be16((rc / msb->page_size) + 1);
-		param.data_address = cpu_to_be32(addr / msb->page_size);
-		param.tpc_param = 0;
-
-		sg_init_one(&msb->req_sg[0], buffer,
-			    be16_to_cpu(param.data_count) * msb->page_size);
+		sg_init_one(&msb->req_sg[0], buffer, attr_len);
 		msb->seg_count = 1;
 		msb->current_seg = 0;
 		msb->current_page = 0;
 		msb->data_dir = READ;
 		msb->transfer_cmd = MSPRO_CMD_READ_ATRB;
 
-		dev_dbg(&card->dev, "reading attribute pages %x, %x\n",
-			be32_to_cpu(param.data_address),
-			be16_to_cpu(param.data_count));
+		dev_dbg(&card->dev, "reading attribute range %x, %x\n",
+			attr_offset, attr_len);
 
-		card->next_request = h_mspro_block_req_init;
-		msb->mrq_handler = h_mspro_block_transfer_data;
-		memstick_init_req(&card->current_mrq, MS_TPC_WRITE_REG,
-				  (char *)&param, sizeof(param));
+		msb->setup_transfer(card, attr_offset, attr_len);
 		memstick_new_req(card->host);
 		wait_for_completion(&card->mrq_complete);
 		if (card->current_mrq.error) {
@@ -1105,7 +1114,8 @@ static int mspro_block_read_attributes(struct memstick_dev *card)
 			goto out_free_buffer;
 		}
 
-		memcpy(s_attr->data, buffer + addr % msb->page_size, rc);
+		memcpy(s_attr->data, buffer + addr % msb->page_size,
+		       s_attr->size);
 	}
 
 	rc = 0;
@@ -1123,6 +1133,8 @@ static int mspro_block_init_card(struct memstick_dev *card)
 	int rc = 0;
 
 	msb->system = MEMSTICK_SYS_SERIAL;
+	msb->setup_transfer = h_mspro_block_setup_cmd;
+
 	card->reg_addr.r_offset = offsetof(struct mspro_register, status);
 	card->reg_addr.r_length = sizeof(struct ms_status_register);
 	card->reg_addr.w_offset = offsetof(struct mspro_register, param);
-- 
1.6.5.1

[PATCH 4/4] memstick: add support for MSPro specific data transfer method

[Alex Dubov]

From: Alex Dubov <oakad@yahoo.com>

This patch enables faster data transfer to/from MSPro media for enabled hosts
(currently only jmb38x_ms). Other supported hosts and transfer methods (HG,
XC) to follow.

Signed-off-by: Alex Dubov <oakad@yahoo.com>
---
 drivers/memstick/core/mspro_block.c |   63 +++++++++++++++++++++++++++++++----
 drivers/memstick/host/jmb38x_ms.c   |    2 +-
 include/linux/memstick.h            |    7 ++++
 3 files changed, 64 insertions(+), 8 deletions(-)

diff --git a/drivers/memstick/core/mspro_block.c b/drivers/memstick/core/mspro_block.c
index ef7679b..ae66634 100644
--- a/drivers/memstick/core/mspro_block.c
+++ b/drivers/memstick/core/mspro_block.c
@@ -588,12 +588,10 @@ static int h_mspro_block_transfer_data(struct memstick_dev *card,
 		return mspro_block_complete_req(card, (*mrq)->error);
 
 	switch ((*mrq)->tpc) {
-	case MS_TPC_WRITE_REG:
-		memstick_init_req(*mrq, MS_TPC_SET_CMD, &msb->transfer_cmd, 1);
-		(*mrq)->need_card_int = 1;
-		return 0;
+	case MS_TPC_EX_SET_CMD:
 	case MS_TPC_SET_CMD:
 		t_val = (*mrq)->int_reg;
+		msb->mrq_handler = h_mspro_block_transfer_data;
 		memstick_init_req(*mrq, MS_TPC_GET_INT, NULL, 1);
 		if (msb->caps & MEMSTICK_CAP_AUTO_GET_INT)
 			goto has_int_reg;
@@ -663,6 +661,20 @@ has_int_reg:
 	}
 }
 
+static int h_mspro_block_invoke_cmd(struct memstick_dev *card,
+				    struct memstick_request **mrq)
+{
+	struct mspro_block_data *msb = memstick_get_drvdata(card);
+
+	if ((*mrq)->error)
+		return mspro_block_complete_req(card, (*mrq)->error);
+
+	card->next_request = h_mspro_block_transfer_data;
+	memstick_init_req(*mrq, MS_TPC_SET_CMD, &msb->transfer_cmd, 1);
+	(*mrq)->need_card_int = 1;
+	return 0;
+}
+
 /*** Transfer setup functions for different access methods. ***/
 
 /** Setup data transfer request for SET_CMD TPC with arguments in card
@@ -688,11 +700,37 @@ static void h_mspro_block_setup_cmd(struct memstick_dev *card, u64 offset,
 	param.data_address = cpu_to_be32((uint32_t)offset);
 
 	card->next_request = h_mspro_block_req_init;
-	msb->mrq_handler = h_mspro_block_transfer_data;
+	msb->mrq_handler = h_mspro_block_invoke_cmd;
 	memstick_init_req(&card->current_mrq, MS_TPC_WRITE_REG,
 			  &param, sizeof(param));
 }
 
+/** Setup data transfer request for EX_SET_CMD TPC with arguments in TPC packet.
+ *
+ *  @card    Current media instance
+ *  @offset  Target data offset in bytes
+ *  @length  Required transfer length in bytes.
+ */
+static void h_mspro_block_ex_setup_cmd(struct memstick_dev *card, u64 offset,
+				       size_t length)
+{
+	struct mspro_block_data *msb = memstick_get_drvdata(card);
+	struct mspro_ex_cmd_param param = {
+		.command = msb->transfer_cmd,
+		.data_count = cpu_to_be16((uint16_t)(length / msb->page_size)),
+		/* ISO C90 warning precludes direct initialization for now. */
+		.data_address = 0
+	};
+
+	do_div(offset, msb->page_size);
+	param.data_address = cpu_to_be32((uint32_t)offset);
+
+	card->next_request = h_mspro_block_req_init;
+	msb->mrq_handler = h_mspro_block_transfer_data;
+	memstick_init_req(&card->current_mrq, MS_TPC_EX_SET_CMD,
+			  &param, sizeof(param));
+}
+
 /*** Data transfer ***/
 
 static int mspro_block_issue_req(struct memstick_dev *card, int chunk)
@@ -718,13 +756,12 @@ try_again:
 		t_off <<= 9;
 		count = blk_rq_bytes(msb->block_req);
 
-		msb->setup_transfer(card, t_off, count);
-
 		msb->data_dir = rq_data_dir(msb->block_req);
 		msb->transfer_cmd = msb->data_dir == READ
 				    ? MSPRO_CMD_READ_DATA
 				    : MSPRO_CMD_WRITE_DATA;
 
+		msb->setup_transfer(card, t_off, count);
 		memstick_new_req(card->host);
 		return 0;
 	}
@@ -1170,6 +1207,18 @@ static int mspro_block_init_card(struct memstick_dev *card)
 	dev_dbg(&card->dev, "card r/w status %d\n", msb->read_only ? 0 : 1);
 
 	msb->page_size = 512;
+
+	/* All MSPro media devices are expected to support extended command
+	 * transfer mode. Faulty or old adapters may chose not to advertise
+	 * this capability and rely on somewhat slower default mode.
+	 *
+	 * Even more advanced HG and XC modes of operation, when implemented,
+	 * will have to be selected after media attribute parsing, to ensure
+	 * full compatibility.
+	 */
+	if (msb->caps & MEMSTICK_CAP_EX_CMD)
+		msb->setup_transfer = h_mspro_block_ex_setup_cmd;
+
 	rc = mspro_block_read_attributes(card);
 	if (rc)
 		return rc;
diff --git a/drivers/memstick/host/jmb38x_ms.c b/drivers/memstick/host/jmb38x_ms.c
index f2b894c..cc67726 100644
--- a/drivers/memstick/host/jmb38x_ms.c
+++ b/drivers/memstick/host/jmb38x_ms.c
@@ -848,7 +848,7 @@ static struct memstick_host *jmb38x_ms_alloc_host(struct jmb38x_ms *jm, int cnt)
 	msh->request = jmb38x_ms_submit_req;
 	msh->set_param = jmb38x_ms_set_param;
 
-	msh->caps = MEMSTICK_CAP_PAR4 | MEMSTICK_CAP_PAR8;
+	msh->caps = MEMSTICK_CAP_PAR4 | MEMSTICK_CAP_PAR8 | MEMSTICK_CAP_EX_CMD;
 
 	setup_timer(&host->timer, jmb38x_ms_abort, (unsigned long)msh);
 
diff --git a/include/linux/memstick.h b/include/linux/memstick.h
index 690c35a..ad75e17 100644
--- a/include/linux/memstick.h
+++ b/include/linux/memstick.h
@@ -105,6 +105,12 @@ struct mspro_param_register {
 	unsigned char  tpc_param;
 } __attribute__((packed));
 
+struct mspro_ex_cmd_param {
+	unsigned char command;
+	__be16        data_count;
+	__be32        data_address;
+} __packed;
+
 struct mspro_io_info_register {
 	unsigned char version;
 	unsigned char io_category;
@@ -279,6 +285,7 @@ struct memstick_host {
 #define MEMSTICK_CAP_AUTO_GET_INT  1
 #define MEMSTICK_CAP_PAR4          2
 #define MEMSTICK_CAP_PAR8          4
+#define MEMSTICK_CAP_EX_CMD        8
 
 	struct work_struct  media_checker;
 	struct device       dev;
-- 
1.6.5.1

Re: memstick: Alternative approach to proposed fixes

[Maxim Levitsky]

On Wed, 2010-10-27 at 04:41 +1100, Alex Dubov wrote:
> As I was not able to convince myself that profound changes proposed by Maxim
> are really necessary, I propose to follow a much milder path with this update
> round (as I outlined in my previous emails).
It is really sad to see that patchset.


> 
> In this small patchset, I fix a couple of small omissions as well as introduce
> support for "extended command" MSPro transfer method (currently enabled for
> JMicron, but I'll clear the TI host for this functionality soon). I expect the
> current solution to scale trivially for HG and XC transfer methods (I don't
> have the spec for the later yet).
Indeed small.
All the changes in this patchset are really minor and optional.
I could have dropped them.


I did much more that that.
I made the memstick code readable so not only you could understand it
I also wrote ms_block.c while adding common support.
I made your code use that support.

It is really sad that you are doing that.

Re: memstick: Alternative approach to proposed fixes

[Maxim Levitsky]

On Wed, 2010-10-27 at 00:55 +0200, Maxim Levitsky wrote:
> On Wed, 2010-10-27 at 04:41 +1100, Alex Dubov wrote:
> > As I was not able to convince myself that profound changes proposed by Maxim
> > are really necessary, I propose to follow a much milder path with this update
> > round (as I outlined in my previous emails).
> It is really sad to see that patchset.
> 
> 
> > 
> > In this small patchset, I fix a couple of small omissions as well as introduce
> > support for "extended command" MSPro transfer method (currently enabled for
> > JMicron, but I'll clear the TI host for this functionality soon). I expect the
> > current solution to scale trivially for HG and XC transfer methods (I don't
> > have the spec for the later yet).
> Indeed small.
> All the changes in this patchset are really minor and optional.
> I could have dropped them.
> 
> 
> I did much more that that.
> I made the memstick code readable so not only you could understand it
> I also wrote ms_block.c while adding common support.
> I made your code use that support.
> 
> It is really sad that you are doing that.
To futher add to that you are just resending my patch #7,
which is just a minor bugfix, and add evem more complexity to mspro when
you add CMDEX.
Why you made it host dependant?, could I ask.

To be honest the ms subsystem is one of most obscure places I have ever
seen.
It took me much more time to write the driver that what it took me to
write xD driver.

Alex, I really really hope you will accept my work.


Best regards,
	Maxim Levitsky

Re: memstick: Alternative approach to proposed fixes

[Maxim Levitsky]

On Wed, 2010-10-27 at 01:07 +0200, Maxim Levitsky wrote:
> On Wed, 2010-10-27 at 00:55 +0200, Maxim Levitsky wrote:
> > On Wed, 2010-10-27 at 04:41 +1100, Alex Dubov wrote:
> > > As I was not able to convince myself that profound changes proposed by Maxim
> > > are really necessary, I propose to follow a much milder path with this update
> > > round (as I outlined in my previous emails).
> > It is really sad to see that patchset.
> > 
> > 
> > > 
> > > In this small patchset, I fix a couple of small omissions as well as introduce
> > > support for "extended command" MSPro transfer method (currently enabled for
> > > JMicron, but I'll clear the TI host for this functionality soon). I expect the
> > > current solution to scale trivially for HG and XC transfer methods (I don't
> > > have the spec for the later yet).
> > Indeed small.
> > All the changes in this patchset are really minor and optional.
> > I could have dropped them.
> > 
> > 
> > I did much more that that.
> > I made the memstick code readable so not only you could understand it
> > I also wrote ms_block.c while adding common support.
> > I made your code use that support.
> > 
> > It is really sad that you are doing that.
> To futher add to that you are just resending my patch #7,
> which is just a minor bugfix, and add evem more complexity to mspro when
> you add CMDEX.
> Why you made it host dependant?, could I ask.
Abd besides CMDEX is really minor thing.
I even initially thought not to add it.


> 
> To be honest the ms subsystem is one of most obscure places I have ever
> seen.
> It took me much more time to write the driver that what it took me to
> write xD driver.
> 
> Alex, I really really hope you will accept my work.
> 
> 
> Best regards,
> 	Maxim Levitsky
> 

Re: memstick: Alternative approach to proposed fixes

[Alex Dubov]

> All the changes in this patchset are really minor and
> optional.
> I could have dropped them.

These are the only substantial fixes you patchset has. The rest of the
large submission is your unilateral decision to rewrite the whole thing
almost from scratch. I proposed, multiple times, to fix the small things
first, then discuss the long term evolution.

> 
> 
> I did much more that that.
> I made the memstick code readable so not only you could
> understand it
> I also wrote ms_block.c while adding common support.
> I made your code use that support.
> 
> It is really sad that you are doing that.
> 
> 
> 
>