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) {
>
> I think it has potential bug. DAVINCI_MMCST0 register has been
> read (assume that either MMCST0_DXRDY or MMCST0_DRRDY was also
> set) and at same time it will break if count has reached 41.
So you suggest ... what? Tending the fifo exactly once, and
removing that loop, would be my suggestion.
> 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.
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
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
