Skip to contentSkip to conversation

tmio_mmc: Prevents unexpected status clear

PATCH In review Yusuke Goda 4 participants 15 messages Jul 7, 2010 – Aug 27, 2010

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 tmio_mmc: Prevents unexpected status clear

This patch clears only necessary bit.

---
 drivers/mmc/host/tmio_mmc.h |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)
drivers/mmc/host/tmio_mmc.h +1 -4
index 64f7d5d..7944604 100644
@@ -82,10 +82,7 @@
 
 #define ack_mmc_irqs(host, i) \
 	do { \
-		u32 mask;\
-		mask  = sd_ctrl_read32((host), CTL_STATUS); \
-		mask &= ~((i) & TMIO_MASK_IRQ); \
-		sd_ctrl_write32((host), CTL_STATUS, mask); \
+		sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
 	} while (0)
 
 
Signature · 1 line
1.7.0
#1 lore raw
Quoted 12 lines · On Wed, 07 Jul 2010 10:59:52 +0900 Yusuke Goda <[email protected]>…

This patch clears only necessary bit.

---
 drivers/mmc/host/tmio_mmc.h |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)
drivers/mmc/host/tmio_mmc.h +0 -0
index 64f7d5d..7944604 100644

drivers/mmc/host/tmio_mmc.h line 86

Quoted 9 lines
drivers/mmc/host/tmio_mmc.h +1 -4
@@ -82,10 +82,7 @@
 
 #define ack_mmc_irqs(host, i) \
 	do { \
-		u32 mask;\
-		mask  = sd_ctrl_read32((host), CTL_STATUS); \
-		mask &= ~((i) & TMIO_MASK_IRQ); \
-		sd_ctrl_write32((host), CTL_STATUS, mask); \
+		sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
 	} while (0)

Can we have a better changelog please?

What was wrong with the old code?

How does the patch fix it?

What are the user-visible runtime effects of the bug?

(It looks like that was a pretty gross bug - how did it pass testing??)

#2 lore raw

Hi Andrew

Thank you for your comment.

Quoted 11 lines

drivers/mmc/host/tmio_mmc.h line 86

Quoted 8 lines

#define ack_mmc_irqs(host, i) \

drivers/mmc/host/tmio_mmc.h +1 -4
 	do { \
-		u32 mask;\
-		mask  = sd_ctrl_read32((host), CTL_STATUS); \
-		mask &= ~((i) & TMIO_MASK_IRQ); \
-		sd_ctrl_write32((host), CTL_STATUS, mask); \
+		sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
 	} while (0)

Can we have a better changelog please?

Quoted 7 lines

What was wrong with the old code?

How does the patch fix it?

What are the user-visible runtime effects of the bug?

(It looks like that was a pretty gross bug - how did it pass testing??)

Example
- CMD53(Single block read / Received data size : 64Byte)

1) Send CMD53
2) Receive "CMD53 response"
3) Call tmio_mmc_cmd_irq(host, status);
-- original code ----------------------------------------------------

 #define ack_mmc_irqs(host, i) \
	do { \
		u32 mask;\
		mask  = sd_ctrl_read32((host), CTL_STATUS); \
	< case 1 >
		mask &= ~((i) & TMIO_MASK_IRQ); \
	< case 2 >
		sd_ctrl_write32((host), CTL_STATUS, mask); \
	} while (0)

---------------------------------------------------------------------

TMIO_STAT_RXRDY status will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);" if TMIO_STAT_RXRDY becomes effective between "< case 1 >" and "< case 2 >".

This causes the phenomenon that a TMIO_STAT_RXRDY interrupt does not occur. When received data are small, it rarely occurs.

Thanks,
Goda

#3 lore raw
Quoted 38 lines · On Tue, 13 Jul 2010 10:16:39 +0900 Yusuke Goda <[email protected]>…

Hi Andrew

Thank you for your comment.

Quoted 11 lines

drivers/mmc/host/tmio_mmc.h line 86

Quoted 8 lines

#define ack_mmc_irqs(host, i) \

drivers/mmc/host/tmio_mmc.h +1 -4
 	do { \
-		u32 mask;\
-		mask  = sd_ctrl_read32((host), CTL_STATUS); \
-		mask &= ~((i) & TMIO_MASK_IRQ); \
-		sd_ctrl_write32((host), CTL_STATUS, mask); \
+		sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
 	} while (0)

Can we have a better changelog please?

Quoted 7 lines

What was wrong with the old code?

How does the patch fix it?

What are the user-visible runtime effects of the bug?

(It looks like that was a pretty gross bug - how did it pass testing??)

Example
- CMD53(Single block read / Received data size : 64Byte)

1) Send CMD53
2) Receive "CMD53 response"
3) Call tmio_mmc_cmd_irq(host, status);
-- original code ----------------------------------------------------

 #define ack_mmc_irqs(host, i) \
	do { \
		u32 mask;\
		mask  = sd_ctrl_read32((host), CTL_STATUS); \
	< case 1 >
		mask &= ~((i) & TMIO_MASK_IRQ); \
	< case 2 >
		sd_ctrl_write32((host), CTL_STATUS, mask); \
	} while (0)
Quoted 8 lines

---------------------------------------------------------------------

TMIO_STAT_RXRDY status will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);" if TMIO_STAT_RXRDY becomes effective between "< case 1 >" and "< case 2 >".

This causes the phenomenon that a TMIO_STAT_RXRDY interrupt does not occur. When received data are small, it rarely occurs.

OK..

But with both this patch and "tmio_mmc-revise-limit-on-data-size.patch" the changelogs fail to describe the impact of the bug upon our users. So when I sit here trying to work out whether the patches should be applied to 2.6.35 and whether they should be backported into -stable, I don't have enough information.

What are your thoughts on this?

#4 lore raw
Quoted 52 lines · On Thu, 15 Jul 2010 13:25:52 -0700 Andrew Morton <[email protected]>…
Quoted 38 lines · On Tue, 13 Jul 2010 10:16:39 +0900 Yusuke Goda <[email protected]>…

Hi Andrew

Thank you for your comment.

Quoted 11 lines

drivers/mmc/host/tmio_mmc.h line 86

Quoted 8 lines

#define ack_mmc_irqs(host, i) \

drivers/mmc/host/tmio_mmc.h +1 -4
 	do { \
-		u32 mask;\
-		mask  = sd_ctrl_read32((host), CTL_STATUS); \
-		mask &= ~((i) & TMIO_MASK_IRQ); \
-		sd_ctrl_write32((host), CTL_STATUS, mask); \
+		sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
 	} while (0)

Can we have a better changelog please?

Quoted 7 lines

What was wrong with the old code?

How does the patch fix it?

What are the user-visible runtime effects of the bug?

(It looks like that was a pretty gross bug - how did it pass testing??)

Example
- CMD53(Single block read / Received data size : 64Byte)

1) Send CMD53
2) Receive "CMD53 response"
3) Call tmio_mmc_cmd_irq(host, status);
-- original code ----------------------------------------------------

 #define ack_mmc_irqs(host, i) \
	do { \
		u32 mask;\
		mask  = sd_ctrl_read32((host), CTL_STATUS); \
	< case 1 >
		mask &= ~((i) & TMIO_MASK_IRQ); \
	< case 2 >
		sd_ctrl_write32((host), CTL_STATUS, mask); \
	} while (0)
Quoted 8 lines

---------------------------------------------------------------------

TMIO_STAT_RXRDY status will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);" if TMIO_STAT_RXRDY becomes effective between "< case 1 >" and "< case 2 >".

This causes the phenomenon that a TMIO_STAT_RXRDY interrupt does not occur. When received data are small, it rarely occurs.

OK..

Quoted 7 lines

But with both this patch and "tmio_mmc-revise-limit-on-data-size.patch" the changelogs fail to describe the impact of the bug upon our users. So when I sit here trying to work out whether the patches should be applied to 2.6.35 and whether they should be backported into -stable, I don't have enough information.

What are your thoughts on this?

Goda, do you have any more ideas on updating the changelog for this patch? It looks to me like this patch is a candidate for stable (whereas the "tmio_mmc-revise-limit-on-data-size.patch" is not, sorry about replying to that one first, I'm reading my mail backwards) because, without this patch, it's possible to miss interrupts because the ack_mmc_irqs() macro clears bit in the CTL_STATUS register that it should not do? Is that correct?

If that is the case then would this be a more appropriate changelog,

"tmio_mmc: Don't clear unhandled pending interrupts

Previously, it was possible for ack_mmc_irqs() to clear pending interrupt bits in the CTL_STATUS register, even though the interrupt handler had not been called. This was because of a race that existed when doing a read-modify-write sequence on CTL_STATUS. After the read step in this sequence, if an interrupt occurred (causing one of the bits in CTL_STATUS to be set) the write step would inadvertently clear it.

This patch eliminates this race by only writing to CTL_STATUS and clearing the interrupts that were passed as an argument to ack_mmc_irqs()."

#5 lore raw
Quoted 83 lines · On Wed, 25 Aug 2010 23:11:07 +0100 Matt Fleming <[email protected]> wrote:
Quoted 52 lines · On Thu, 15 Jul 2010 13:25:52 -0700 Andrew Morton <[email protected]>…
Quoted 38 lines · On Tue, 13 Jul 2010 10:16:39 +0900 Yusuke Goda <[email protected]>…

Hi Andrew

Thank you for your comment.

Quoted 11 lines

drivers/mmc/host/tmio_mmc.h line 86

Quoted 8 lines

#define ack_mmc_irqs(host, i) \

drivers/mmc/host/tmio_mmc.h +1 -4
 	do { \
-		u32 mask;\
-		mask  = sd_ctrl_read32((host), CTL_STATUS); \
-		mask &= ~((i) & TMIO_MASK_IRQ); \
-		sd_ctrl_write32((host), CTL_STATUS, mask); \
+		sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
 	} while (0)

Can we have a better changelog please?

Quoted 7 lines

What was wrong with the old code?

How does the patch fix it?

What are the user-visible runtime effects of the bug?

(It looks like that was a pretty gross bug - how did it pass testing??)

Example
- CMD53(Single block read / Received data size : 64Byte)

1) Send CMD53
2) Receive "CMD53 response"
3) Call tmio_mmc_cmd_irq(host, status);
-- original code ----------------------------------------------------

 #define ack_mmc_irqs(host, i) \
	do { \
		u32 mask;\
		mask  = sd_ctrl_read32((host), CTL_STATUS); \
	< case 1 >
		mask &= ~((i) & TMIO_MASK_IRQ); \
	< case 2 >
		sd_ctrl_write32((host), CTL_STATUS, mask); \
	} while (0)
Quoted 8 lines

---------------------------------------------------------------------

TMIO_STAT_RXRDY status will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);" if TMIO_STAT_RXRDY becomes effective between "< case 1 >" and "< case 2 >".

This causes the phenomenon that a TMIO_STAT_RXRDY interrupt does not occur. When received data are small, it rarely occurs.

OK..

Quoted 7 lines

But with both this patch and "tmio_mmc-revise-limit-on-data-size.patch" the changelogs fail to describe the impact of the bug upon our users. So when I sit here trying to work out whether the patches should be applied to 2.6.35 and whether they should be backported into -stable, I don't have enough information.

What are your thoughts on this?

Goda, do you have any more ideas on updating the changelog for this patch? It looks to me like this patch is a candidate for stable (whereas the "tmio_mmc-revise-limit-on-data-size.patch" is not, sorry about replying to that one first, I'm reading my mail backwards) because, without this patch, it's possible to miss interrupts because the ack_mmc_irqs() macro clears bit in the CTL_STATUS register that it should not do? Is that correct?

If that is the case then would this be a more appropriate changelog,

"tmio_mmc: Don't clear unhandled pending interrupts

Previously, it was possible for ack_mmc_irqs() to clear pending interrupt bits in the CTL_STATUS register, even though the interrupt handler had not been called. This was because of a race that existed when doing a read-modify-write sequence on CTL_STATUS. After the read step in this sequence, if an interrupt occurred (causing one of the bits in CTL_STATUS to be set) the write step would inadvertently clear it.

Quoted 3 lines

This patch eliminates this race by only writing to CTL_STATUS and clearing the interrupts that were passed as an argument to ack_mmc_irqs()."

hm, I seem to have secretly dropped this patch as well.

Oh well. I restored it as tmio_mmc-dont-clear-unhandled-pending-interrupts.patch and tagged it for a -stable backport. Unless I hear otherwise I'll send it in to Linus when we return from Brazil a couple of weeks from now.

#6 lore raw

Hi Matt, Andrew

Quoted 87 lines · Andrew Morton
Quoted 80 lines · On Wed, 25 Aug 2010 23:11:07 +0100 Matt Fleming <[email protected]> wrote:
Quoted 50 lines · On Thu, 15 Jul 2010 13:25:52 -0700 Andrew Morton <[email protected]>…
Quoted 37 lines · On Tue, 13 Jul 2010 10:16:39 +0900 Yusuke Goda <[email protected]>…

Hi Andrew

Thank you for your comment.

Quoted 10 lines

drivers/mmc/host/tmio_mmc.h line 86

Quoted 8 lines

#define ack_mmc_irqs(host, i) \

drivers/mmc/host/tmio_mmc.h +1 -4
 	do { \
-		u32 mask;\
-		mask  = sd_ctrl_read32((host), CTL_STATUS); \
-		mask &= ~((i) & TMIO_MASK_IRQ); \
-		sd_ctrl_write32((host), CTL_STATUS, mask); \
+		sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
 	} while (0)

Can we have a better changelog please?

Quoted 7 lines

What was wrong with the old code?

How does the patch fix it?

What are the user-visible runtime effects of the bug?

(It looks like that was a pretty gross bug - how did it pass testing??)

Example
- CMD53(Single block read / Received data size : 64Byte)

1) Send CMD53
2) Receive "CMD53 response"
3) Call tmio_mmc_cmd_irq(host, status);
-- original code ----------------------------------------------------

 #define ack_mmc_irqs(host, i) \
	do { \
		u32 mask;\
		mask  = sd_ctrl_read32((host), CTL_STATUS); \
	< case 1 >
		mask &= ~((i) & TMIO_MASK_IRQ); \
	< case 2 >
		sd_ctrl_write32((host), CTL_STATUS, mask); \
	} while (0)
Quoted 8 lines

---------------------------------------------------------------------

TMIO_STAT_RXRDY status will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);" if TMIO_STAT_RXRDY becomes effective between "< case 1 >" and "< case 2 >".

This causes the phenomenon that a TMIO_STAT_RXRDY interrupt does not occur. When received data are small, it rarely occurs.

OK..

Quoted 7 lines

But with both this patch and "tmio_mmc-revise-limit-on-data-size.patch" the changelogs fail to describe the impact of the bug upon our users. So when I sit here trying to work out whether the patches should be applied to 2.6.35 and whether they should be backported into -stable, I don't have enough information.

What are your thoughts on this?

Goda, do you have any more ideas on updating the changelog for this patch? It looks to me like this patch is a candidate for stable (whereas the "tmio_mmc-revise-limit-on-data-size.patch" is not, sorry about replying to that one first, I'm reading my mail backwards) because, without this patch, it's possible to miss interrupts because the ack_mmc_irqs() macro clears bit in the CTL_STATUS register that it should not do? Is that correct?

If that is the case then would this be a more appropriate changelog,

"tmio_mmc: Don't clear unhandled pending interrupts

Previously, it was possible for ack_mmc_irqs() to clear pending interrupt bits in the CTL_STATUS register, even though the interrupt handler had not been called. This was because of a race that existed when doing a read-modify-write sequence on CTL_STATUS. After the read step in this sequence, if an interrupt occurred (causing one of the bits in CTL_STATUS to be set) the write step would inadvertently clear it.

Quoted 3 lines

This patch eliminates this race by only writing to CTL_STATUS and clearing the interrupts that were passed as an argument to ack_mmc_irqs()."

Quoted 6 lines

hm, I seem to have secretly dropped this patch as well.

Oh well. I restored it as tmio_mmc-dont-clear-unhandled-pending-interrupts.patch and tagged it for a -stable backport. Unless I hear otherwise I'll send it in to Linus when we return from Brazil a couple of weeks from now.

Thank you for your actions.
I agree to new changelog.

In fact, I contributed the patch which changed changelog.
http://www.spinics.net/lists/linux-mmc/msg02623.html
http://www.spinics.net/lists/linux-mmc/msg02624.html
However, these will not be necessary.

Thanks,
Goda

#7 lore raw
Quoted 97 lines · On Thu, 26 Aug 2010 09:53:33 +0900 Yusuke Goda <[email protected]>…

Hi Matt, Andrew

Quoted 87 lines · Andrew Morton
Quoted 80 lines · On Wed, 25 Aug 2010 23:11:07 +0100 Matt Fleming <[email protected]> wrote:
Quoted 50 lines · On Thu, 15 Jul 2010 13:25:52 -0700 Andrew Morton <[email protected]>…
Quoted 37 lines · On Tue, 13 Jul 2010 10:16:39 +0900 Yusuke Goda <[email protected]>…

Hi Andrew

Thank you for your comment.

Quoted 10 lines

Review

Quoted 8 lines

#define ack_mmc_irqs(host, i) \

(quoted diff) +1 -4
 	do { \
-		u32 mask;\
-		mask  = sd_ctrl_read32((host), CTL_STATUS); \
-		mask &= ~((i) & TMIO_MASK_IRQ); \
-		sd_ctrl_write32((host), CTL_STATUS, mask); \
+		sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
 	} while (0)

Can we have a better changelog please?

Quoted 7 lines

What was wrong with the old code?

How does the patch fix it?

What are the user-visible runtime effects of the bug?

(It looks like that was a pretty gross bug - how did it pass testing??)

Example
- CMD53(Single block read / Received data size : 64Byte)

1) Send CMD53
2) Receive "CMD53 response"
3) Call tmio_mmc_cmd_irq(host, status);
-- original code ----------------------------------------------------

 #define ack_mmc_irqs(host, i) \
	do { \
		u32 mask;\
		mask  = sd_ctrl_read32((host), CTL_STATUS); \
	< case 1 >
		mask &= ~((i) & TMIO_MASK_IRQ); \
	< case 2 >
		sd_ctrl_write32((host), CTL_STATUS, mask); \
	} while (0)
Quoted 8 lines

---------------------------------------------------------------------

TMIO_STAT_RXRDY status will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);" if TMIO_STAT_RXRDY becomes effective between "< case 1 >" and "< case 2 >".

This causes the phenomenon that a TMIO_STAT_RXRDY interrupt does not occur. When received data are small, it rarely occurs.

OK..

Quoted 7 lines

But with both this patch and "tmio_mmc-revise-limit-on-data-size.patch" the changelogs fail to describe the impact of the bug upon our users. So when I sit here trying to work out whether the patches should be applied to 2.6.35 and whether they should be backported into -stable, I don't have enough information.

What are your thoughts on this?

Goda, do you have any more ideas on updating the changelog for this patch? It looks to me like this patch is a candidate for stable (whereas the "tmio_mmc-revise-limit-on-data-size.patch" is not, sorry about replying to that one first, I'm reading my mail backwards) because, without this patch, it's possible to miss interrupts because the ack_mmc_irqs() macro clears bit in the CTL_STATUS register that it should not do? Is that correct?

If that is the case then would this be a more appropriate changelog,

"tmio_mmc: Don't clear unhandled pending interrupts

Previously, it was possible for ack_mmc_irqs() to clear pending interrupt bits in the CTL_STATUS register, even though the interrupt handler had not been called. This was because of a race that existed when doing a read-modify-write sequence on CTL_STATUS. After the read step in this sequence, if an interrupt occurred (causing one of the bits in CTL_STATUS to be set) the write step would inadvertently clear it.

Quoted 3 lines

This patch eliminates this race by only writing to CTL_STATUS and clearing the interrupts that were passed as an argument to ack_mmc_irqs()."

Quoted 6 lines

hm, I seem to have secretly dropped this patch as well.

Oh well. I restored it as tmio_mmc-dont-clear-unhandled-pending-interrupts.patch and tagged it for a -stable backport. Unless I hear otherwise I'll send it in to Linus when we return from Brazil a couple of weeks from now.

Quoted 7 lines

Thank you for your actions.
I agree to new changelog.

In fact, I contributed the patch which changed changelog.
http://www.spinics.net/lists/linux-mmc/msg02623.html
http://www.spinics.net/lists/linux-mmc/msg02624.html
However, these will not be necessary.

Oh, I missed these. Sorry!

Andrew, this bit of Goda's changelog is worth adding to "tmio_mmc-dont-clear-unhandled-pending-interrupts.patch"

"Observed with the TMIO_STAT_RXRDY bit together with CMD53 on AR6002 and BCM4318 SDIO cards in polled mode."

#8 lore raw
Quoted 105 lines · Matt Fleming
Quoted 95 lines · On Thu, 26 Aug 2010 09:53:33 +0900 Yusuke Goda <[email protected]>…
Quoted 87 lines · Andrew Morton
Quoted 80 lines · On Wed, 25 Aug 2010 23:11:07 +0100 Matt Fleming <[email protected]> wrote:
Quoted 50 lines · On Thu, 15 Jul 2010 13:25:52 -0700 Andrew Morton <[email protected]>…
Quoted 39 lines · On Tue, 13 Jul 2010 10:16:39 +0900 Yusuke Goda <[email protected]>…

Hi Andrew

Thank you for your comment.

Quoted 10 lines
Quoted 8 lines

 #define ack_mmc_irqs(host, i) \
       do { \
-              u32 mask;\
-              mask  = sd_ctrl_read32((host), CTL_STATUS); \
-              mask &= ~((i) & TMIO_MASK_IRQ); \
-              sd_ctrl_write32((host), CTL_STATUS, mask); \
+              sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
       } while (0)

Can we have a better changelog please?

Quoted 7 lines

What was wrong with the old code?

How does the patch fix it?

What are the user-visible runtime effects of the bug?

(It looks like that was a pretty gross bug - how did it pass testing??)

Example
 - CMD53(Single block read / Received data size : 64Byte)

 1) Send CMD53
 2) Receive "CMD53 response"
 3) Call tmio_mmc_cmd_irq(host, status);
-- original code ---------------------------------------------------- #define ack_mmc_irqs(host, i) \
 do { \
         u32 mask;\
         mask  = sd_ctrl_read32((host), CTL_STATUS); \
 < case 1 >
         mask &= ~((i) & TMIO_MASK_IRQ); \
 < case 2 >
         sd_ctrl_write32((host), CTL_STATUS, mask); \
 } while (0)
---------------------------------------------------------------------

Quoted 6 lines

TMIO_STAT_RXRDY status will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);" if TMIO_STAT_RXRDY becomes effective between "< case 1 >" and "< case 2 >".

This causes the phenomenon that a TMIO_STAT_RXRDY interrupt does not occur. When received data are small, it rarely occurs.

OK..

Quoted 7 lines

But with both this patch and "tmio_mmc-revise-limit-on-data-size.patch" the changelogs fail to describe the impact of the bug upon our users. So when I sit here trying to work out whether the patches should be applied to 2.6.35 and whether they should be backported into -stable, I don't have enough information.

What are your thoughts on this?

Goda, do you have any more ideas on updating the changelog for this patch? It looks to me like this patch is a candidate for stable (whereas the "tmio_mmc-revise-limit-on-data-size.patch" is not, sorry about replying to that one first, I'm reading my mail backwards) because, without this patch, it's possible to miss interrupts because the ack_mmc_irqs() macro clears bit in the CTL_STATUS register that it should not do? Is that correct?

If that is the case then would this be a more appropriate changelog,

"tmio_mmc: Don't clear unhandled pending interrupts

Previously, it was possible for ack_mmc_irqs() to clear pending interrupt bits in the CTL_STATUS register, even though the interrupt handler had not been called. This was because of a race that existed when doing a read-modify-write sequence on CTL_STATUS. After the read step in this sequence, if an interrupt occurred (causing one of the bits in CTL_STATUS to be set) the write step would inadvertently clear it.

Quoted 3 lines

This patch eliminates this race by only writing to CTL_STATUS and clearing the interrupts that were passed as an argument to ack_mmc_irqs()."

Quoted 6 lines

hm, I seem to have secretly dropped this patch as well.

Oh well.  I restored it as tmio_mmc-dont-clear-unhandled-pending-interrupts.patch and tagged it for a -stable backport.  Unless I hear otherwise I'll send it in to Linus when we return from Brazil a couple of weeks from now.

Quoted 7 lines

Thank you for your actions.
I agree to new changelog.

In fact, I contributed the patch which changed changelog.
 http://www.spinics.net/lists/linux-mmc/msg02623.html
 http://www.spinics.net/lists/linux-mmc/msg02624.html
However, these will not be necessary.

Quoted 7 lines

Oh, I missed these. Sorry!

Andrew, this bit of Goda's changelog is worth adding to "tmio_mmc-dont-clear-unhandled-pending-interrupts.patch"

"Observed with the TMIO_STAT_RXRDY bit together with CMD53 on AR6002 and BCM4318 SDIO cards in polled mode."

Hi Matt,

Just FYI, the newer version of these patches also have a whole bunch of acked-by and tested-by tags, see this email:

http://lkml.org/lkml/2010/8/20/429

Thanks for your help!

Cheers,

/ magnus

#9 lore raw
Quoted 9 lines · Magnus Damm

Hi Matt,

Just FYI, the newer version of these patches also have a whole bunch of acked-by and tested-by tags, see this email:

http://lkml.org/lkml/2010/8/20/429

Thanks for your help!

Argh, right. Claws-mail searching has completely failed me. I didn't even see that thread when searching to "tmio_mmc". Back to mutt..

Andrew, can you drop the patch with my changelog and pick up the one in that thread seeing as it's got all the tags and a new changelog? Thanks.

#10 lore raw
Quoted 9 lines · On Thu, 26 Aug 2010 08:26:42 +0100 Matt Fleming <[email protected]> wrote:
Quoted 8 lines · Magnus Damm

Hi Matt,

Just FYI, the newer version of these patches also have a whole bunch of acked-by and tested-by tags, see this email:

http://lkml.org/lkml/2010/8/20/429

Quoted 7 lines
Quoted 1 line

Thanks for your help!

Argh, right. Claws-mail searching has completely failed me. I didn't even see that thread when searching to "tmio_mmc". Back to mutt..

Andrew, can you drop the patch with my changelog and pick up the one in that thread seeing as it's got all the tags and a new changelog? Thanks.

I actually already had it, as tmio_mmc-dont-clear-unhandled-pending-interrupts.patch, scheduled for 2.6.36 and -stable.

What's the score with "tmio_mmc: allow 2 byte requests in 4-bit mode"? I didn't merge it because Ian said "This change needs to be modified to test what hardware is present. this wont work on my hardware TTBOMK.".
Then I later _did_ merge it because it got sneakily renamed to
"tmio_mmc: revise a limit of the data size".

#11 lore raw
Quoted 24 lines · Andrew Morton
Quoted 9 lines · On Thu, 26 Aug 2010 08:26:42 +0100 Matt Fleming <[email protected]> wrote:
Quoted 8 lines · Magnus Damm

Hi Matt,

Just FYI, the newer version of these patches also have a whole bunch of acked-by and tested-by tags, see this email:

http://lkml.org/lkml/2010/8/20/429

Quoted 7 lines
Quoted 1 line

Thanks for your help!

Argh, right. Claws-mail searching has completely failed me. I didn't even see that thread when searching to "tmio_mmc". Back to mutt..

Andrew, can you drop the patch with my changelog and pick up the one in that thread seeing as it's got all the tags and a new changelog? Thanks.

I actually already had it, as tmio_mmc-dont-clear-unhandled-pending-interrupts.patch, scheduled for 2.6.36 and -stable.

Quoted 5 lines

What's the score with "tmio_mmc: allow 2 byte requests in 4-bit mode"? I didn't merge it because Ian said "This change needs to be modified to test what hardware is present. this wont work on my hardware TTBOMK.".
Then I later _did_ merge it because it got sneakily renamed to
"tmio_mmc: revise a limit of the data size".

I've stuck my oar in and confused everybody now, it seems. As Ian pointed out, before "tmio_mmc: revise a limit of the data size" can be merged something like the following is needed,

drivers/mmc/host/tmio_mmc.c +5 -1
index ee7d0a5..3eabd91 100644
@@ -657,11 +657,15 @@ static void tmio_mmc_release_dma(struct tmio_mmc_host *host)
 static int tmio_mmc_start_data(struct tmio_mmc_host *host,
 	struct mmc_data *data)
 {
+	struct mfd_cell	*cell = host->pdev->dev.platform_data;
+	struct tmio_mmc_data *pdata = cell->driver_data;
+
 	pr_debug("setup data transfer: blocksize %08x  nr_blocks %d\n",
 		 data->blksz, data->blocks);
 
 	/* Hardware cannot perform 1 and 2 byte requests in 4 bit mode */
-	if (data->blksz < 4 && host->mmc->ios.bus_width == MMC_BUS_WIDTH_4) {
+	if (data->blksz < 4 && !(pdata->flags & TMIO_MMC_2BYTE_BLKSZ) &&
+	    host->mmc->ios.bus_width == MMC_BUS_WIDTH_4) {
 		pr_err("%s: %d byte block unsupported in 4 bit mode\n",
 		       mmc_hostname(host->mmc), data->blksz);
 		return -EINVAL;
include/linux/mfd/tmio.h +5 -0
index f07425b..56467cb 100644
@@ -52,6 +52,11 @@
 
 /* tmio MMC platform flags */
 #define TMIO_MMC_WRPROTECT_DISABLE	(1 << 0)
+/*
+ * Some controllers can support a 2-byte block size when the bus width
+ * is configured in 4-bit mode.
+ */
+#define TMIO_MMC_2BYTE_BLKSZ		(1 << 1)
 
 int tmio_core_mmc_enable(void __iomem *cnf, int shift, unsigned long base);
 int tmio_core_mmc_resume(void __iomem *cnf, int shift, unsigned long base);

Magnus, since people have tested 2-byte blksz transfers on SDHI and it works, does it make sense to have this change to drivers/mfd/sh_mobile_sdhi.c? Are you aware of a version of the SDHI block that doens't support this mode?

drivers/mfd/sh_mobile_sdhi.c +6 -0
index cd16459..1f3a1b1 100644
@@ -112,6 +112,12 @@ static int __init sh_mobile_sdhi_probe(struct platform_device *pdev)
 		mmc_data->ocr_mask = p->tmio_ocr_mask;
 	}
 
+	/*
+	 * All SDHI blocks support 2-byte and larger block sizes in 4-bit
+	 * bus width mode.
+	 */
+	mmc_data->flags |= TMIO_MMC_2BYTE_BLKSZ;
+
 	if (p && p->dma_slave_tx >= 0 && p->dma_slave_rx >= 0) {
 		priv->param_tx.slave_id = p->dma_slave_tx;
 		priv->param_rx.slave_id = p->dma_slave_rx;

So I think these patches need to either be merged into "tmio_mmc: revise a limit of the data size" with a bit in the changelog explaning that some tmio blocks don't support 2-byte tranfers, or added as separate patches before it.

Does everyone agree?

#12 lore raw
Quoted 1 line · On Thu, 26 Aug 2010 23:10:24 +0100 Matt Fleming <[email protected]> wrote:

I've stuck my oar in and confused everybody now, it seems.

I've just unconfused myself my dropping the patch ;) Please send new one(s) when the dust has settled?

The only tmio_mmc patch in my tree at present is
tmio_mmc-dont-clear-unhandled-pending-interrupts:

From: Yusuke Goda <[email protected]>

Previously, it was possible for ack_mmc_irqs() to clear pending interrupt bits in the CTL_STATUS register, even though the interrupt handler had not been called. This was because of a race that existed when doing a read-modify-write sequence on CTL_STATUS. After the read step in this sequence, if an interrupt occurred (causing one of the bits in CTL_STATUS to be set) the write step would inadvertently clear it.

This patch eliminates this race by only writing to CTL_STATUS and clearing the interrupts that were passed as an argument to ack_mmc_irqs()."

[[email protected]: rewrote changelog]
Cc Magnus Damm <[email protected]>
Cc Matt Fleming <[email protected]>
Cc Ian Molton <[email protected]>
Cc Samuel Ortiz <[email protected]>
Cc Paul Mundt <[email protected]>

---

 drivers/mmc/host/tmio_mmc.h |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/mmc/host/tmio_mmc.h renamed from drivers/mmc/host/tmio_mmc.h~tmio_mmc-dont-clear-unhandled-pending-interrupts +1 -4
diff -puN drivers/mmc/host/tmio_mmc.h~tmio_mmc-dont-clear-unhandled-pending-interrupts drivers/mmc/host/tmio_mmc.h
@@ -82,10 +82,7 @@
 
 #define ack_mmc_irqs(host, i) \
 	do { \
-		u32 mask;\
-		mask  = sd_ctrl_read32((host), CTL_STATUS); \
-		mask &= ~((i) & TMIO_MASK_IRQ); \
-		sd_ctrl_write32((host), CTL_STATUS, mask); \
+		sd_ctrl_write32((host), CTL_STATUS, ~(i)); \
 	} while (0)
 
 

_

Acked-by Magnus Damm Tested-by Arnd Hannemann Acked-by Ian Molton
#13 lore raw
Quoted 7 lines · Andrew Morton
Quoted 1 line · On Thu, 26 Aug 2010 23:10:24 +0100 Matt Fleming <[email protected]> wrote:

I've stuck my oar in and confused everybody now, it seems.

I've just unconfused myself my dropping the patch ;) Please send new one(s) when the dust has settled?

Sure thing.

Quoted 15 lines

The only tmio_mmc patch in my tree at present is
tmio_mmc-dont-clear-unhandled-pending-interrupts:

From: Yusuke Goda <[email protected]>

Previously, it was possible for ack_mmc_irqs() to clear pending interrupt bits in the CTL_STATUS register, even though the interrupt handler had not been called. This was because of a race that existed when doing a read-modify-write sequence on CTL_STATUS. After the read step in this sequence, if an interrupt occurred (causing one of the bits in CTL_STATUS to be set) the write step would inadvertently clear it.

Quoted 3 lines

This patch eliminates this race by only writing to CTL_STATUS and clearing the interrupts that were passed as an argument to ack_mmc_irqs()."

Would you mind adding this snippet to the end of the changelog?

"Observed with the TMIO_STAT_RXRDY bit together with CMD53 on AR6002 and BCM4318 SDIO cards in polled mode."

And these tags,

#14 lore raw
In reply to Matt Fleming
Quoted 74 lines · Matt Fleming
Quoted 24 lines · Andrew Morton
Quoted 9 lines · On Thu, 26 Aug 2010 08:26:42 +0100 Matt Fleming <[email protected]> wrote:
Quoted 8 lines · Magnus Damm

Hi Matt,

Just FYI, the newer version of these patches also have a whole bunch of acked-by and tested-by tags, see this email:

http://lkml.org/lkml/2010/8/20/429

Quoted 7 lines
Quoted 1 line

Thanks for your help!

Argh, right. Claws-mail searching has completely failed me. I didn't even see that thread when searching to "tmio_mmc". Back to mutt..

Andrew, can you drop the patch with my changelog and pick up the one in that thread seeing as it's got all the tags and a new changelog? Thanks.

I actually already had it, as tmio_mmc-dont-clear-unhandled-pending-interrupts.patch, scheduled for 2.6.36 and -stable.

Quoted 5 lines

What's the score with "tmio_mmc: allow 2 byte requests in 4-bit mode"? I didn't merge it because Ian said "This change needs to be modified to test what hardware is present.  this wont work on my hardware TTBOMK.". Then I later _did_ merge it because it got sneakily renamed to "tmio_mmc: revise a limit of the data size".

I've stuck my oar in and confused everybody now, it seems. As Ian pointed out, before "tmio_mmc: revise a limit of the data size" can be merged something like the following is needed,

drivers/mmc/host/tmio_mmc.c +5 -1
index ee7d0a5..3eabd91 100644
@@ -657,11 +657,15 @@ static void tmio_mmc_release_dma(struct tmio_mmc_host *host)
  static int tmio_mmc_start_data(struct tmio_mmc_host *host,
        struct mmc_data *data)
  {
+       struct mfd_cell *cell = host->pdev->dev.platform_data;
+       struct tmio_mmc_data *pdata = cell->driver_data;
+
        pr_debug("setup data transfer: blocksize %08x  nr_blocks %d\n",
                 data->blksz, data->blocks);
 
        /* Hardware cannot perform 1 and 2 byte requests in 4 bit mode */
-       if (data->blksz < 4 && host->mmc->ios.bus_width == MMC_BUS_WIDTH_4) {
+       if (data->blksz < 4 && !(pdata->flags & TMIO_MMC_2BYTE_BLKSZ) &&
+           host->mmc->ios.bus_width == MMC_BUS_WIDTH_4) {
                pr_err("%s: %d byte block unsupported in 4 bit mode\n",
                       mmc_hostname(host->mmc), data->blksz);
                return -EINVAL;
include/linux/mfd/tmio.h +5 -0
index f07425b..56467cb 100644
@@ -52,6 +52,11 @@
 
  /* tmio MMC platform flags */
  #define TMIO_MMC_WRPROTECT_DISABLE     (1 << 0)
+/*
+ * Some controllers can support a 2-byte block size when the bus width
+ * is configured in 4-bit mode.
+ */
+#define TMIO_MMC_2BYTE_BLKSZ           (1 << 1)
 
  int tmio_core_mmc_enable(void __iomem *cnf, int shift, unsigned long base);
  int tmio_core_mmc_resume(void __iomem *cnf, int shift, unsigned long base);
Quoted 4 lines

Magnus, since people have tested 2-byte blksz transfers on SDHI and it works, does it make sense to have this change to drivers/mfd/sh_mobile_sdhi.c? Are you aware of a version of the SDHI block that doens't support this mode?

Yes, updating drivers/mfd/sh_mobile_sdhi.c seems like a good plan.

We've tested this on a few different SH/ARM processors with SDHI, and they all seem to be ok with 2-byte mode.

Quoted 18 lines
drivers/mfd/sh_mobile_sdhi.c +6 -0
index cd16459..1f3a1b1 100644
@@ -112,6 +112,12 @@ static int __init sh_mobile_sdhi_probe(struct platform_device *pdev)
                mmc_data->ocr_mask = p->tmio_ocr_mask;
        }
 
+       /*
+        * All SDHI blocks support 2-byte and larger block sizes in 4-bit
+        * bus width mode.
+        */
+       mmc_data->flags |= TMIO_MMC_2BYTE_BLKSZ;
+
        if (p && p->dma_slave_tx >= 0 && p->dma_slave_rx >= 0) {
                priv->param_tx.slave_id = p->dma_slave_tx;
                priv->param_rx.slave_id = p->dma_slave_rx;
Quoted 6 lines

So I think these patches need to either be merged into "tmio_mmc: revise a limit of the data size" with a bit in the changelog explaning that some tmio blocks don't support 2-byte tranfers, or added as separate patches before it.

Does everyone agree?

I certainly agree. =)

By the way, you may want to check that this patch doesn't collide with the SDHI/MMCIF hotplug patches that were recently posted to the linux-sh list.

Thanks a lot!

/ magnus