[PATCH] spi: trigger trace event for message-done before mesg->complete

[PATCH] spi: trigger trace event for message-done before mesg->complete

[Uwe Kleine-König]

With spidev the mesg->complete callback points to spidev_complete.
Calling this unblocks spidev_sync and so spidev_sync_write finishes. As
the struct spi_message just read is a local variable in
spidev_sync_write and recording the trace event accesses this message
the recording is better done first. The same can happen for
spidev_sync_read.

This fixes an oops observed on a 3.14-rt system with spidev activity
after

	echo 1 > /sys/kernel/debug/tracing/events/spi/enable

.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 6d3b9b76c80b..df577518b770 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -994,13 +994,14 @@ void spi_finalize_current_message(struct spi_master *master)
 				"failed to unprepare message: %d\n", ret);
 		}
 	}
+
+	trace_spi_message_done(mesg);
+
 	master->cur_msg_prepared = false;
 
 	mesg->state = NULL;
 	if (mesg->complete)
 		mesg->complete(mesg->context);
-
-	trace_spi_message_done(mesg);
 }
 EXPORT_SYMBOL_GPL(spi_finalize_current_message);
 
-- 
2.1.4

[PATCH] spi: trigger trace event for message-done before mesg->complete

[Mark Brown]

On Wed, Mar 18, 2015 at 11:27:28AM +0100, Uwe Kleine-K?nig wrote:
> With spidev the mesg->complete callback points to spidev_complete.
> Calling this unblocks spidev_sync and so spidev_sync_write finishes. As
> the struct spi_message just read is a local variable in
> spidev_sync_write and recording the trace event accesses this message
> the recording is better done first. The same can happen for
> spidev_sync_read.

Applied, thanks.  I'm a bit suspicious that the changelog talks only
about spidev though...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150318/bc861a72/attachment.sig>

[PATCH] spi: trigger trace event for message-done before mesg->complete

[Uwe Kleine-König]

On Wed, Mar 18, 2015 at 11:36:35AM +0000, Mark Brown wrote:
> On Wed, Mar 18, 2015 at 11:27:28AM +0100, Uwe Kleine-K?nig wrote:
> > With spidev the mesg->complete callback points to spidev_complete.
> > Calling this unblocks spidev_sync and so spidev_sync_write finishes. As
> > the struct spi_message just read is a local variable in
> > spidev_sync_write and recording the trace event accesses this message
> > the recording is better done first. The same can happen for
> > spidev_sync_read.
> 
> Applied, thanks.  I'm a bit suspicious that the changelog talks only
> about spidev though...
That's my workflow. Maybe the important thing here is also that I use
-rt?!

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] spi: trigger trace event for message-done before mesg->complete

[Mark Brown]

On Wed, Mar 18, 2015 at 02:32:26PM +0100, Uwe Kleine-K?nig wrote:
> On Wed, Mar 18, 2015 at 11:36:35AM +0000, Mark Brown wrote:

> > Applied, thanks.  I'm a bit suspicious that the changelog talks only
> > about spidev though...

> That's my workflow. Maybe the important thing here is also that I use
> -rt?!

My point here is that the analysis of the issue shouldn't depend on
spidev in particular, if you need to call out the specific driver you're
working with that's an alarm sign that it's doing something weird and
perhaps the problem is with the driver.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150318/0c57f72d/attachment.sig>

[PATCH] spi: trigger trace event for message-done before mesg->complete

[Uwe Kleine-König]

On Wed, Mar 18, 2015 at 01:58:54PM +0000, Mark Brown wrote:
> On Wed, Mar 18, 2015 at 02:32:26PM +0100, Uwe Kleine-K?nig wrote:
> > On Wed, Mar 18, 2015 at 11:36:35AM +0000, Mark Brown wrote:
> 
> > > Applied, thanks.  I'm a bit suspicious that the changelog talks only
> > > about spidev though...
> 
> > That's my workflow. Maybe the important thing here is also that I use
> > -rt?!
> 
> My point here is that the analysis of the issue shouldn't depend on
> spidev in particular, if you need to call out the specific driver you're
> working with that's an alarm sign that it's doing something weird and
> perhaps the problem is with the driver.
This just happens to be the driver I saw the problem with. Don't have
another spi device on that bus to cross check with other drivers. I
wouldn't be too concerned here.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] spi: trigger trace event for message-done before mesg->complete

[Mark Brown]

On Wed, Mar 18, 2015 at 03:48:54PM +0100, Uwe Kleine-K?nig wrote:
> On Wed, Mar 18, 2015 at 01:58:54PM +0000, Mark Brown wrote:

> > My point here is that the analysis of the issue shouldn't depend on
> > spidev in particular, if you need to call out the specific driver you're
> > working with that's an alarm sign that it's doing something weird and
> > perhaps the problem is with the driver.

> This just happens to be the driver I saw the problem with. Don't have
> another spi device on that bus to cross check with other drivers. I
> wouldn't be too concerned here.

Sure, but it's better to write it up in terms of a generic driver - it's
the difference between "let's work around this driver" and "the core
isn't doing the right thing for drivers here".
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150319/7062fe0f/attachment.sig>

[PATCH] spi: trigger trace event for message-done before mesg->complete

[Uwe Kleine-König]

Hello Mark,

On Thu, Mar 19, 2015 at 11:24:30AM +0000, Mark Brown wrote:
> On Wed, Mar 18, 2015 at 03:48:54PM +0100, Uwe Kleine-K?nig wrote:
> > On Wed, Mar 18, 2015 at 01:58:54PM +0000, Mark Brown wrote:
> 
> > > My point here is that the analysis of the issue shouldn't depend on
> > > spidev in particular, if you need to call out the specific driver you're
> > > working with that's an alarm sign that it's doing something weird and
> > > perhaps the problem is with the driver.
> 
> > This just happens to be the driver I saw the problem with. Don't have
> > another spi device on that bus to cross check with other drivers. I
> > wouldn't be too concerned here.
> 
> Sure, but it's better to write it up in terms of a generic driver - it's
> the difference between "let's work around this driver" and "the core
> isn't doing the right thing for drivers here".
OK, so something like:

------------>8------------
spi: trigger trace event for message-done before mesg->complete

The message's complete callback might (permissibly) free the memory that
holds the message. As recording the trace event for the end of a
transfer accesses this message the recording is better done first.

This fixes an oops observed on a 3.14-rt system with bus activity using
spidev after

        echo 1 > /sys/kernel/debug/tracing/events/spi/enable

. (For spidev mesg->complete points to spidev_complete. Calling that
unblocks spidev_sync and so spidev_sync_write (or spidev_sync_read).
This in turn leaves the scope of the local variable that holds the
message.)
------------>8------------

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] spi: trigger trace event for message-done before mesg->complete

[Mark Brown]

On Thu, Mar 19, 2015 at 12:48:42PM +0100, Uwe Kleine-K?nig wrote:
> On Thu, Mar 19, 2015 at 11:24:30AM +0000, Mark Brown wrote:

> > Sure, but it's better to write it up in terms of a generic driver - it's
> > the difference between "let's work around this driver" and "the core
> > isn't doing the right thing for drivers here".

> OK, so something like:

> spi: trigger trace event for message-done before mesg->complete
> 
> The message's complete callback might (permissibly) free the memory that
> holds the message. As recording the trace event for the end of a
> transfer accesses this message the recording is better done first.

Yes, that's the sort of thing I was looking for - it just makes the
review a bit easier.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150319/3e2f9f1f/attachment.sig>