On Thu, 26 Aug 2010 09:53:33 +0900
Yusuke Goda <yusuke.goda.sx@xxxxxxxxxxx> wrote:
> Hi Matt, Andrew
>
> Andrew Morton wrote:
> > On Wed, 25 Aug 2010 23:11:07 +0100
> > Matt Fleming <matt@xxxxxxxxxxxxxxxxx> wrote:
> >
> >> On Thu, 15 Jul 2010 13:25:52 -0700
> >> Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> >>
> >>> On Tue, 13 Jul 2010 10:16:39 +0900
> >>> Yusuke Goda <yusuke.goda.sx@xxxxxxxxxxx> 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.
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."
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
