[PATCH v3 01/15] dmaengine: dw: fix master selection

[PATCH v3 01/15] dmaengine: dw: fix master selection

[Andy Shevchenko]

The commit 895005202987 ("dmaengine: dw: apply both HS interfaces and remove
slave_id usage") cleaned up the code to avoid usage of depricated slave_id
member of generic slave configuration.

Meanwhile it broke the master selection by removing important call to
dwc_set_masters() in ->device_alloc_chan_resources() which copied masters from
custom slave configuration to the internal channel structure.

Everything works until now since there is no customized connection of
DesignWare DMA IP to the bus, i.e. one bus and one or more masters are in use.
The configurations where 2 masters are connected to the different masters are
not working anymore. We are expecting one user of such configuration and need
to select masters properly. Besides that it is obviously a performance
regression since only one master is in use in multi-master configuration.

Select masters in accordance to what user asked for. Keep this patch in a form
more suitable for back porting.

We are safe to take necessary data in ->device_alloc_chan_resources() because
we don't support generic slave configuration embedded into custom one, and thus
the only way to provide it is to use parameter to a filter function which is
called exactly before channel resource allocation.

Fixes: 895005202987 ("dmaengine: dw: apply both HS interfaces and remove slave_id usage")
Cc: stable@vger.kernel.org
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw/core.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 5ad0ec1..ba46628 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -130,26 +130,14 @@ static void dwc_desc_put(struct dw_dma_chan *dwc, struct dw_desc *desc)
 static void dwc_initialize(struct dw_dma_chan *dwc)
 {
 	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
-	struct dw_dma_slave *dws = dwc->chan.private;
 	u32 cfghi = DWC_CFGH_FIFO_MODE;
 	u32 cfglo = DWC_CFGL_CH_PRIOR(dwc->priority);
 
 	if (dwc->initialized == true)
 		return;
 
-	if (dws) {
-		/*
-		 * We need controller-specific data to set up slave
-		 * transfers.
-		 */
-		BUG_ON(!dws->dma_dev || dws->dma_dev != dw->dma.dev);
-
-		cfghi |= DWC_CFGH_DST_PER(dws->dst_id);
-		cfghi |= DWC_CFGH_SRC_PER(dws->src_id);
-	} else {
-		cfghi |= DWC_CFGH_DST_PER(dwc->dst_id);
-		cfghi |= DWC_CFGH_SRC_PER(dwc->src_id);
-	}
+	cfghi |= DWC_CFGH_DST_PER(dwc->dst_id);
+	cfghi |= DWC_CFGH_SRC_PER(dwc->src_id);
 
 	channel_writel(dwc, CFG_LO, cfglo);
 	channel_writel(dwc, CFG_HI, cfghi);
@@ -941,7 +929,7 @@ bool dw_dma_filter(struct dma_chan *chan, void *param)
 	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
 	struct dw_dma_slave *dws = param;
 
-	if (!dws || dws->dma_dev != chan->device->dev)
+	if (dws->dma_dev != chan->device->dev)
 		return false;
 
 	/* We have to copy data since dws can be temporary storage */
@@ -1165,6 +1153,11 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 	 * doesn't mean what you think it means), and status writeback.
 	 */
 
+	/*
+	 * We need controller-specific data to set up slave transfers.
+	 */
+	BUG_ON(chan->private && !dw_dma_filter(chan, chan->private));
+
 	/* Enable controller here if needed */
 	if (!dw->in_use)
 		dw_dma_on(dw);
@@ -1226,6 +1219,14 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 	spin_lock_irqsave(&dwc->lock, flags);
 	list_splice_init(&dwc->free_list, &list);
 	dwc->descs_allocated = 0;
+
+	/* Clear custom channel configuration */
+	dwc->src_id = 0;
+	dwc->dst_id = 0;
+
+	dwc->src_master = 0;
+	dwc->dst_master = 0;
+
 	dwc->initialized = false;
 
 	/* Disable interrupts */
-- 
2.7.0

Re: [PATCH v3 01/15] dmaengine: dw: fix master selection

[Vinod Koul]

On Fri, Mar 18, 2016 at 04:24:40PM +0200, Andy Shevchenko wrote:
> +	/*
> +	 * We need controller-specific data to set up slave transfers.
> +	 */
> +	BUG_ON(chan->private && !dw_dma_filter(chan, chan->private));

I don't think BUG_ON is apt here, gracefully failing and printing that can
be better..

-- 
~Vinod

Re: [PATCH v3 01/15] dmaengine: dw: fix master selection

[Andy Shevchenko]

On Mon, 2016-04-04 at 10:03 -0700, Vinod Koul wrote:
> On Fri, Mar 18, 2016 at 04:24:40PM +0200, Andy Shevchenko wrote:
> > 
> > +	/*
> > +	 * We need controller-specific data to set up slave
> > transfers.
> > +	 */
> > +	BUG_ON(chan->private && !dw_dma_filter(chan, chan-
> > >private));
> I don't think BUG_ON is apt here, gracefully failing and printing
> that can
> be better..

Hm, this is coming from the existing code.

I would prefer to keep this one as is since it's targeted to stable@,
and I may issue sequential patch to replace BUG_ON. Sounds okay?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 01/15] dmaengine: dw: fix master selection

[Vinod Koul]

On Mon, Apr 04, 2016 at 08:10:54PM +0300, Andy Shevchenko wrote:
> On Mon, 2016-04-04 at 10:03 -0700, Vinod Koul wrote:
> > On Fri, Mar 18, 2016 at 04:24:40PM +0200, Andy Shevchenko wrote:
> > > 
> > > +	/*
> > > +	�* We need controller-specific data to set up slave
> > > transfers.
> > > +	�*/
> > > +	BUG_ON(chan->private && !dw_dma_filter(chan, chan-
> > > >private));
> > I don't think BUG_ON is apt here, gracefully failing and printing
> > that can
> > be better..
> 
> Hm, this is coming from the existing code.
> 
> I would prefer to keep this one as is since it's targeted to stable@,
> and I may issue sequential patch to replace BUG_ON. Sounds okay?

But since this is going stable, how about we remove here in the patch and
benefit stable tree as well?

Thanks
-- 
~Vinod

Re: [PATCH v3 01/15] dmaengine: dw: fix master selection

[Andy Shevchenko]

On Wed, Apr 6, 2016 at 9:56 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Mon, Apr 04, 2016 at 08:10:54PM +0300, Andy Shevchenko wrote:
>> On Mon, 2016-04-04 at 10:03 -0700, Vinod Koul wrote:
>> > On Fri, Mar 18, 2016 at 04:24:40PM +0200, Andy Shevchenko wrote:
>> > >
>> > > + /*
>> > > +  * We need controller-specific data to set up slave
>> > > transfers.
>> > > +  */
>> > > + BUG_ON(chan->private && !dw_dma_filter(chan, chan-
>> > > >private));
>> > I don't think BUG_ON is apt here, gracefully failing and printing
>> > that can
>> > be better..
>>
>> Hm, this is coming from the existing code.
>>
>> I would prefer to keep this one as is since it's targeted to stable@,
>> and I may issue sequential patch to replace BUG_ON. Sounds okay?
>
> But since this is going stable, how about we remove here in the patch and
> benefit stable tree as well?

I'm fine with that. I will send you patch tomorrow.Other than that do
we have any issues with the rest?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 01/15] dmaengine: dw: fix master selection

[Koul, Vinod]

On Wed, 2016-04-06 at 22:56 +0300, Andy Shevchenko wrote:
> On Wed, Apr 6, 2016 at 9:56 PM, Vinod Koul <vinod.koul@intel.com>
> wrote:
> > On Mon, Apr 04, 2016 at 08:10:54PM +0300, Andy Shevchenko wrote:
> > > On Mon, 2016-04-04 at 10:03 -0700, Vinod Koul wrote:
> > > > On Fri, Mar 18, 2016 at 04:24:40PM +0200, Andy Shevchenko wrote:
> > > > > 
> > > > > + /*
> > > > > +  * We need controller-specific data to set up slave
> > > > > transfers.
> > > > > +  */
> > > > > + BUG_ON(chan->private && !dw_dma_filter(chan, chan-
> > > > > > private));
> > > > I don't think BUG_ON is apt here, gracefully failing and
> > > > printing
> > > > that can
> > > > be better..
> > > 
> > > Hm, this is coming from the existing code.
> > > 
> > > I would prefer to keep this one as is since it's targeted to
> > > stable@,
> > > and I may issue sequential patch to replace BUG_ON. Sounds okay?
> > 
> > But since this is going stable, how about we remove here in the
> > patch and
> > benefit stable tree as well?
> 
> I'm fine with that. I will send you patch tomorrow.Other than that do
> we have any issues with the rest?

I did look at few more they looked fine, but didn't look at entire
series


-- 
~Vinod

Re: [PATCH v3 01/15] dmaengine: dw: fix master selection

[Andy Shevchenko]

On Thu, Apr 7, 2016 at 12:09 AM, Koul, Vinod <vinod.koul@intel.com> wrote:
> On Wed, 2016-04-06 at 22:56 +0300, Andy Shevchenko wrote:
>> On Wed, Apr 6, 2016 at 9:56 PM, Vinod Koul <vinod.koul@intel.com>
>> wrote:
>> > On Mon, Apr 04, 2016 at 08:10:54PM +0300, Andy Shevchenko wrote:
>> > > On Mon, 2016-04-04 at 10:03 -0700, Vinod Koul wrote:
>> > > > On Fri, Mar 18, 2016 at 04:24:40PM +0200, Andy Shevchenko wrote:

>> > > > > + /*
>> > > > > +  * We need controller-specific data to set up slave
>> > > > > transfers.
>> > > > > +  */
>> > > > > + BUG_ON(chan->private && !dw_dma_filter(chan, chan-
>> > > > > > private));

>> > > > I don't think BUG_ON is apt here, gracefully failing and
>> > > > printing
>> > > > that can
>> > > > be better..

By the way, what level do you want to have warning on?
I consider now between dev_warn() and dev_WARN()...

>> do
>> we have any issues with the rest?
>
> I did look at few more they looked fine, but didn't look at entire
> series

Thanks, got it!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3.5 1/1] dmaengine: dw: fix master selection

[Andy Shevchenko]

The commit 895005202987 ("dmaengine: dw: apply both HS interfaces and remove
slave_id usage") cleaned up the code to avoid usage of depricated slave_id
member of generic slave configuration.

Meanwhile it broke the master selection by removing important call to
dwc_set_masters() in ->device_alloc_chan_resources() which copied masters from
custom slave configuration to the internal channel structure.

Everything works until now since there is no customized connection of
DesignWare DMA IP to the bus, i.e. one bus and one or more masters are in use.
The configurations where 2 masters are connected to the different masters are
not working anymore. We are expecting one user of such configuration and need
to select masters properly. Besides that it is obviously a performance
regression since only one master is in use in multi-master configuration.

Select masters in accordance with what user asked for. Keep this patch in a form
more suitable for back porting.

We are safe to take necessary data in ->device_alloc_chan_resources() because
we don't support generic slave configuration embedded into custom one, and thus
the only way to provide such is to use the parameter to a filter function which
is called exactly before channel resource allocation.

While here, replase BUG_ON to less noisy dev_warn() and prevent channel
allocation in case of error.

Fixes: 895005202987 ("dmaengine: dw: apply both HS interfaces and remove slave_id usage")
Cc: stable@vger.kernel.org
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw/core.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 5ad0ec1..97199b3 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -130,26 +130,14 @@ static void dwc_desc_put(struct dw_dma_chan *dwc, struct dw_desc *desc)
 static void dwc_initialize(struct dw_dma_chan *dwc)
 {
 	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
-	struct dw_dma_slave *dws = dwc->chan.private;
 	u32 cfghi = DWC_CFGH_FIFO_MODE;
 	u32 cfglo = DWC_CFGL_CH_PRIOR(dwc->priority);
 
 	if (dwc->initialized == true)
 		return;
 
-	if (dws) {
-		/*
-		 * We need controller-specific data to set up slave
-		 * transfers.
-		 */
-		BUG_ON(!dws->dma_dev || dws->dma_dev != dw->dma.dev);
-
-		cfghi |= DWC_CFGH_DST_PER(dws->dst_id);
-		cfghi |= DWC_CFGH_SRC_PER(dws->src_id);
-	} else {
-		cfghi |= DWC_CFGH_DST_PER(dwc->dst_id);
-		cfghi |= DWC_CFGH_SRC_PER(dwc->src_id);
-	}
+	cfghi |= DWC_CFGH_DST_PER(dwc->dst_id);
+	cfghi |= DWC_CFGH_SRC_PER(dwc->src_id);
 
 	channel_writel(dwc, CFG_LO, cfglo);
 	channel_writel(dwc, CFG_HI, cfghi);
@@ -941,7 +929,7 @@ bool dw_dma_filter(struct dma_chan *chan, void *param)
 	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
 	struct dw_dma_slave *dws = param;
 
-	if (!dws || dws->dma_dev != chan->device->dev)
+	if (dws->dma_dev != chan->device->dev)
 		return false;
 
 	/* We have to copy data since dws can be temporary storage */
@@ -1165,6 +1153,14 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 	 * doesn't mean what you think it means), and status writeback.
 	 */
 
+	/*
+	 * We need controller-specific data to set up slave transfers.
+	 */
+	if (chan->private && !dw_dma_filter(chan, chan->private)) {
+		dev_warn(chan2dev(chan), "Wrong controller-specific data\n");
+		return -EINVAL;
+	}
+
 	/* Enable controller here if needed */
 	if (!dw->in_use)
 		dw_dma_on(dw);
@@ -1226,6 +1222,14 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 	spin_lock_irqsave(&dwc->lock, flags);
 	list_splice_init(&dwc->free_list, &list);
 	dwc->descs_allocated = 0;
+
+	/* Clear custom channel configuration */
+	dwc->src_id = 0;
+	dwc->dst_id = 0;
+
+	dwc->src_master = 0;
+	dwc->dst_master = 0;
+
 	dwc->initialized = false;
 
 	/* Disable interrupts */
-- 
2.8.0.rc3

Re: [PATCH v3 01/15] dmaengine: dw: fix master selection

[Andy Shevchenko]

On Wed, 2016-04-06 at 21:09 +0000, Koul, Vinod wrote:
> On Wed, 2016-04-06 at 22:56 +0300, Andy Shevchenko wrote:
> > 
> > On Wed, Apr 6, 2016 at 9:56 PM, Vinod Koul <vinod.koul@intel.com>
> > wrote:
> > > 
> > > On Mon, Apr 04, 2016 at 08:10:54PM +0300, Andy Shevchenko wrote:
> > > > 
> > > > On Mon, 2016-04-04 at 10:03 -0700, Vinod Koul wrote:
> > > > > 
> > > > > On Fri, Mar 18, 2016 at 04:24:40PM +0200, Andy Shevchenko
> > > > > wrote:
> > > > > > 
> > > > > > 
> > > > > > + /*
> > > > > > +  * We need controller-specific data to set up slave
> > > > > > transfers.
> > > > > > +  */
> > > > > > + BUG_ON(chan->private && !dw_dma_filter(chan, chan-
> > > > > > > 
> > > > > > > private));
> > > > > I don't think BUG_ON is apt here, gracefully failing and
> > > > > printing
> > > > > that can
> > > > > be better..
> > > > Hm, this is coming from the existing code.
> > > > 
> > > > I would prefer to keep this one as is since it's targeted to
> > > > stable@,
> > > > and I may issue sequential patch to replace BUG_ON. Sounds okay?
> > > But since this is going stable, how about we remove here in the
> > > patch and
> > > benefit stable tree as well?
> > I'm fine with that. I will send you patch tomorrow.Other than that
> > do
> > we have any issues with the rest?
> I did look at few more they looked fine, but didn't look at entire
> series

I've sent an update to patch 1 with "PATCH v3.5" in the subject.

Anything else should I do?

At least Intel Quark UART series (published) and SATA DWC 460ex (not
published in mailing list yet) are relying on this. It would be nice to
have immutable branch as soon as everything okay with the series.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy