Skip to contentSkip to conversation

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

PATCH New Uwe Kleine-König 2 participants 8 messages Mar 18, 2015 – Mar 19, 2015 SPI

lore
Share & tools
lore

Uwe Kleine-König

author
#0 lore raw

PATCH 1/1 spi: trigger trace event for message-done before mesg->complete

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 <[email protected]>
---
 drivers/spi/spi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/spi/spi.c +3 -2
index 6d3b9b76c80b..df577518b770 100644
@@ -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);
Signature · 1 line
2.1.4
maintainer
#1 lore raw

PATCH 1/1 spi: trigger trace event for message-done before mesg->complete

Quoted 6 lines · 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.

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>

author
#2 lore raw

PATCH 1/1 spi: trigger trace event for message-done before mesg->complete

Quoted 10 lines · Mark Brown
Quoted 6 lines · 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.

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

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
maintainer
#3 lore raw

PATCH 1/1 spi: trigger trace event for message-done before mesg->complete

Quoted 7 lines · Uwe Kleine-K?nig
Quoted 2 lines · Mark Brown

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>

author
#4 lore raw

PATCH 1/1 spi: trigger trace event for message-done before mesg->complete

Quoted 6 lines · Mark Brown
Quoted 4 lines · Uwe Kleine-K?nig
Quoted 2 lines · Mark Brown

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

Quoted 7 lines
Quoted 2 lines

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

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
maintainer
#5 lore raw

PATCH 1/1 spi: trigger trace event for message-done before mesg->complete

Quoted 10 lines · Uwe Kleine-K?nig
Quoted 4 lines · Mark Brown

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>

author
#6 lore raw

PATCH 1/1 spi: trigger trace event for message-done before mesg->complete

Hello Mark,

Quoted 8 lines · Mark Brown
Quoted 6 lines · Uwe Kleine-K?nig
Quoted 4 lines · Mark Brown

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.

Quoted 7 lines
Quoted 3 lines

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

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
maintainer
#7 lore raw

PATCH 1/1 spi: trigger trace event for message-done before mesg->complete

Quoted 6 lines · Uwe Kleine-K?nig
Quoted 3 lines · Mark Brown

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".

Quoted 7 lines

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>