Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1300459 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] dma: Revert "dmaengine: mic_x100: add missing  spin_unlock" Vinod Koul <vinod.koul@intel.com> - 2016-01-04 04:40 +0100
    Re: [PATCH] dma: Revert "dmaengine: mic_x100: add missing  spin_unlock" Lars-Peter Clausen <lars@metafoo.de> - 2016-01-04 15:40 +0100
      Re: [PATCH] dma: Revert "dmaengine: mic_x100: add missing  spin_unlock" Vinod Koul <vinod.koul@intel.com> - 2016-01-04 16:10 +0100
    Re: [PATCH] dma: Revert "dmaengine: mic_x100: add missing spin_unlock" Ashutosh Dixit <ashutosh.dixit@intel.com> - 2016-01-04 23:50 +0100
      Re: [PATCH] dma: Revert "dmaengine: mic_x100: add missing  spin_unlock" Vinod Koul <vinod.koul@intel.com> - 2016-01-06 11:10 +0100

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1300780

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300798

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1302518

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web