Re: [PATCH] tmio_mmc: Prevents unexpected status clear

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 25 Aug 2010 23:11:07 +0100
Matt Fleming <matt@console-pimps.org> wrote:

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

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.