On Wed, Jan 16, 2013 at 11:23:57AM +0100, Ulf Hansson wrote:
> On 13 January 2013 20:24, Russell King - ARM Linux
> <linux@xxxxxxxxxxxxxxxx> wrote:
> > On Mon, Jan 07, 2013 at 03:58:27PM +0100, Ulf Hansson wrote:
> >> @@ -374,19 +415,12 @@ static void mmci_dma_unmap(struct mmci_host *host, struct mmc_data *data)
> >> * contiguous buffers. On TX, we'll get a FIFO underrun error.
> >> */
> >> if (status & MCI_RXDATAAVLBLMASK) {
> >> - dmaengine_terminate_all(chan);
> >> - if (!data->error)
> >> - data->error = -EIO;
> >> - }
> >> -
> >> - if (data->flags & MMC_DATA_WRITE) {
> >> - dir = DMA_TO_DEVICE;
> >> - } else {
> >> - dir = DMA_FROM_DEVICE;
> >> + data->error = -EIO;
> >> + mmci_dma_data_error(host);
> >
> > Please explain the change of behaviour here. Before your change, we _only_
> > set data->error if the error is not set. Here, we overwrite the error code
> > no matter what. What is the reasoning for that change?
> >
> > The reason the code is like it _was_ is so that any bytes remaining in the
> > FIFO are _only_ reported as an error if there wasn't a preceding error.
> > That is the behaviour I desired when I wrote this code.
>
> Since we need to do dmaengine_terminate_all(chan), that will mean
> another request could potentially already be prepared and thus it
> could also be terminated.
>
> Then by always reporting an error the async request handling in the
> mmc protocol layer, can do proper error handling and clean up the
> previously prepared request.
Sigh, I don't think you understand what this code is doing at all then.
We will _always_ report _an_ error as the code originally stands if we
encounter MCI_RXDATAAVLBLMASK being set. The error that we report will
be the _correct_ one - either the one which preceeds this condition
occuring _or_ an IO error because not all data was read from the FIFO
by the DMA engine.
If the DMA engine fails to read all the data from the FIFO, _and_ there
was a preceeding error, we should _not_ overwrite the preceeding error.
This is exactly what the original code does.
Your version _always_ overwrites the previous error. This could result
in FIFO/CRC errors always being reported as an IO error rather than
their proper error codes, and therefore _breaking_ error handling.
--
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
