tmio_mmc: Prevents unexpected status clear
Subsystems (3)
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
@@ -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
Quoted 12 lines · On Wed, 07 Jul 2010 10:59:52 +0900 Yusuke Goda <[email protected]>…
This patch clears only necessary bit.
Signed-off-by Yusuke Goda <[email protected]>--- drivers/mmc/host/tmio_mmc.h | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-)drivers/mmc/host/tmio_mmc.h +0
-0
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??)
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
-4do { \ -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
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
-4do { \ -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) Calltmio_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_RXRDYstatus will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);" ifTMIO_STAT_RXRDYbecomes effective between "< case 1 >" and "< case 2 >".This causes the phenomenon that a
TMIO_STAT_RXRDYinterrupt 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?
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
-4do { \ -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) Calltmio_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_RXRDYstatus will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);" ifTMIO_STAT_RXRDYbecomes effective between "< case 1 >" and "< case 2 >".This causes the phenomenon that a
TMIO_STAT_RXRDYinterrupt 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()."
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
-4do { \ -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) Calltmio_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_RXRDYstatus will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);" ifTMIO_STAT_RXRDYbecomes effective between "< case 1 >" and "< case 2 >".This causes the phenomenon that a
TMIO_STAT_RXRDYinterrupt 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 theCTL_STATUSregister 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 interruptsPreviously, it was possible for
ack_mmc_irqs()to clear pending interrupt bits in theCTL_STATUSregister, even though the interrupt handler had not been called. This was because of a race that existed when doing a read-modify-write sequence onCTL_STATUS. After the read step in this sequence, if an interrupt occurred (causing one of the bits inCTL_STATUSto be set) the write step would inadvertently clear it.
Quoted 3 lines
This patch eliminates this race by only writing to
CTL_STATUSand clearing the interrupts that were passed as an argument toack_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.
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
-4do { \ -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) Calltmio_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_RXRDYstatus will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);" ifTMIO_STAT_RXRDYbecomes effective between "< case 1 >" and "< case 2 >".This causes the phenomenon that a
TMIO_STAT_RXRDYinterrupt 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 theCTL_STATUSregister 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 interruptsPreviously, it was possible for
ack_mmc_irqs()to clear pending interrupt bits in theCTL_STATUSregister, even though the interrupt handler had not been called. This was because of a race that existed when doing a read-modify-write sequence onCTL_STATUS. After the read step in this sequence, if an interrupt occurred (causing one of the bits inCTL_STATUSto be set) the write step would inadvertently clear it.Quoted 3 lines
This patch eliminates this race by only writing to
CTL_STATUSand clearing the interrupts that were passed as an argument toack_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
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
-4do { \ - 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) Calltmio_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_RXRDYstatus will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);" ifTMIO_STAT_RXRDYbecomes effective between "< case 1 >" and "< case 2 >".This causes the phenomenon that a
TMIO_STAT_RXRDYinterrupt 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 theCTL_STATUSregister 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 interruptsPreviously, it was possible for
ack_mmc_irqs()to clear pending interrupt bits in theCTL_STATUSregister, even though the interrupt handler had not been called. This was because of a race that existed when doing a read-modify-write sequence onCTL_STATUS. After the read step in this sequence, if an interrupt occurred (causing one of the bits inCTL_STATUSto be set) the write step would inadvertently clear it.Quoted 3 lines
This patch eliminates this race by only writing to
CTL_STATUSand clearing the interrupts that were passed as an argument toack_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."
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) Calltmio_mmc_cmd_irq(host, status);
-- original code ---------------------------------------------------- #defineack_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_RXRDYstatus will be cleared by "sd_ctrl_write32((host), CTL_STATUS, mask);" ifTMIO_STAT_RXRDYbecomes effective between "< case 1 >" and "< case 2 >".This causes the phenomenon that a
TMIO_STAT_RXRDYinterrupt 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 theCTL_STATUSregister 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 interruptsPreviously, it was possible for
ack_mmc_irqs()to clear pending interrupt bits in theCTL_STATUSregister, even though the interrupt handler had not been called. This was because of a race that existed when doing a read-modify-write sequence onCTL_STATUS. After the read step in this sequence, if an interrupt occurred (causing one of the bits inCTL_STATUSto be set) the write step would inadvertently clear it.Quoted 3 lines
This patch eliminates this race by only writing to
CTL_STATUSand clearing the interrupts that were passed as an argument toack_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_RXRDYbit 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
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.
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:
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".
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:
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
@@ -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
@@ -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
@@ -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?
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()."
---
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
@@ -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)
_
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_mmcpatch 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 theCTL_STATUSregister, even though the interrupt handler had not been called. This was because of a race that existed when doing a read-modify-write sequence onCTL_STATUS. After the read step in this sequence, if an interrupt occurred (causing one of the bits inCTL_STATUSto be set) the write step would inadvertently clear it.
Quoted 3 lines
This patch eliminates this race by only writing to
CTL_STATUSand clearing the interrupts that were passed as an argument toack_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,
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:
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@@ -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@@ -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@@ -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