On Thu, Aug 26, 2010 at 3:53 PM, Matt Fleming <matt@xxxxxxxxxxxxxxxxx> wrote:
> On Thu, 26 Aug 2010 09:53:33 +0900
> Yusuke Goda <yusuke.goda.sx@xxxxxxxxxxx> wrote:
>> 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."
Hi Matt,
Just FYI, the newer version of these patches also have a whole bunch
of acked-by and tested-by tags, see this email:
http://lkml.org/lkml/2010/8/20/429
Thanks for your help!
Cheers,
/ magnus
--
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
