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()."
--
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
