On Thursday 12 February 2009, Kumar, Purushotam wrote:
> > On Wednesday 11 February 2009, Kumar, Purushotam wrote:
> > > > @@ -914,7 +927,12 @@ static inline int handle_core_command(
> > > > status = readl(host->base + DAVINCI_MMCST0);
> > > > if (!status)
> > > > break;
> > > > +
> > > > qstatus |= status;
> > > > + if (count++ > 40) {
> > > > + dev_dbg(mmc_dev(host->mmc), "PIO timeout\n");
> > > > + break;
> > > > + }
> > > > }
> > > >
> > > > if (qstatus & MMCST0_DATDNE) {
>
> >
> > > There will be no write to FIFO and so MMCST0_DXRDY/MMCST0_DRRDY
> > > will not be set again and so no next interrupt which could cause
> > > hang. In order to fix this bug, we need to check count value before
> > > DAVINCI_MMCST0 register read. But, I feel we should not add any
> > > PIO timeout here. I checked on EVM and count has never increased
> > > more than 1 for high speed card.
> >
> > If it it only gets to one, then there's no need for the loop in
> > the first place. I'm a bit curious why that loop is there in the
> > first place ... it shouldn't actually be needed.
>
> This loop was added because we have observed earlier on non-linux
> operating system that MMC gives very fast interrupt when FIFO is
> 32 bytes which has caused stack overflow. In my test, count has
> never increased more than 1 but it may be more than 1 for some
> or other card. It will be better idea to write to FIFO when
> DXRDY/ DRRDY bit is already set instead of going out and coming
> back 2nd time immediately to IRQ.
>
> At first glance, it seems that it is unbounded while loop. But
> it has proper break if "status" is zero. All bits in register
> DAVINCI_MMCST0 are cleared by read and so read of DAVINCI_MMCST0
> will certainly return zero after few iteration irrespective of
> any condition because controller does not report same error
> again. So, I feel there is no change required and we should leave
> it as it is. What is your view on this?
It needs at least a comment, pointing out that bytes_left will
either decrease to zero or there will be no more FIFO business
to handle (since I/O is happening) ... so it's not unbounded
even when DMA is disabled.
> > Note there's another unbounded loop in the driver, which I didn't
> > fix: the main irq handler, mmc_davinci_irq(), which is calling
> > that handle_core_command() thing. The handle_core() thing ought
> > to be the main body of the IRQ handler; without looping; and
> > that "spurious IRQ branch should return IRQ_NONE (and never even
> > happen, for that matter, easily arranged).
> >
> > - Dave
>
> Ya, You are right mmc_davinci_irq(), which is calling that
> handle_core_command() in the unbounded loop. Do we really need
> this loop at first place? I tried after removing this unbounded
> loop and it seems that everything is working. What is your view
> on this? I agree with you that we should have IRQ_NONE.
Remove that loop.
- Dave
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
