Skip to contentSkip to conversation

Driver for Ricoh cardreader

PATCH Superseded Maxim Levitsky 2 participants 12 messages Aug 3, 2010 – Aug 6, 2010

Subsystems (2)
Sony memorystick Ricoh R5C592 memorystick driver

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Maxim Levitsky

author maintainer
#0 lore raw

cover Driver for Ricoh cardreader

Hi,

Here is the driver for the reader I have.
Plus one fix for hangs on unclean removal introduced my changes in block core.

This is just writen and debbuged, but works very well now.
I don't know if possible, but I would like to see that in 2.6.36, because this is new driver, so no posiibility of regressions.

Best regards,
Maxim Levitsky

author maintainer
#1 lore raw

PATCH 1/2 MEMSTICK: fix hangs on unexpected device removal in mspro_blk

Now that del_gendisk syncs, we better start rejecting requests right away.

Signed-off-by Maxim Levitsky <[email protected]>
---
 drivers/memstick/core/mspro_block.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/memstick/core/mspro_block.c +3 -2
index 8327e24..8d27c13 100644
@@ -1330,13 +1330,14 @@ static void mspro_block_remove(struct memstick_dev *card)
 	struct mspro_block_data *msb = memstick_get_drvdata(card);
 	unsigned long flags;
 
-	del_gendisk(msb->disk);
-	dev_dbg(&card->dev, "mspro block remove\n");
 	spin_lock_irqsave(&msb->q_lock, flags);
 	msb->eject = 1;
 	blk_start_queue(msb->queue);
 	spin_unlock_irqrestore(&msb->q_lock, flags);
 
+	del_gendisk(msb->disk);
+	dev_dbg(&card->dev, "mspro block remove\n");
+
 	blk_cleanup_queue(msb->queue);
 	msb->queue = NULL;
Signature · 1 line
1.7.0.4
maintainer
#2 lore raw

Re: [PATCH 1/2] MEMSTICK: fix hangs on unexpected device removal in mspro_blk

Quoted 2 lines · Maxim Levitsky

Now that del_gendisk syncs, we better
start rejecting requests right away.

I don't quite see why this change is needed. My understanding is, user accessible interface should be marked as removed as early, as possible.

Quoted 39 lines
Signed-off-by Maxim Levitsky <[email protected]>
---
 drivers/memstick/core/mspro_block.c |    5
+++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/memstick/core/mspro_block.c +0 -2
index 8327e24..8d27c13 100644
@@ -1330,13 +1330,14 @@ static void
 mspro_block_remove(struct memstick_dev *card)
     struct mspro_block_data *msb =
 memstick_get_drvdata(card);
     unsigned long flags;
 
-    del_gendisk(msb->disk);
-    dev_dbg(&card->dev, "mspro block
 remove\n");
     spin_lock_irqsave(&msb->q_lock,
 flags);
     msb->eject = 1;
     blk_start_queue(msb->queue);
    

spin_unlock_irqrestore(&msb->q_lock, flags);

+    del_gendisk(msb->disk);
+    dev_dbg(&card->dev, "mspro block
remove\n");
+
    blk_cleanup_queue(msb->queue);
    msb->queue = NULL;

Signature · 1 line
1.7.0.4
author maintainer
#3 lore raw

Re: [PATCH 1/2] MEMSTICK: fix hangs on unexpected device removal in mspro_blk

Quoted 8 lines · Alex Dubov
Quoted 2 lines · Maxim Levitsky

Now that del_gendisk syncs, we better
start rejecting requests right away.

I don't quite see why this change is needed. My understanding is, user accessible interface should be marked as removed as early, as possible.

The problem here is that del_gendisk, syncs the device. This is new change, made after you did your drivers.

I have this problem on jMicron too (which otherwise works fine).

PS:

I have a copy of your ms_block.c.
I would would be very happy if you share with me, what problems does it still have (besides need of trivial port for changes in block system, because I want to push it upstream too.

I have MS DUO 64M to test it against.

Best regards,
Maxim Levitsky

author maintainer
#4 lore raw

Re: [PATCH 1/2] MEMSTICK: fix hangs on unexpected device removal in mspro_blk

Quoted 7 lines · Maxim Levitsky
Quoted 6 lines · Alex Dubov
Quoted 2 lines · Maxim Levitsky

Now that del_gendisk syncs, we better
start rejecting requests right away.

Quoted 7 lines
Quoted 2 lines

I don't quite see why this change is needed. My understanding is, user accessible interface should be marked as removed as early, as possible.

The problem here is that del_gendisk, syncs the device. This is new change, made after you did your drivers.

I have this problem on jMicron too (which otherwise works fine).

The problem is that card check thread explicitly calls ->stop before removing the device.
In case of mspro_blk.c that stops the request queue. Attempt to call del_gendisk with stopped request queue hangs due to syncing.

Quoted 12 lines

PS:

I have a copy of your ms_block.c.
I would would be very happy if you share with me, what problems does it still have (besides need of trivial port for changes in block system, because I want to push it upstream too.

I have MS DUO 64M to test it against.

Best regards,
Maxim Levitsky

maintainer
#5 lore raw

Re: [PATCH 1/2] MEMSTICK: fix hangs on unexpected device removal in mspro_blk

Quoted 17 lines

From: Maxim Levitsky <[email protected]>
Subject: Re: [PATCH 1/2] MEMSTICK: fix hangs on unexpected device removal in mspro_blk

Quoted 6 lines
Quoted 6 lines
Quoted 3 lines

Now that del_gendisk syncs, we better
start rejecting requests right away.

I don't quite see why this change is needed. My

understanding is, user

Quoted 1 line
Quoted 1 line

accessible interface should be marked as removed

as early, as possible.

Quoted 2 lines

The problem here is that del_gendisk, syncs the

device.

Quoted 3 lines

This is new change, made after you did your drivers.

I have this problem on jMicron too (which otherwise

Quoted 8 lines

works fine).
The problem is that card check thread explicitly calls ->stop before
removing the device.
In case of mspro_blk.c that stops the request queue. Attempt to call del_gendisk with stopped request queue hangs due to
syncing.

Well, ok. Sounds good.

Quoted 10 lines
Quoted 3 lines

I have a copy of your ms_block.c.
I would would be very happy if you share with me, what

problems does it

Quoted 1 line

still have (besides need of trivial port for changes

in block system,

Quoted 4 lines

because I want to push it upstream too.

I have MS DUO 64M to test it against.

I've got two rather different implementations of ms_block, if I remember correctly. Both suffer from random data corruptions, thanks to my inexplicable desire to write the state machine as tightly, as possible. Only the later one does the spec mandated geometry correctly - I actually wrote the first version before I've seen the spec for the first time.

author maintainer
#6 lore raw

Re: [PATCH 1/2] MEMSTICK: fix hangs on unexpected device removal in mspro_blk

Quoted 37 lines · Alex Dubov
Quoted 17 lines

From: Maxim Levitsky <[email protected]>
Subject: Re: [PATCH 1/2] MEMSTICK: fix hangs on unexpected device removal in mspro_blk

Quoted 6 lines
Quoted 6 lines
Quoted 3 lines

Now that del_gendisk syncs, we better
start rejecting requests right away.

I don't quite see why this change is needed. My

understanding is, user

Quoted 1 line
Quoted 1 line

accessible interface should be marked as removed

as early, as possible.

Quoted 2 lines

The problem here is that del_gendisk, syncs the

device.

Quoted 3 lines

This is new change, made after you did your drivers.

I have this problem on jMicron too (which otherwise

Quoted 8 lines

works fine).
The problem is that card check thread explicitly calls ->stop before
removing the device.
In case of mspro_blk.c that stops the request queue. Attempt to call del_gendisk with stopped request queue hangs due to
syncing.

Well, ok. Sounds good.

Quoted 8 lines
Quoted 3 lines

I have a copy of your ms_block.c.
I would would be very happy if you share with me, what

problems does it

Quoted 1 line

still have (besides need of trivial port for changes

in block system,

Quoted 2 lines

because I want to push it upstream too.

Quoted 8 lines
Quoted 2 lines
Quoted 2 lines

I have MS DUO 64M to test it against.

I've got two rather different implementations of ms_block, if I remember correctly. Both suffer from random data corruptions, thanks to my inexplicable desire to write the state machine as tightly, as possible. Only the later one does the spec mandated geometry correctly - I actually wrote the first version before I've seen the spec for the first time.

By 'spec mandated geometry' you mean the fact the card needs to be broken into zones, somewhat like XD?

Best regards,
Maxim Levitsky

maintainer
#7 lore raw

Re: [PATCH 1/2] MEMSTICK: fix hangs on unexpected device removal in mspro_blk

Quoted 6 lines

By 'spec mandated geometry' you mean the fact the card needs to be
broken into zones, somewhat like XD?

Yes, and in fancy fashion.

First zone should have at most 494 useful logical blocks, other zones - 496. 16 physical blocks per zone are considered local spares, 2 first blocks in zone 1 must be identical and contain factory information.

author maintainer
#8 lore raw

PATCH 2/2 MEMSTICK: Add driver for Ricoh R5C592 Card reader.

In reply to Maxim Levitsky
Signed-off-by Maxim Levitsky <[email protected]>
---
 MAINTAINERS                    |    6 +
 drivers/memstick/host/Kconfig  |   13 +
 drivers/memstick/host/Makefile |    1 +
 drivers/memstick/host/r592.c   |  920 ++++++++++++++++++++++++++++++++++++++++
 drivers/memstick/host/r592.h   |  178 ++++++++
 5 files changed, 1118 insertions(+), 0 deletions(-)
 create mode 100644 drivers/memstick/host/r592.c
 create mode 100644 drivers/memstick/host/r592.h
MAINTAINERS +6 -0
index 88fcb7c..7ebdd32 100644
@@ -4822,6 +4822,12 @@ S:	Maintained
 F:	drivers/mtd/nand/r822.c
 F:	drivers/mtd/nand/r822.h
 
+RICOH R5C592 MEMORYSTICK DRIVER
+M:	Maxim Levitsky <[email protected]>
+S:	Maintained
+F:	drivers/memstick/host/r592.c
+F:	drivers/memstick/host/r592.h
+
 RISCOM8 DRIVER
 S:	Orphan
 F:	Documentation/serial/riscom8.txt
drivers/memstick/host/Kconfig +13 -0
index 4ce5c8d..7022a2a 100644
@@ -30,3 +30,16 @@ config MEMSTICK_JMICRON_38X
 
           To compile this driver as a module, choose M here: the
 	  module will be called jmb38x_ms.
+
+config MEMSTICK_R592
+	tristate "Ricoh R5C592 MemoryStick interface support (EXPERIMENTAL)"
+	depends on EXPERIMENTAL && PCI
+
+	help
+	  Say Y here if you want to be able to access MemoryStick cards with
+	  the Ricoh R5C592 MemoryStick card reader (which is part of 5 in one
+		multifunction reader)
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called r592.
+
drivers/memstick/host/Makefile +1 -0
index 12530e4..ad63c16 100644
@@ -8,3 +8,4 @@ endif
 
 obj-$(CONFIG_MEMSTICK_TIFM_MS)		+= tifm_ms.o
 obj-$(CONFIG_MEMSTICK_JMICRON_38X)	+= jmb38x_ms.o
+obj-$(CONFIG_MEMSTICK_R592)		+= r592.o
drivers/memstick/host/r592.c new file +920 -0

920 lines not on this page · raw message

drivers/memstick/host/r592.h new file +178 -0

178 lines not on this page · raw message

Signature · 1 line
1.7.0.4
maintainer
#9 lore raw

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

I see two immediate problems with this patch:

1. On cosmetic level, custom debug macros should not be employed. Device core already have this functionality (dynamic debug levels and such). Please, use dev_dbg and friends for print-outs.

2. On a structural level, I'd rather prefer host drivers to not start their own threads. If you look at both current host implementations, they operate in callback fashion. Apart from saving some resources, this reduces the amount of problems encountered during suspend/resume and shutdown.

author maintainer
#10 lore raw

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

Quoted 5 lines · Alex Dubov

I see two immediate problems with this patch:

1. On cosmetic level, custom debug macros should not be employed. Device core already have this functionality (dynamic debug levels and such). Please, use dev_dbg and friends for print-outs.

This allows much easier control for debug.
Single module parameter is enough to adjust it.
This helps me help users.
(Eg, kernel compilation is out of question)

Quoted 5 lines

2. On a structural level, I'd rather prefer host drivers to not start their own threads. If you look at both current host implementations, they operate in callback fashion. Apart from saving some resources, this reduces the amount of problems encountered during suspend/resume and shutdown.

This isn't possible.
Hardware doesn't support interrupts on memstick bus changes, it only supports DMA done from/to internal FIFO, and DMA it only possible for 512 byte TPCs.

Best regards,
Maxim Levitsky

author maintainer
#11 lore raw

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

Quoted 13 lines · Maxim Levitsky
Quoted 5 lines · Alex Dubov

I see two immediate problems with this patch:

1. On cosmetic level, custom debug macros should not be employed. Device core already have this functionality (dynamic debug levels and such). Please, use dev_dbg and friends for print-outs.

This allows much easier control for debug.
Single module parameter is enough to adjust it.
This helps me help users.
(Eg, kernel compilation is out of question)

Quoted 1 line
Quoted 9 lines
Quoted 4 lines

2. On a structural level, I'd rather prefer host drivers to not start their own threads. If you look at both current host implementations, they operate in callback fashion. Apart from saving some resources, this reduces the amount of problems encountered during suspend/resume and shutdown.

This isn't possible.
Hardware doesn't support interrupts on memstick bus changes, it only supports DMA done from/to internal FIFO, and DMA it only possible for 512 byte TPCs.

Another question.

I see that current code ignores MEMSTICK_CAP_AUTO_GET_INT Instread mspro_blk.c enables this capability for parallel mode, assuming that hw supports it. Its true in my case, but might not be true in other cases.
I think I should fix that, right?

Also I see that you bath TPC_READ_LONG_DATA/TPC_READ_LONG_DATA Does that mean that every HW sector is larger that 512?
If so, you are doing copy on write, right?
I have small caching in my sm_ftl of last sector. It helps performance a lot.

Also I want to clarify that the only kind of interrupts supported by hw (besides usual card detection interrupt), is DMA done interrupt. Thats why I have to use thread.
Doing polling in r592_submit_req (which runs in atomic context is just cruel).
Besides, under moderate IO load, the IO thread doesn't sleep, thus there is no overhead of wake/sleep.

Best regards,
Maxim Levitsky