[PATCH 0/2] dma: mmp_pdma: Fix phy channels not protected issue

[PATCH 0/2] dma: mmp_pdma: Fix phy channels not protected issue

[Xiang Wang]

From: Xiang Wang <wangx@marvell.com>

This patch set deals with the issues that 1) phy channels are not protected
in mmp_pdma. 2) dma request<->channel mapping is not cleared when a phy chan
is freed.

Xiang Wang (2):
  dma: mmp_pdma: add protect when alloc/free phy channels
  dma: mmp_pdma: clear DRCMR when free a phy channel

 drivers/dma/mmp_pdma.c |   47 +++++++++++++++++++++++++++++++----------------
 1 files changed, 31 insertions(+), 16 deletions(-)

-- 
1.7.5.4

[PATCH 1/2] dma: mmp_pdma: add protect when alloc/free phy channels

[Xiang Wang]

From: Xiang Wang <wangx@marvell.com>

In mmp pdma, phy channels are allocated/freed dynamically
and frequently. But no proper protection is added.
Conflict will happen when multi-users are requesting phy
channels at the same time. Use spinlock to protect.

Signed-off-by: Xiang Wang <wangx@marvell.com>
---
 drivers/dma/mmp_pdma.c |   41 +++++++++++++++++++++++++----------------
 1 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
index c26699f..21820c9 100644
--- a/drivers/dma/mmp_pdma.c
+++ b/drivers/dma/mmp_pdma.c
@@ -121,6 +121,7 @@ struct mmp_pdma_device {
 	struct device			*dev;
 	struct dma_device		device;
 	struct mmp_pdma_phy		*phy;
+	spinlock_t phy_lock; /* protect alloc/free phy channels */
 };
 
 #define tx_to_mmp_pdma_desc(tx) container_of(tx, struct mmp_pdma_desc_sw, async_tx)
@@ -219,6 +220,7 @@ static struct mmp_pdma_phy *lookup_phy(struct mmp_pdma_chan *pchan)
 	int prio, i;
 	struct mmp_pdma_device *pdev = to_mmp_pdma_dev(pchan->chan.device);
 	struct mmp_pdma_phy *phy;
+	unsigned long flags;
 
 	/*
 	 * dma channel priorities
@@ -227,6 +229,8 @@ static struct mmp_pdma_phy *lookup_phy(struct mmp_pdma_chan *pchan)
 	 * ch 8 - 11, 24 - 27  <--> (2)
 	 * ch 12 - 15, 28 - 31  <--> (3)
 	 */
+
+	spin_lock_irqsave(&pdev->phy_lock, flags);
 	for (prio = 0; prio <= (((pdev->dma_channels - 1) & 0xf) >> 2); prio++) {
 		for (i = 0; i < pdev->dma_channels; i++) {
 			if (prio != ((i & 0xf) >> 2))
@@ -234,14 +238,29 @@ static struct mmp_pdma_phy *lookup_phy(struct mmp_pdma_chan *pchan)
 			phy = &pdev->phy[i];
 			if (!phy->vchan) {
 				phy->vchan = pchan;
+				spin_unlock_irqrestore(&pdev->phy_lock, flags);
 				return phy;
 			}
 		}
 	}
 
+	spin_unlock_irqrestore(&pdev->phy_lock, flags);
 	return NULL;
 }
 
+static void free_phy(struct mmp_pdma_chan *pchan)
+{
+	struct mmp_pdma_device *pdev = to_mmp_pdma_dev(pchan->chan.device);
+	unsigned long flags;
+	if (!pchan->phy)
+		return;
+
+	spin_lock_irqsave(&pdev->phy_lock, flags);
+	pchan->phy->vchan = NULL;
+	pchan->phy = NULL;
+	spin_unlock_irqrestore(&pdev->phy_lock, flags);
+}
+
 /* desc->tx_list ==> pending list */
 static void append_pending_queue(struct mmp_pdma_chan *chan,
 					struct mmp_pdma_desc_sw *desc)
@@ -277,10 +296,7 @@ static void start_pending_queue(struct mmp_pdma_chan *chan)
 
 	if (list_empty(&chan->chain_pending)) {
 		/* chance to re-fetch phy channel with higher prio */
-		if (chan->phy) {
-			chan->phy->vchan = NULL;
-			chan->phy = NULL;
-		}
+		free_phy(chan);
 		dev_dbg(chan->dev, "no pending list\n");
 		return;
 	}
@@ -377,10 +393,7 @@ static int mmp_pdma_alloc_chan_resources(struct dma_chan *dchan)
 		dev_err(chan->dev, "unable to allocate descriptor pool\n");
 		return -ENOMEM;
 	}
-	if (chan->phy) {
-		chan->phy->vchan = NULL;
-		chan->phy = NULL;
-	}
+	free_phy(chan);
 	chan->idle = true;
 	chan->dev_addr = 0;
 	return 1;
@@ -411,10 +424,7 @@ static void mmp_pdma_free_chan_resources(struct dma_chan *dchan)
 	chan->desc_pool = NULL;
 	chan->idle = true;
 	chan->dev_addr = 0;
-	if (chan->phy) {
-		chan->phy->vchan = NULL;
-		chan->phy = NULL;
-	}
+	free_phy(chan);
 	return;
 }
 
@@ -581,10 +591,7 @@ static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd,
 	switch (cmd) {
 	case DMA_TERMINATE_ALL:
 		disable_chan(chan->phy);
-		if (chan->phy) {
-			chan->phy->vchan = NULL;
-			chan->phy = NULL;
-		}
+		free_phy(chan);
 		spin_lock_irqsave(&chan->desc_lock, flags);
 		mmp_pdma_free_desc_list(chan, &chan->chain_pending);
 		mmp_pdma_free_desc_list(chan, &chan->chain_running);
@@ -777,6 +784,8 @@ static int mmp_pdma_probe(struct platform_device *op)
 		return -ENOMEM;
 	pdev->dev = &op->dev;
 
+	spin_lock_init(&pdev->phy_lock);
+
 	iores = platform_get_resource(op, IORESOURCE_MEM, 0);
 	if (!iores)
 		return -EINVAL;
-- 
1.7.5.4

[PATCH 2/2] dma: mmp_pdma: clear DRCMR when free a phy channel

[Xiang Wang]

From: Xiang Wang <wangx@marvell.com>

In mmp pdma, phy channels are allocated/freed dynamically.
The mapping from DMA request to DMA channel number in DRCMR
should be cleared when a phy channel is freed. Otherwise
conflicts will happen when:
1. A is using channel 2 and free it after finished, but A
still maps to channel 2 in DRCMR of A.
2. Now another one B gets channel 2. So B maps to channel 2
too in DRCMR of B.
In the datasheet, it is described that "Do not map two active
requests to the same channel since it produces unpredictable
results" and we can observe that during test.

Signed-off-by: Xiang Wang <wangx@marvell.com>
---
 drivers/dma/mmp_pdma.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
index 21820c9..4cd7276 100644
--- a/drivers/dma/mmp_pdma.c
+++ b/drivers/dma/mmp_pdma.c
@@ -252,9 +252,15 @@ static void free_phy(struct mmp_pdma_chan *pchan)
 {
 	struct mmp_pdma_device *pdev = to_mmp_pdma_dev(pchan->chan.device);
 	unsigned long flags;
+	u32 reg;
 	if (!pchan->phy)
 		return;
 
+	/* clear the channel mapping in DRCMR */
+	reg = pchan->phy->vchan->drcmr;
+	reg = (((reg) < 64) ? 0x0100 : 0x1100) + (((reg) & 0x3f) << 2);
+	writel(0, pchan->phy->base + reg);
+
 	spin_lock_irqsave(&pdev->phy_lock, flags);
 	pchan->phy->vchan = NULL;
 	pchan->phy = NULL;
-- 
1.7.5.4

Re: [PATCH 2/2] dma: mmp_pdma: clear DRCMR when free a phy channel

[Andy Shevchenko]

On Mon, Jun 3, 2013 at 11:02 AM, Xiang Wang <wangxfdu@gmail.com> wrote:
> From: Xiang Wang <wangx@marvell.com>
>
> In mmp pdma, phy channels are allocated/freed dynamically.
> The mapping from DMA request to DMA channel number in DRCMR
> should be cleared when a phy channel is freed. Otherwise
> conflicts will happen when:
> 1. A is using channel 2 and free it after finished, but A
> still maps to channel 2 in DRCMR of A.
> 2. Now another one B gets channel 2. So B maps to channel 2
> too in DRCMR of B.
> In the datasheet, it is described that "Do not map two active
> requests to the same channel since it produces unpredictable
> results" and we can observe that during test.


> --- a/drivers/dma/mmp_pdma.c
> +++ b/drivers/dma/mmp_pdma.c
> @@ -252,9 +252,15 @@ static void free_phy(struct mmp_pdma_chan *pchan)
>  {
>         struct mmp_pdma_device *pdev = to_mmp_pdma_dev(pchan->chan.device);
>         unsigned long flags;
> +       u32 reg;

May be empty line here (and maybe even in previous patch)?

>         if (!pchan->phy)
>                 return;
>
> +       /* clear the channel mapping in DRCMR */
> +       reg = pchan->phy->vchan->drcmr;
> +       reg = (((reg) < 64) ? 0x0100 : 0x1100) + (((reg) & 0x3f) << 2);

Too many braces.
It is not a macro, you don't need to embrace reg.

> +       writel(0, pchan->phy->base + reg);
> +
>         spin_lock_irqsave(&pdev->phy_lock, flags);
>         pchan->phy->vchan = NULL;
>         pchan->phy = NULL;



--
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] dma: mmp_pdma: add protect when alloc/free phy channels

[Vinod Koul]

On Mon, Jun 03, 2013 at 04:02:08PM +0800, Xiang Wang wrote:
> From: Xiang Wang <wangx@marvell.com>
> 
> In mmp pdma, phy channels are allocated/freed dynamically
> and frequently. But no proper protection is added.
> Conflict will happen when multi-users are requesting phy
> channels at the same time. Use spinlock to protect.
> 
> Signed-off-by: Xiang Wang <wangx@marvell.com>
> ---
>  drivers/dma/mmp_pdma.c |   41 +++++++++++++++++++++++++----------------
>  1 files changed, 25 insertions(+), 16 deletions(-)
> 

> +static void free_phy(struct mmp_pdma_chan *pchan)
pls namespace this
> +{
> +	struct mmp_pdma_device *pdev = to_mmp_pdma_dev(pchan->chan.device);
> +	unsigned long flags;
empty line pls

> +	if (!pchan->phy)
> +		return;
> +
> +	spin_lock_irqsave(&pdev->phy_lock, flags);
> +	pchan->phy->vchan = NULL;
> +	pchan->phy = NULL;
> +	spin_unlock_irqrestore(&pdev->phy_lock, flags);
> +}
> +
rest looks okay

--
~Vinod

Re: [PATCH 1/2] dma: mmp_pdma: add protect when alloc/free phy channels

[Xiang Wang]

2013/6/17 Vinod Koul <vinod.koul@intel.com>:
> On Mon, Jun 03, 2013 at 04:02:08PM +0800, Xiang Wang wrote:
>> From: Xiang Wang <wangx@marvell.com>
>>
>> In mmp pdma, phy channels are allocated/freed dynamically
>> and frequently. But no proper protection is added.
>> Conflict will happen when multi-users are requesting phy
>> channels at the same time. Use spinlock to protect.
>>
>> Signed-off-by: Xiang Wang <wangx@marvell.com>
>> ---
>>  drivers/dma/mmp_pdma.c |   41 +++++++++++++++++++++++++----------------
>>  1 files changed, 25 insertions(+), 16 deletions(-)
>>
>
>> +static void free_phy(struct mmp_pdma_chan *pchan)
> pls namespace this
Updated in patch set V3. Thanks!
>> +{
>> +     struct mmp_pdma_device *pdev = to_mmp_pdma_dev(pchan->chan.device);
>> +     unsigned long flags;
> empty line pls
Updated in patch set V3. Thanks!
>
>> +     if (!pchan->phy)
>> +             return;
>> +
>> +     spin_lock_irqsave(&pdev->phy_lock, flags);
>> +     pchan->phy->vchan = NULL;
>> +     pchan->phy = NULL;
>> +     spin_unlock_irqrestore(&pdev->phy_lock, flags);
>> +}
>> +
> rest looks okay
>
> --
> ~Vinod



--
Regards,
Xiang