On 16 January 2013 11:49, Russell King - ARM Linux
<linux@xxxxxxxxxxxxxxxx> wrote:
> 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.
Sorry, Russell I was talking junk. :-) Please ignore my last respond.
It shall be safe to keep the original behavior. The important thing is
that an error code get set, so the async request handling can clean up
a prepared request, which was already handled.
I will fixup the patch and send a v4, thanks a lot spotting this.
Kind regards
Ulf Hansson
--
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
