[patch 2.6.15-rc5-mm2] SPI, priority inversion tweak

[patch 2.6.15-rc5-mm2] SPI, priority inversion tweak

[David Brownell]

[-- Attachment #1: Type: text/plain, Size: 207 bytes --]

This is an updated version of the patch from Mark Underwood, handling
the no-memory case better and using SLAB_KERNEL not SLAB_ATOMIC.

Please apply it on top of the current SPI code in the MM tree.

- Dave

[-- Attachment #2: spi-kmalloc.patch --]
[-- Type: text/x-diff, Size: 1522 bytes --]

Update the SPI framework to remove a potential priority inversion case by
reverting to kmalloc if the pre-allocated DMA-safe buffer isn't available.

From: Mark Underwood <basicmark@yahoo.com>
Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

--- g26.orig/drivers/spi/spi.c	2005-12-11 11:06:38.000000000 -0800
+++ g26/drivers/spi/spi.c	2005-12-13 09:56:22.000000000 -0800
@@ -541,22 +541,30 @@ int spi_write_then_read(struct spi_devic
 	int			status;
 	struct spi_message	message;
 	struct spi_transfer	x[2];
+	u8			*local_buf;
 
 	/* Use preallocated DMA-safe buffer.  We can't avoid copying here,
 	 * (as a pure convenience thing), but we can keep heap costs
-	 * out of the hot path.
+	 * out of the hot path ...
 	 */
 	if ((n_tx + n_rx) > SPI_BUFSIZ)
 		return -EINVAL;
 
-	down(&lock);
+	/* ... unless someone else is using the pre-allocated buffer */
+	if (down_trylock(&lock)) {
+		local_buf = kmalloc(SPI_BUFSIZ, GFP_KERNEL);
+		if (!local_buf)
+			return -ENOMEM;
+	} else
+		local_buf = buf;
+
 	memset(x, 0, sizeof x);
 
-	memcpy(buf, txbuf, n_tx);
-	x[0].tx_buf = buf;
+	memcpy(local_buf, txbuf, n_tx);
+	x[0].tx_buf = local_buf;
 	x[0].len = n_tx;
 
-	x[1].rx_buf = buf + n_tx;
+	x[1].rx_buf = local_buf + n_tx;
 	x[1].len = n_rx;
 
 	/* do the i/o */
@@ -568,7 +576,11 @@ int spi_write_then_read(struct spi_devic
 		status = message.status;
 	}
 
-	up(&lock);
+	if (x[0].tx_buf == buf)
+		up(&lock);
+	else
+		kfree(local_buf);
+
 	return status;
 }
 EXPORT_SYMBOL_GPL(spi_write_then_read);

Re: [spi-devel-general] [patch 2.6.15-rc5-mm2] SPI, priority inversion tweak

[Vitaly Wool]

So you're turning this to be unsafe if the buffer is in use, right? Funny...

David Brownell wrote:

>This is an updated version of the patch from Mark Underwood, handling
>the no-memory case better and using SLAB_KERNEL not SLAB_ATOMIC.
>
>Please apply it on top of the current SPI code in the MM tree.
>
>- Dave
>  
>
>------------------------------------------------------------------------
>
>Update the SPI framework to remove a potential priority inversion case by
>reverting to kmalloc if the pre-allocated DMA-safe buffer isn't available.
>
>From: Mark Underwood <basicmark@yahoo.com>
>Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
>
>--- g26.orig/drivers/spi/spi.c	2005-12-11 11:06:38.000000000 -0800
>+++ g26/drivers/spi/spi.c	2005-12-13 09:56:22.000000000 -0800
>@@ -541,22 +541,30 @@ int spi_write_then_read(struct spi_devic
> 	int			status;
> 	struct spi_message	message;
> 	struct spi_transfer	x[2];
>+	u8			*local_buf;
> 
> 	/* Use preallocated DMA-safe buffer.  We can't avoid copying here,
> 	 * (as a pure convenience thing), but we can keep heap costs
>-	 * out of the hot path.
>+	 * out of the hot path ...
> 	 */
> 	if ((n_tx + n_rx) > SPI_BUFSIZ)
> 		return -EINVAL;
> 
>-	down(&lock);
>+	/* ... unless someone else is using the pre-allocated buffer */
>+	if (down_trylock(&lock)) {
>+		local_buf = kmalloc(SPI_BUFSIZ, GFP_KERNEL);
>+		if (!local_buf)
>+			return -ENOMEM;
>+	} else
>+		local_buf = buf;
>+
> 	memset(x, 0, sizeof x);
> 
>-	memcpy(buf, txbuf, n_tx);
>-	x[0].tx_buf = buf;
>+	memcpy(local_buf, txbuf, n_tx);
>+	x[0].tx_buf = local_buf;
> 	x[0].len = n_tx;
> 
>-	x[1].rx_buf = buf + n_tx;
>+	x[1].rx_buf = local_buf + n_tx;
> 	x[1].len = n_rx;
> 
> 	/* do the i/o */
>@@ -568,7 +576,11 @@ int spi_write_then_read(struct spi_devic
> 		status = message.status;
> 	}
> 
>-	up(&lock);
>+	if (x[0].tx_buf == buf)
>+		up(&lock);
>+	else
>+		kfree(local_buf);
>+
> 	return status;
> }
> EXPORT_SYMBOL_GPL(spi_write_then_read);
>  
>

Re: [spi-devel-general] [patch 2.6.15-rc5-mm2] SPI, priority inversion tweak

[David Brownell]

On Tuesday 13 December 2005 1:49 pm, Vitaly Wool wrote:
> So you're turning this to be unsafe if the buffer is in use, right? Funny...

I have no idea what you mean by that comment.  The parameters to that
function have always been documented as "will copy", and the two branches
(busy/not) differ only in _which_ buffer they use from the heap (the
fast pre-allocated one, or a freshly allocated scratch buffer).  Heap
buffers are by definition DMA-safe.

- Dave


> David Brownell wrote:
> 
> >This is an updated version of the patch from Mark Underwood, handling
> >the no-memory case better and using SLAB_KERNEL not SLAB_ATOMIC.
> >
> 

Re: [spi-devel-general] [patch 2.6.15-rc5-mm2] SPI, priority inversion tweak

[Vitaly Wool]

Okay, sorry, I've misundastood that.

Vitaly

David Brownell wrote:

>On Tuesday 13 December 2005 1:49 pm, Vitaly Wool wrote:
>  
>
>>So you're turning this to be unsafe if the buffer is in use, right? Funny...
>>    
>>
>
>I have no idea what you mean by that comment.  The parameters to that
>function have always been documented as "will copy", and the two branches
>(busy/not) differ only in _which_ buffer they use from the heap (the
>fast pre-allocated one, or a freshly allocated scratch buffer).  Heap
>buffers are by definition DMA-safe.
>
>- Dave
>
>
>  
>
>>David Brownell wrote:
>>
>>    
>>
>>>This is an updated version of the patch from Mark Underwood, handling
>>>the no-memory case better and using SLAB_KERNEL not SLAB_ATOMIC.
>>>
>>>      
>>>
>
>
>  
>

Re: [spi-devel-general] [patch 2.6.15-rc5-mm2] SPI, priority inversion tweak

[Vitaly Wool]

Hmm...

>-	down(&lock);
>+	/* ... unless someone else is using the pre-allocated buffer */
>+	if (down_trylock(&lock)) {
>+		local_buf = kmalloc(SPI_BUFSIZ, GFP_KERNEL);
>+		if (!local_buf)
>+			return -ENOMEM;
>+	} else
>+		local_buf = buf;
>+
>  
>
Okay, so suppose we have two controller drivers working in two threads 
and calling write_then_read in such a way that the one called later has 
to allocate a new buffer. Suppose also that both controller drivers are 
working in PIO mode. In this situation you have one redundant kmalloc 
and two redundant memcpy's, not speaking about overhead brought up by 
mutexes. Bad!
So I still can't say I'm accepting this approach.

Worth mentioning is that the approach I propose is free from this drawback.

Vitaly