Re: [PATCH] tmio_mmc: Prevents unexpected status clear

[Matt Fleming <matt@console-pimps.org>]

On Thu, 15 Jul 2010 13:25:52 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 13 Jul 2010 10:16:39 +0900
> Yusuke Goda <yusuke.goda.sx@renesas.com> 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()."