LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 2.6.19] mmc: Fix handling of response types in imxmmc and tifm drivers
@ 2007-01-04 15:04 Philip Langdale
  2007-01-07 17:34 ` Pavel Pisa
  0 siblings, 1 reply; 7+ messages in thread
From: Philip Langdale @ 2007-01-04 15:04 UTC (permalink / raw)
  To: linux-kernel, Pierre Ossman; +Cc: Alex Dubov, Pavel Pisa

This change depends on my SDHC patch and fixes a bug that was revealed during the
development of that patch. The R6 response type should be identical to R1 (and R7)
but was incorrectly defined differently. Fixing the R6 definition breaks assumptions
in these two drivers that response type flags are unique. Pierre and Alex both
believe that treating R6 and R7 as R1 will be sufficient. ie: The controllers do
not care about the differences between them. Due to lack of hardware, I have done
no testing.

Signed-off-by: Philip Langdale <philipl@overt.org>
Cc: Alex Dubov <oakad@yahoo.com>
Cc: Pavel Pisa <ppisa@pikron.com>
---
 drivers/mmc/imxmmc.c    |    3 ---
 drivers/mmc/omap.c      |    2 +-
 drivers/mmc/pxamci.c    |    2 +-
 drivers/mmc/tifm_sd.c   |    3 ---
 include/linux/mmc/mmc.h |    2 +-
 5 files changed, 3 insertions(+), 9 deletions(-)

diff -ur linux-2.6.19-sdhc/drivers/mmc/imxmmc.c linux-2.6.19-rspfix/drivers/mmc/imxmmc.c
--- linux-2.6.19-sdhc/drivers/mmc/imxmmc.c	2007-01-04 06:52:12.000000000 -0800
+++ linux-2.6.19-rspfix/drivers/mmc/imxmmc.c	2007-01-04 06:53:16.000000000 -0800
@@ -351,9 +351,6 @@
 	case MMC_RSP_R3: /* short */
 		cmdat |= CMD_DAT_CONT_RESPONSE_FORMAT_R3;
 		break;
-	case MMC_RSP_R6: /* short CRC */
-		cmdat |= CMD_DAT_CONT_RESPONSE_FORMAT_R6;
-		break;
 	default:
 		break;
 	}
diff -ur linux-2.6.19-sdhc/drivers/mmc/omap.c linux-2.6.19-rspfix/drivers/mmc/omap.c
--- linux-2.6.19-sdhc/drivers/mmc/omap.c	2007-01-04 06:52:12.000000000 -0800
+++ linux-2.6.19-rspfix/drivers/mmc/omap.c	2007-01-04 05:46:24.000000000 -0800
@@ -206,7 +206,7 @@
 	/* Our hardware needs to know exact type */
 	switch (RSP_TYPE(mmc_resp_type(cmd))) {
 	case RSP_TYPE(MMC_RSP_R1):
-		/* resp 1, resp 1b */
+		/* resp 1, 1b, 6, 7 */
 		resptype = 1;
 		break;
 	case RSP_TYPE(MMC_RSP_R2):
diff -ur linux-2.6.19-sdhc/drivers/mmc/pxamci.c linux-2.6.19-rspfix/drivers/mmc/pxamci.c
--- linux-2.6.19-sdhc/drivers/mmc/pxamci.c	2007-01-04 06:52:12.000000000 -0800
+++ linux-2.6.19-rspfix/drivers/mmc/pxamci.c	2007-01-04 05:46:36.000000000 -0800
@@ -171,7 +171,7 @@

 #define RSP_TYPE(x)	((x) & ~(MMC_RSP_BUSY|MMC_RSP_OPCODE))
 	switch (RSP_TYPE(mmc_resp_type(cmd))) {
-	case RSP_TYPE(MMC_RSP_R1): /* r1, r1b, r6 */
+	case RSP_TYPE(MMC_RSP_R1): /* r1, r1b, r6, r7 */
 		cmdat |= CMDAT_RESP_SHORT;
 		break;
 	case RSP_TYPE(MMC_RSP_R3):
diff -ur linux-2.6.19-sdhc/drivers/mmc/tifm_sd.c linux-2.6.19-rspfix/drivers/mmc/tifm_sd.c
--- linux-2.6.19-sdhc/drivers/mmc/tifm_sd.c	2007-01-04 06:52:12.000000000 -0800
+++ linux-2.6.19-rspfix/drivers/mmc/tifm_sd.c	2007-01-04 06:53:38.000000000 -0800
@@ -173,9 +173,6 @@
 	case MMC_RSP_R3:
 		rc |= TIFM_MMCSD_RSP_R3;
 		break;
-	case MMC_RSP_R6:
-		rc |= TIFM_MMCSD_RSP_R6;
-		break;
 	default:
 		BUG();
 	}
diff -ur linux-2.6.19-sdhc/include/linux/mmc/mmc.h linux-2.6.19-rspfix/include/linux/mmc/mmc.h
--- linux-2.6.19-sdhc/include/linux/mmc/mmc.h	2007-01-04 06:51:09.000000000 -0800
+++ linux-2.6.19-rspfix/include/linux/mmc/mmc.h	2007-01-04 05:41:49.000000000 -0800
@@ -42,7 +42,7 @@
 #define MMC_RSP_R1B	(MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE|MMC_RSP_BUSY)
 #define MMC_RSP_R2	(MMC_RSP_PRESENT|MMC_RSP_136|MMC_RSP_CRC)
 #define MMC_RSP_R3	(MMC_RSP_PRESENT)
-#define MMC_RSP_R6	(MMC_RSP_PRESENT|MMC_RSP_CRC)
+#define MMC_RSP_R6	(MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
 #define MMC_RSP_R7	(MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)

 #define mmc_resp_type(cmd)	((cmd)->flags & (MMC_RSP_PRESENT|MMC_RSP_136|MMC_RSP_CRC|MMC_RSP_BUSY|MMC_RSP_OPCODE))

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2.6.19] mmc: Fix handling of response types in imxmmc and tifm drivers
  2007-01-04 15:04 [PATCH 2.6.19] mmc: Fix handling of response types in imxmmc and tifm drivers Philip Langdale
@ 2007-01-07 17:34 ` Pavel Pisa
  2007-01-10 19:02   ` Pierre Ossman
  0 siblings, 1 reply; 7+ messages in thread
From: Pavel Pisa @ 2007-01-07 17:34 UTC (permalink / raw)
  To: Philip Langdale; +Cc: linux-kernel, Pierre Ossman, Alex Dubov, Sascha Hauer

Hello Philip,

On Thursday 04 January 2007 16:04, Philip Langdale wrote:
> This change depends on my SDHC patch and fixes a bug that was revealed
> during the development of that patch. The R6 response type should be
> identical to R1 (and R7) but was incorrectly defined differently. Fixing
> the R6 definition breaks assumptions in these two drivers that response
> type flags are unique. Pierre and Alex both believe that treating R6 and R7
> as R1 will be sufficient. ie: The controllers do not care about the
> differences between them. Due to lack of hardware, I have done no testing.

I have tested your patch.
Kernel builds. I have not found much time for testing.
But I would not like to block changes and I am going
for next week to project meeting in Spain, so there is
my reply.

I have 2.6.19 + realtime-patches rt14 on the hand.
I have been able to mount and use some cards, but it
I have observed some problems probably related to timing
when I have tried to change CPU frequency.

I need to find time to do more checking on vanilla and RT kernels
when I return. I have some ideas what could be enhanced to ensure
better MX1 SDHC cards recognition under RT kernels. I am not sure,
what causes other seen problems, but I have observed these things
on RT even without your patch.

Conclusion: I knowledge your patch and admit, that I need to
find time for my homeworks.

Best wishes

               Pavel Pisa



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2.6.19] mmc: Fix handling of response types in imxmmc and tifm drivers
  2007-01-07 17:34 ` Pavel Pisa
@ 2007-01-10 19:02   ` Pierre Ossman
  2007-01-19  1:50     ` mmc: correct semantics of the mmc_host_remove Alex Dubov
  0 siblings, 1 reply; 7+ messages in thread
From: Pierre Ossman @ 2007-01-10 19:02 UTC (permalink / raw)
  To: Pavel Pisa; +Cc: Philip Langdale, linux-kernel, Alex Dubov, Sascha Hauer

Pavel Pisa wrote:
> I have tested your patch.
> Kernel builds. I have not found much time for testing.
> But I would not like to block changes and I am going
> for next week to project meeting in Spain, so there is
> my reply.
>
> I have 2.6.19 + realtime-patches rt14 on the hand.
> I have been able to mount and use some cards, but it
> I have observed some problems probably related to timing
> when I have tried to change CPU frequency.
>
>   

>From what I gather, the imx driver/hw is a bit funky in several areas.

My plan with this patch is -mm for this release, and merged during the next.

Rgds

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org


^ permalink raw reply	[flat|nested] 7+ messages in thread

* mmc: correct semantics of the mmc_host_remove
  2007-01-10 19:02   ` Pierre Ossman
@ 2007-01-19  1:50     ` Alex Dubov
  2007-01-19  8:47       ` Pierre Ossman
  0 siblings, 1 reply; 7+ messages in thread
From: Alex Dubov @ 2007-01-19  1:50 UTC (permalink / raw)
  To: Pierre Ossman; +Cc: linux-kernel

Greetings.

It appears to me that under certain circumstances mmc layer will issue requests to the host after
mmc_host_remove returns. This happens, for example, in tifm_sd driver because mmc_host may be
removed mid-transfer, as the socket shall be freed for possible reuse by different media type.
Currently, the only solution is to sleep a little somewhere after mmc_remove_host but before
mmc_free_host. I think the correct way to handle the situation is to ensure that mmc_host is never
accessed by the mmc layer after mmc_remove_host returns.


I think a easy way to handle this is to modify __mmc_claim_host to fail if the mmc_host is marked
for removal (this implies that its return value should be checked on use, which is not currently
the case everywhere). This way, mmc_host_remove can claim host, mark it as "dead" and then return
safely knowing that nobody will send any more requests to the host. 

Some questions:
1. Will this suffice for the task? 
2. Are there any reasons not to do this?
3. Is it possible to replace the fancy locking loop in the __mmc_claim_host with mutex based
locking (mutex does the same thing, isn't it)?



 
____________________________________________________________________________________
Get your own web address.  
Have a HUGE year through Yahoo! Small Business.
http://smallbusiness.yahoo.com/domains/?p=BESTDEAL

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: mmc: correct semantics of the mmc_host_remove
  2007-01-19  1:50     ` mmc: correct semantics of the mmc_host_remove Alex Dubov
@ 2007-01-19  8:47       ` Pierre Ossman
  2007-01-20  3:46         ` Alex Dubov
  0 siblings, 1 reply; 7+ messages in thread
From: Pierre Ossman @ 2007-01-19  8:47 UTC (permalink / raw)
  To: Alex Dubov; +Cc: linux-kernel

Alex Dubov wrote:
> Greetings.
>
> It appears to me that under certain circumstances mmc layer will issue requests to the host after
> mmc_host_remove returns. This happens, for example, in tifm_sd driver because mmc_host may be
> removed mid-transfer, as the socket shall be freed for possible reuse by different media type.
> Currently, the only solution is to sleep a little somewhere after mmc_remove_host but before
> mmc_free_host. I think the correct way to handle the situation is to ensure that mmc_host is never
> accessed by the mmc layer after mmc_remove_host returns.
>
>   

That shouldn't be possible. Are you using the block queue fixes I wrote?
Otherwise you will get problems like this.

Basically, when you call mmc_host_remove(), it will remove all card
devices. That shouldn't complete until all card drivers have released
control of the card. At that point there is no one else accessing the
device. If you see something else, then we have a bug somewhere.

Rgds

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: mmc: correct semantics of the mmc_host_remove
  2007-01-19  8:47       ` Pierre Ossman
@ 2007-01-20  3:46         ` Alex Dubov
  2007-01-20 10:21           ` Pierre Ossman
  0 siblings, 1 reply; 7+ messages in thread
From: Alex Dubov @ 2007-01-20  3:46 UTC (permalink / raw)
  To: Pierre Ossman; +Cc: linux-kernel

> That shouldn't be possible. Are you using the block queue fixes I wrote?
> Otherwise you will get problems like this.
> 
> Basically, when you call mmc_host_remove(), it will remove all card
> devices. That shouldn't complete until all card drivers have released
> control of the card. At that point there is no one else accessing the
> device. If you see something else, then we have a bug somewhere.
> 
Indeed, I may be out of sync on this. Simply, I have this rather ugly hack in the tifm_sd remove
code which I was forced to add because of the issue in question.
I'll do some tests with newer kernels then.



 
____________________________________________________________________________________
Now that's room service!  Choose from over 150,000 hotels
in 45,000 destinations on Yahoo! Travel to find your fit.
http://farechase.yahoo.com/promo-generic-14795097

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: mmc: correct semantics of the mmc_host_remove
  2007-01-20  3:46         ` Alex Dubov
@ 2007-01-20 10:21           ` Pierre Ossman
  0 siblings, 0 replies; 7+ messages in thread
From: Pierre Ossman @ 2007-01-20 10:21 UTC (permalink / raw)
  To: Alex Dubov; +Cc: linux-kernel

Alex Dubov wrote:
>
> Indeed, I may be out of sync on this. Simply, I have this rather ugly hack in the tifm_sd remove
> code which I was forced to add because of the issue in question.
> I'll do some tests with newer kernels then.
>
>   

Please do. And if you see a problem, please try to figure out who it is
accessing the device.

Rgds

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2007-01-20 10:21 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-01-04 15:04 [PATCH 2.6.19] mmc: Fix handling of response types in imxmmc and tifm drivers Philip Langdale
2007-01-07 17:34 ` Pavel Pisa
2007-01-10 19:02   ` Pierre Ossman
2007-01-19  1:50     ` mmc: correct semantics of the mmc_host_remove Alex Dubov
2007-01-19  8:47       ` Pierre Ossman
2007-01-20  3:46         ` Alex Dubov
2007-01-20 10:21           ` Pierre Ossman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).