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.
--
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
