On 08/07/2015 02:32 PM, Russell King - ARM Linux wrote: > On Fri, Aug 07, 2015 at 02:21:59PM -0400, Peter Hurley wrote: >> [ + Heikki ] >> >> On 08/07/2015 12:33 PM, Russell King - ARM Linux wrote: >>> What you have is a race condition in the code you a responsible for >>> maintaining, caused by poorly implemented code. Fix it, rather than >>> whinging about drivers outside of your subsystem having never implemented >>> _optional_ things that you choose to merge broken code which relied upon >>> it _without_ checking that the operation succeeded. >>> >>> It is _entirely_ your code which is wrong here. >>> >>> I will wait for that to be fixed before acking the omap-dma change since >>> you obviously need something to test with. >> >> I'm not sure to what you're referring here. >> >> A WARNing fixes nothing. > > The warning can wait. > >> If you mean some patch, as yet unwritten, that handles the dma cases when >> dmaengine_pause() is unimplemented without data loss, ok, but please confirm >> that's what you mean. > > But the regression needs fixing. I too would prefer the bug to be fixed. But calling it a regression is incorrect. There is no previous SHA in which this problem didn't exist, except before either 8250_dma or 8250_omap was added. >From the outset, both the 8250 dma code and the 8250_omap driver (mistakenly) relied on dmaengine_pause. >> However, at some point one must look at the api and wonder if the separation >> of concern has been drawn in the right place. > > It _is_ in the right place. dmaengine_pause() always has been permitted > to fail. It's the responsibility of the user of this API to _check_ the > return code to find out whether it had the desired effect. Not checking > the return code is a bug in the caller's code. > > If that wasn't the case, dmaengine_pause() would have a void return type. > It doesn't. It has an 'int' to allow failure A resource error is significantly different than ENOSYS or EINVAL. > or to allow non- > implementation for cases where the underlying hardware can't pause the > channel without causing data loss. That's your assertion; I've seen no documentation to back that up (other than the de facto commit). And quite frankly, that's absurd. 1. No other driver implements _only some_ use-cases of dmaengine_pause(). 2. The number of users expecting dmaengine_pause to be implemented for non-cyclic dma transfers _dwarfs_ cyclic users. 3. There's a dedicated query interface, dma_get_slave_caps(), for which omap-dma returns /true/ -- not /maybe/ -- to indicate dmaengine_pause() is implemented. As a consumer of the api, I'd much rather opt-out at device initialization time knowing that a required feature is unimplemented, than discover it at i/o time when it's too late. > What would you think is better: an API which silently loses data, or > one which refuses to stop the transfer and reports an error code back > to the caller. An api which provides a means of determining if necessary functionality is implemented _during setup_. That way the consumer of the api can determine if the feature is supportable. For example, dma_get_slave_caps() could differentiate * pause for cyclic support * pause for non-cyclic support * pause and resume support * pause and terminate support .... > You seem to be arguing for the former, and as such, there's no way I > can take you seriously. Leaping to conclusions. > In any case, Greg has now commented on the patch adding the feature, > basically refusing it for stable tree inclusion. So the matter is > settled: omap-dma isn't going to get the pause feature added in stable > trees any time soon. So a different solution now needs to be found, > which is what I've been saying all along... While Sebastian's initial patch is a good first-cut at addressing 8250_omap's use of omap-dma, none of the patches address the general design problem I have outlined above; namely, that simply returning an error at use time for an unimplemented slave transaction is fundamentally flawed. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
