Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 11:08:48AM -0400, Peter Hurley wrote:
> [ + Greg KH ]
> 
> On 08/07/2015 09:57 AM, Russell King - ARM Linux wrote:
> > 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.
> > 
> >>> 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?

> > 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.

> > 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.

Sorry, I have little sympathy here.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/