[PATCH] dma: Revert "dmaengine: mic_x100: add missing spin_unlock"

[PATCH] dma: Revert "dmaengine: mic_x100: add missing spin_unlock"

[Ashutosh Dixit]

This reverts commit e958e079e254 ("dmaengine: mic_x100: add missing
spin_unlock").

The above patch is incorrect. There is nothing wrong with the original
code. The spin_lock is acquired in the "prep" functions and released
in "submit".

Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/dma/mic_x100_dma.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/dma/mic_x100_dma.c b/drivers/dma/mic_x100_dma.c
index cddfa8d..068e920 100644
--- a/drivers/dma/mic_x100_dma.c
+++ b/drivers/dma/mic_x100_dma.c
@@ -317,7 +317,6 @@ mic_dma_prep_memcpy_lock(struct dma_chan *ch, dma_addr_t dma_dest,
 	struct mic_dma_chan *mic_ch = to_mic_dma_chan(ch);
 	struct device *dev = mic_dma_ch_to_device(mic_ch);
 	int result;
-	struct dma_async_tx_descriptor *tx = NULL;
 
 	if (!len && !flags)
 		return NULL;
@@ -325,13 +324,10 @@ mic_dma_prep_memcpy_lock(struct dma_chan *ch, dma_addr_t dma_dest,
 	spin_lock(&mic_ch->prep_lock);
 	result = mic_dma_do_dma(mic_ch, flags, dma_src, dma_dest, len);
 	if (result >= 0)
-		tx = allocate_tx(mic_ch);
-
-	if (!tx)
-		dev_err(dev, "Error enqueueing dma, error=%d\n", result);
-
+		return allocate_tx(mic_ch);
+	dev_err(dev, "Error enqueueing dma, error=%d\n", result);
 	spin_unlock(&mic_ch->prep_lock);
-	return tx;
+	return NULL;
 }
 
 static struct dma_async_tx_descriptor *
@@ -339,14 +335,13 @@ mic_dma_prep_interrupt_lock(struct dma_chan *ch, unsigned long flags)
 {
 	struct mic_dma_chan *mic_ch = to_mic_dma_chan(ch);
 	int ret;
-	struct dma_async_tx_descriptor *tx = NULL;
 
 	spin_lock(&mic_ch->prep_lock);
 	ret = mic_dma_do_dma(mic_ch, flags, 0, 0, 0);
 	if (!ret)
-		tx = allocate_tx(mic_ch);
+		return allocate_tx(mic_ch);
 	spin_unlock(&mic_ch->prep_lock);
-	return tx;
+	return NULL;
 }
 
 /* Return the status of the transaction */
-- 
2.0.0.rc3.2.g998f840

Re: [PATCH] dma: Revert "dmaengine: mic_x100: add missing spin_unlock"

[Saurabh Sengar]

On 23 December 2015 at 09:05, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote:
> This reverts commit e958e079e254 ("dmaengine: mic_x100: add missing
> spin_unlock").
>
> The above patch is incorrect. There is nothing wrong with the original
> code. The spin_lock is acquired in the "prep" functions and released
> in "submit".

Hi Ashutosh,

If it is need to be released by submit function, we don't require the
spin_unlock on success case as well.
am I correct ?

> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  drivers/dma/mic_x100_dma.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/dma/mic_x100_dma.c b/drivers/dma/mic_x100_dma.c
> index cddfa8d..068e920 100644
> --- a/drivers/dma/mic_x100_dma.c
> +++ b/drivers/dma/mic_x100_dma.c
> @@ -317,7 +317,6 @@ mic_dma_prep_memcpy_lock(struct dma_chan *ch, dma_addr_t dma_dest,
>         struct mic_dma_chan *mic_ch = to_mic_dma_chan(ch);
>         struct device *dev = mic_dma_ch_to_device(mic_ch);
>         int result;
> -       struct dma_async_tx_descriptor *tx = NULL;
>
>         if (!len && !flags)
>                 return NULL;
> @@ -325,13 +324,10 @@ mic_dma_prep_memcpy_lock(struct dma_chan *ch, dma_addr_t dma_dest,
>         spin_lock(&mic_ch->prep_lock);
>         result = mic_dma_do_dma(mic_ch, flags, dma_src, dma_dest, len);
>         if (result >= 0)
> -               tx = allocate_tx(mic_ch);
> -
> -       if (!tx)
> -               dev_err(dev, "Error enqueueing dma, error=%d\n", result);
> -
> +               return allocate_tx(mic_ch);
> +       dev_err(dev, "Error enqueueing dma, error=%d\n", result);
>         spin_unlock(&mic_ch->prep_lock);

This spin_unlock shouldn't be required as explained it is getting
released by submit function

> -       return tx;
> +       return NULL;
>  }
>
>  static struct dma_async_tx_descriptor *
> @@ -339,14 +335,13 @@ mic_dma_prep_interrupt_lock(struct dma_chan *ch, unsigned long flags)
>  {
>         struct mic_dma_chan *mic_ch = to_mic_dma_chan(ch);
>         int ret;
> -       struct dma_async_tx_descriptor *tx = NULL;
>
>         spin_lock(&mic_ch->prep_lock);
>         ret = mic_dma_do_dma(mic_ch, flags, 0, 0, 0);
>         if (!ret)
> -               tx = allocate_tx(mic_ch);
> +               return allocate_tx(mic_ch);
>         spin_unlock(&mic_ch->prep_lock);

and this too ?

> -       return tx;
> +       return NULL;
>  }
>
>  /* Return the status of the transaction */
> --
> 2.0.0.rc3.2.g998f840
>

Re: [PATCH] dma: Revert "dmaengine: mic_x100: add missing spin_unlock"

[Ashutosh Dixit]

On Wed, Dec 23 2015 at 12:45:31 AM, Saurabh Sengar <saurabh.truth@gmail.com> wrote:
> On 23 December 2015 at 09:05, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote:
>> This reverts commit e958e079e254 ("dmaengine: mic_x100: add missing
>> spin_unlock").
>>
>> The above patch is incorrect. There is nothing wrong with the original
>> code. The spin_lock is acquired in the "prep" functions and released
>> in "submit".
>
> Hi Ashutosh,
>
> If it is need to be released by submit function, we don't require the
> spin_unlock on success case as well.
> am I correct ?

No, you are wrong. In the prep functions, we are not using spin_unlock in
success cases but in failure ones.

>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>> ---
>>  drivers/dma/mic_x100_dma.c | 15 +++++----------
>>  1 file changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/dma/mic_x100_dma.c b/drivers/dma/mic_x100_dma.c
>> index cddfa8d..068e920 100644
>> --- a/drivers/dma/mic_x100_dma.c
>> +++ b/drivers/dma/mic_x100_dma.c
>> @@ -317,7 +317,6 @@ mic_dma_prep_memcpy_lock(struct dma_chan *ch, dma_addr_t dma_dest,
>>         struct mic_dma_chan *mic_ch = to_mic_dma_chan(ch);
>>         struct device *dev = mic_dma_ch_to_device(mic_ch);
>>         int result;
>> -       struct dma_async_tx_descriptor *tx = NULL;
>>
>>         if (!len && !flags)
>>                 return NULL;
>> @@ -325,13 +324,10 @@ mic_dma_prep_memcpy_lock(struct dma_chan *ch, dma_addr_t dma_dest,
>>         spin_lock(&mic_ch->prep_lock);
>>         result = mic_dma_do_dma(mic_ch, flags, dma_src, dma_dest, len);
>>         if (result >= 0)
>> -               tx = allocate_tx(mic_ch);
>> -
>> -       if (!tx)
>> -               dev_err(dev, "Error enqueueing dma, error=%d\n", result);
>> -
>> +               return allocate_tx(mic_ch);
>> +       dev_err(dev, "Error enqueueing dma, error=%d\n", result);
>>         spin_unlock(&mic_ch->prep_lock);
>
> This spin_unlock shouldn't be required as explained it is getting
> released by submit function

This is a failure case. Submit will not be called in this case so
spin_unlock needs to be called here.

>> -       return tx;
>> +       return NULL;
>>  }
>>
>>  static struct dma_async_tx_descriptor *
>> @@ -339,14 +335,13 @@ mic_dma_prep_interrupt_lock(struct dma_chan *ch, unsigned long flags)
>>  {
>>         struct mic_dma_chan *mic_ch = to_mic_dma_chan(ch);
>>         int ret;
>> -       struct dma_async_tx_descriptor *tx = NULL;
>>
>>         spin_lock(&mic_ch->prep_lock);
>>         ret = mic_dma_do_dma(mic_ch, flags, 0, 0, 0);
>>         if (!ret)
>> -               tx = allocate_tx(mic_ch);
>> +               return allocate_tx(mic_ch);
>>         spin_unlock(&mic_ch->prep_lock);
>
> and this too ?

Yes, this too.

>> -       return tx;
>> +       return NULL;
>>  }
>>
>>  /* Return the status of the transaction */
>> --
>> 2.0.0.rc3.2.g998f840
>>

Re: [PATCH] dma: Revert "dmaengine: mic_x100: add missing spin_unlock"

[Vinod Koul]

On Tue, Dec 22, 2015 at 07:35:23PM -0800, Ashutosh Dixit wrote:
> This reverts commit e958e079e254 ("dmaengine: mic_x100: add missing
> spin_unlock").
> 
> The above patch is incorrect. There is nothing wrong with the original
> code. The spin_lock is acquired in the "prep" functions and released
> in "submit".

And going by dmaengine sematics, I do not think that is entrely right.

A user may choose to prepare multiple desciptors and then sumbit later,
looking at code I do not see how that will work.

Please fix that


> 
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  drivers/dma/mic_x100_dma.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/dma/mic_x100_dma.c b/drivers/dma/mic_x100_dma.c
> index cddfa8d..068e920 100644
> --- a/drivers/dma/mic_x100_dma.c
> +++ b/drivers/dma/mic_x100_dma.c
> @@ -317,7 +317,6 @@ mic_dma_prep_memcpy_lock(struct dma_chan *ch, dma_addr_t dma_dest,
>  	struct mic_dma_chan *mic_ch = to_mic_dma_chan(ch);
>  	struct device *dev = mic_dma_ch_to_device(mic_ch);
>  	int result;
> -	struct dma_async_tx_descriptor *tx = NULL;
>  
>  	if (!len && !flags)
>  		return NULL;
> @@ -325,13 +324,10 @@ mic_dma_prep_memcpy_lock(struct dma_chan *ch, dma_addr_t dma_dest,
>  	spin_lock(&mic_ch->prep_lock);
>  	result = mic_dma_do_dma(mic_ch, flags, dma_src, dma_dest, len);
>  	if (result >= 0)
> -		tx = allocate_tx(mic_ch);
> -
> -	if (!tx)
> -		dev_err(dev, "Error enqueueing dma, error=%d\n", result);
> -
> +		return allocate_tx(mic_ch);
> +	dev_err(dev, "Error enqueueing dma, error=%d\n", result);
>  	spin_unlock(&mic_ch->prep_lock);
> -	return tx;
> +	return NULL;
>  }
>  
>  static struct dma_async_tx_descriptor *
> @@ -339,14 +335,13 @@ mic_dma_prep_interrupt_lock(struct dma_chan *ch, unsigned long flags)
>  {
>  	struct mic_dma_chan *mic_ch = to_mic_dma_chan(ch);
>  	int ret;
> -	struct dma_async_tx_descriptor *tx = NULL;
>  
>  	spin_lock(&mic_ch->prep_lock);
>  	ret = mic_dma_do_dma(mic_ch, flags, 0, 0, 0);
>  	if (!ret)
> -		tx = allocate_tx(mic_ch);
> +		return allocate_tx(mic_ch);
>  	spin_unlock(&mic_ch->prep_lock);
> -	return tx;
> +	return NULL;
>  }
>  
>  /* Return the status of the transaction */
> -- 
> 2.0.0.rc3.2.g998f840
> 

-- 
~Vinod

Re: [PATCH] dma: Revert "dmaengine: mic_x100: add missing spin_unlock"

[Lars-Peter Clausen]

On 01/04/2016 04:35 AM, Vinod Koul wrote:
> On Tue, Dec 22, 2015 at 07:35:23PM -0800, Ashutosh Dixit wrote:
>> This reverts commit e958e079e254 ("dmaengine: mic_x100: add missing
>> spin_unlock").
>>
>> The above patch is incorrect. There is nothing wrong with the original
>> code. The spin_lock is acquired in the "prep" functions and released
>> in "submit".
> 
> And going by dmaengine sematics, I do not think that is entrely right.
> 
> A user may choose to prepare multiple desciptors and then sumbit later,
> looking at code I do not see how that will work.

The DMAengine API actually mandates that prep and submit must always be
called in pairs, without any other DMAengine calls in between. The patch is
correct.

Quoting from Documentation/dmaengine/client.txt:

   Once a descriptor has been obtained, the callback information can be
   added and the descriptor must then be submitted.  Some DMA engine
   drivers may hold a spinlock between a successful preparation and
   submission so it is important that these two operations are closely
   paired.

Re: [PATCH] dma: Revert "dmaengine: mic_x100: add missing spin_unlock"

[Vinod Koul]

On Mon, Jan 04, 2016 at 03:35:34PM +0100, Lars-Peter Clausen wrote:
> On 01/04/2016 04:35 AM, Vinod Koul wrote:
> > On Tue, Dec 22, 2015 at 07:35:23PM -0800, Ashutosh Dixit wrote:
> >> This reverts commit e958e079e254 ("dmaengine: mic_x100: add missing
> >> spin_unlock").
> >>
> >> The above patch is incorrect. There is nothing wrong with the original
> >> code. The spin_lock is acquired in the "prep" functions and released
> >> in "submit".
> > 
> > And going by dmaengine sematics, I do not think that is entrely right.
> > 
> > A user may choose to prepare multiple desciptors and then sumbit later,
> > looking at code I do not see how that will work.
> 
> The DMAengine API actually mandates that prep and submit must always be
> called in pairs, without any other DMAengine calls in between. The patch is
> correct.
> 
> Quoting from Documentation/dmaengine/client.txt:
> 
>    Once a descriptor has been obtained, the callback information can be
>    added and the descriptor must then be submitted.  Some DMA engine
>    drivers may hold a spinlock between a successful preparation and
>    submission so it is important that these two operations are closely
>    paired.

This is true for slave cases as has been made clear in the Documentation.
For non slave cases that is not entirely right. mic_x100 falls in latter
category.

Thanks
-- 
~Vinod

Re: [PATCH] dma: Revert "dmaengine: mic_x100: add missing spin_unlock"

[Ashutosh Dixit]

On Sun, Jan 03 2016 at 10:35:26 PM, "Koul, Vinod" <vinod.koul@intel.com> wrote:
> On Tue, Dec 22, 2015 at 07:35:23PM -0800, Ashutosh Dixit wrote:
>> This reverts commit e958e079e254 ("dmaengine: mic_x100: add missing
>> spin_unlock").
>>
>> The above patch is incorrect. There is nothing wrong with the original
>> code. The spin_lock is acquired in the "prep" functions and released
>> in "submit".
>
> And going by dmaengine sematics, I do not think that is entrely right.
>
> A user may choose to prepare multiple desciptors and then sumbit later,
> looking at code I do not see how that will work.
>
> Please fix that

The mic_x100_dma driver still allows a client to prepare and submit
multiple descriptors and triggers the hardware only when issue_pending
is called (or a threshold is exceeded). Identical coding patterns exist
in the IOAT dma driver, on which the mic_x100_dma driver is based.

Further, mic_x100 dma channels are private and used only by other MIC
drivers such as SCIF (drivers/misc/mic/scif). These drivers obviously
alternate prep and submit calls as required by mic_x100_dma. We do not
envisage a wider use of the mic_x100_dma driver at this point.

A change such as allowing multiple prep's before a submit requires large
scale changes in the driver. In the absence of a real use case, there is
no plan to make such changes at present. At this point we only want the
driver to be restored to its previously functional state for the 4.4
kernel. The patch in question results in system lockups so it is a
serious v4.4 regression.

Re: [PATCH] dma: Revert "dmaengine: mic_x100: add missing spin_unlock"

[Vinod Koul]

On Mon, Jan 04, 2016 at 05:40:39PM -0500, Ashutosh Dixit wrote:
> On Sun, Jan 03 2016 at 10:35:26 PM, "Koul, Vinod" <vinod.koul@intel.com> wrote:
> > On Tue, Dec 22, 2015 at 07:35:23PM -0800, Ashutosh Dixit wrote:
> >> This reverts commit e958e079e254 ("dmaengine: mic_x100: add missing
> >> spin_unlock").
> >>
> >> The above patch is incorrect. There is nothing wrong with the original
> >> code. The spin_lock is acquired in the "prep" functions and released
> >> in "submit".
> >
> > And going by dmaengine sematics, I do not think that is entrely right.
> >
> > A user may choose to prepare multiple desciptors and then sumbit later,
> > looking at code I do not see how that will work.
> >
> > Please fix that
> 
> The mic_x100_dma driver still allows a client to prepare and submit
> multiple descriptors and triggers the hardware only when issue_pending
> is called (or a threshold is exceeded). Identical coding patterns exist
> in the IOAT dma driver, on which the mic_x100_dma driver is based.
> 
> Further, mic_x100 dma channels are private and used only by other MIC
> drivers such as SCIF (drivers/misc/mic/scif). These drivers obviously
> alternate prep and submit calls as required by mic_x100_dma. We do not
> envisage a wider use of the mic_x100_dma driver at this point.

The whole point of using an API is to create standard usages, future clients
can come up and your argument is not future proof
> 
> A change such as allowing multiple prep's before a submit requires large
> scale changes in the driver. In the absence of a real use case, there is
> no plan to make such changes at present. At this point we only want the
> driver to be restored to its previously functional state for the 4.4
> kernel. The patch in question results in system lockups so it is a
> serious v4.4 regression.

I will revert it but please fix the driver..

-- 
~Vinod