Hi Matt, Andrew Andrew Morton wrote: > On Wed, 25 Aug 2010 23:11:07 +0100 > Matt Fleming <[email protected]> wrote: > >> On Thu, 15 Jul 2010 13:25:52 -0700 >> Andrew Morton <[email protected]> wrote: >> >>> On Tue, 13 Jul 2010 10:16:39 +0900 >>> Yusuke Goda <[email protected]> wrote: >>> >>>> Hi Andrew >>>> >>>> Thank you for your comment. >>>> >>>>>> #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??) >>>> 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. >>>> >>> 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? >> 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()." > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
