On 08/07/2015 11:29 AM, Russell King - ARM Linux wrote:

On Fri, Aug 07, 2015 at 11:08:48AM -0400, Peter Hurley wrote:

quoted

[ + Greg KH ]

On 08/07/2015 09:57 AM, Russell King - ARM Linux wrote:

quoted

As it is something that the driver has _not_ supported, you are clearly
adding a feature to an existing driver.  It's not a bug fix.

quoted

quoted

If something else has been converted to pause channels and that is causing
a problem, then _that_ conversion is where the bug lies, not the lack of
support in the omap-dma.

FWIW, the actual bug is the api that silently does nothing.

Incorrect.

static int omap_dma_pause(struct dma_chan *chan)
{
        struct omap_chan *c = to_omap_dma_chan(chan);

        /* Pause/Resume only allowed with cyclic mode */
        if (!c->cyclic)
                return -EINVAL;

Asking for the channel to be paused will return an error code indicating
that the request failed.  That will be propagated back through to the
return code of dmaengine_pause().

If we look at what 8250-dma.c is doing:

                if (dma->rx_running) {
                        dmaengine_pause(dma->rxchan);

It's 8250-dma.c which is silently _ignoring_ the return code, failing
to check that the operation it requested worked.  Maybe this should be
WARN_ON(dmaengine_pause(dma->rxchan)) or at least it should print a
message?

Thanks for the suggestion; I'll hold on to that and push it after we add
the 8250 omap dma pause in mainline.

quoted

quoted

Right, so the patch which caused the regression is the one which arranged
for the 8250-dma + omap-dma combination to work together, not the missing
pause support in omap-dma.

That would be the original submission patch set for an entire driver,
the 8250_omap driver.

Well, that's where the bug lies, and I don't agree with your assessment
that it would mean reverting the whole thing.

The binding between the two drivers is controlled via DT.  DT tells it
which DMA controller and which DMA input to use.  So, as DMA is currently
broken on this, the solution is to break that link so that 8250-omap goes
back to using PIO only.

quoted

quoted

As the binding of drivers is controlled by DT, you can disable the binding
of these two drivers

No. 8250 dma is not a stand-alone driver. Even if it were, how would you go
back and fix DTs in the wild?

Well, we have reached an impass then.

I'm not going to accept a feature addition to a driver as a stable patch
without it being adequately tested over _several_ kernel revisions to
ensure that we don't end up cocking up some driver which uses the DMA
interfaces correctly.  It's too big a risk.

So, I guess that means that older kernels will just have to remain broken -
all because the basic testing of the original code was never undertaken
to ensure that basic stuff like reception of characters worked properly.

Well, instead of me saying something snide about the lack of upstream serial
driver unit tests, I'll say I've been working on cleaning up and organizing
my own tty/serial subsystem and driver units tests which I hope to upstream
in the fall.

Those include i/o validators that ran this driver for days at time without
error at max line rate. Unfortunately, that hardware does not exhibit the
same problem as the DRA7 noted in the changelog.

Sorry, I have little sympathy here.