spi: remove some spidev oops-on-rmmod paths
PATCH 1/1 spi: remove some spidev oops-on-rmmod paths
Somehow the spidev code forgot to include a critical mechanism:
when the underlying device is removed (e.g. spi_master rmmod), open file descriptors must be prevented from issuing new I/O requests to that device. On penalty of the oopsing reported by Sebastian Siewior <[email protected]> ...
This is a partial fix, adding handshaking between the lower level (SPI messaging) and the file operations using the spi_dev. There's still a refcounting issue to be addressed (separately) with respect to the spidev_data itself.
--- drivers/spi/spidev.c | 82 ++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 72 insertions(+), 10 deletions(-)
drivers/spi/spidev.c +86 -13
@@ -68,6 +68,7 @@ static unsigned long minors[N_SPI_MINORSstruct spidev_data {struct device dev; +spinlock_t spi_lock;struct spi_device *spi;struct list_head device_entry;
@@ -85,12 +86,72 @@ MODULE_PARM_DESC(bufsiz, "data bytes in/*-------------------------------------------------------------------------*/ +/* +* We can't use the standard synchronous wrappers for file I/O; we +* need to protect against async removal of the underlying spi_device. +*/ +static void spidev_complete (void *arg) +{ +complete (arg); +} + +static int spidev_sync (struct spidev_data *spidev,struct spi_message *message) +{ +DECLARE_COMPLETION_ONSTACK (done); +int status; + + message->complete = spidev_complete; + message->context = &done; + +spin_lock_irq (&spidev->spi_lock); +if (spidev->spi ==NULL ) + status = -ESHUTDOWN ; +else + status =spi_async (spidev->spi, message); +spin_unlock_irq (&spidev->spi_lock); + +if (status ==0 ) { +wait_for_completion (&done); + status = message->status; + } +return status; +} + +static inline int +spidev_sync_write (struct spidev_data *spidev,size_t len) +{ +struct spi_transfer t = { + .tx_buf = spidev->buffer, + .len = len, + }; +struct spi_message m; + +spi_message_init (&m); +spi_message_add_tail (&t, &m); +return spidev_sync (spidev, &m); +} + +static inline int +spidev_sync_read (struct spidev_data *spidev,size_t len) +{ +struct spi_transfer t = { + .rx_buf = spidev->buffer, + .len = len, + }; +struct spi_message m; + +spi_message_init (&m); +spi_message_add_tail (&t, &m); +return spidev_sync (spidev, &m); +} + +/*-------------------------------------------------------------------------*/ +/* Read-only message with current device setup */ static ssize_t spidev_read (struct file *filp,char __user *buf,size_t count,loff_t *f_pos) {struct spidev_data *spidev; -struct spi_device *spi;ssize_t status =0 ;/* chipselect only toggles at start or end of operation */
@@ -98,10 +159,9 @@ spidev_read(struct file *filp, char __usreturn -EMSGSIZE ; spidev = filp->private_data; - spi = spidev->spi;mutex_lock (&spidev->buf_lock); - status =spi_read (spi, spidev->buffer, count); + status =spidev_sync_read (spidev, count);if (status ==0 ) {unsigned long missing;
@@ -122,7 +182,6 @@ spidev_write(struct file *filp, const ch size_t count, loff_t *f_pos) { struct spidev_data *spidev; -struct spi_device *spi;ssize_t status =0 ;unsigned long missing;
@@ -131,12 +190,11 @@ spidev_write(struct file *filp, const chreturn -EMSGSIZE ; spidev = filp->private_data; - spi = spidev->spi;mutex_lock (&spidev->buf_lock); missing =copy_from_user (spidev->buffer, buf, count);if (missing ==0 ) { - status =spi_write (spi, spidev->buffer, count); + status =spidev_sync_write (spidev, count);if (status ==0 ) status = count; }else
@@ -153,7 +211,6 @@ static int spidev_message(struct spidev_struct spi_transfer *k_xfers;struct spi_transfer *k_tmp;struct spi_ioc_transfer *u_tmp; -struct spi_device *spi = spidev->spi;unsigned n, total;u8 *buf;int status = -EFAULT ;
@@ -215,7 +272,7 @@ static int spidev_message(struct spidev_spi_message_add_tail (k_tmp, &msg); } - status =spi_sync (spi, &msg); + status =spidev_sync (spidev, &msg);if (status <0 ) goto done;
@@ -269,8 +326,16 @@ spidev_ioctl(struct inode *inode, structif (err)return -EFAULT ; +/* guard against device removal before, or while, +* we issue this ioctl. +*/ spidev = filp->private_data; - spi = spidev->spi; +spin_lock_irq (&spidev->spi_lock); + spi =spi_dev_get (spidev->spi); +spin_unlock_irq (&spidev->spi_lock); + +if (spi ==NULL ) +return ESHUTDOWN ;switch (cmd) {/* read requests */
@@ -356,8 +421,10 @@ spidev_ioctl(struct inode *inode, structdefault :/* segmented and/or full-duplex I/O request */ if (_IOC_NR (cmd) !=_IOC_NR (SPI_IOC_MESSAGE (0 )) - ||_IOC_DIR (cmd) != _IOC_WRITE) -return -ENOTTY ; + ||_IOC_DIR (cmd) != _IOC_WRITE) { + retval = -ENOTTY ; +break ; + } tmp =_IOC_SIZE (cmd);if ((tmp %sizeof (struct spi_ioc_transfer )) !=0 ) {
@@ -385,6 +452,7 @@ spidev_ioctl(struct inode *inode, structkfree (ioc);break ; } +spi_dev_put (spi);return retval; }
@@ -488,6 +556,7 @@ static int spidev_probe(struct spi_devic/* Initialize the driver data */ spidev->spi = spi; +spin_lock_init (&spidev->spi_lock);mutex_init (&spidev->buf_lock);INIT_LIST_HEAD (&spidev->device_entry);
@@ -526,13 +595,17 @@ static int spidev_remove(struct spi_devi {struct spidev_data *spidev =dev_get_drvdata (&spi->dev); -mutex_lock (&device_list_lock); +/* make sure ops on existing fds can abort cleanly */ +spin_lock_irq (&spidev->spi_lock); + spidev->spi =NULL ; +spin_unlock_irq (&spidev->spi_lock); +/* prevent new opens */ +mutex_lock (&device_list_lock);list_del (&spidev->device_entry);dev_set_drvdata (&spi->dev,NULL );clear_bit (MINOR (spidev->dev.devt), minors);device_unregister (&spidev->dev); -mutex_unlock (&device_list_lock);return 0 ;
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
A message that is not in the archive (sent off-list or not yet received): 200805211829.07189.david-b-yBeKhBN/[email protected]
Quoted 11 lines · On Wed, 21 May 2008 18:29:07 -0700 David Brownell…
Somehow the spidev code forgot to include a critical mechanism:
when the underlying device is removed (e.g.spi_masterrmmod), open file descriptors must be prevented from issuing new I/O requests to that device. On penalty of the oopsing reported by Sebastian Siewior <[email protected]> ...This is a partial fix, adding handshaking between the lower level (SPI messaging) and the file operations using the
spi_dev. There's still a refcounting issue to be addressed (separately) with respect to thespidev_dataitself.
For 2.6.26, I assume?
drivers/spi/spidev.c MODULE_PARM_DESC(bufsiz, "data bytes in · line 135
Quoted 4 lines
drivers/spi/spidev.c +4
-0+static inline int +spidev_sync_write(struct spidev_data *spidev, size_t len) +static inline int +spidev_sync_read(struct spidev_data *spidev, size_t len)
These strictly should return a ssize_t.
The inlining is wrong+pointless, but I appear to be losing that struggle.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
A message that is not in the archive (sent off-list or not yet received): 20080521205327.5aa27048.akpm-de/[email protected]
Quoted 8 lines · Andrew Morton
Quoted 6 lines · On Wed, 21 May 2008 18:29:07 -0700 David Brownell…
Somehow the spidev code forgot to include a critical mechanism:
when the underlying device is removed (e.g.spi_masterrmmod), open file descriptors must be prevented from issuing new I/O requests to that device. On penalty of the oopsing reported by Sebastian Siewior <[email protected]> ...
Quoted 7 lines
Quoted 5 lines
This is a partial fix, adding handshaking between the lower level (SPI messaging) and the file operations using the
spi_dev. There's still a refcounting issue to be addressed (separately) with respect to thespidev_dataitself.For 2.6.26, I assume?
Yes please.
Quoted 6 lines
drivers/spi/spidev.c MODULE_PARM_DESC(bufsiz, "data bytes in · line 135
Quoted 4 lines
drivers/spi/spidev.c +4
-0+static inline int +spidev_sync_write(struct spidev_data *spidev, size_t len) +static inline int +spidev_sync_read(struct spidev_data *spidev, size_t len)These strictly should return a
ssize_t.
True, but this was cut/paste from spi_write()/spi_read() which have that same issue. Should you feel compelled to fix that, go ahead ... else I (or someone) will get to it "someday".
Quoted 1 line
The inlining is wrong+pointless, but I appear to be losing that struggle.
I wish it actually *were* pointless, but the last several times I've had occasion to look at what current GCCs do I've observed that it's doing the Stupid Thing.
The short version of the story is that GCC inlining heuristics are still goofed. I've seen a few cases where GCC 4.3 does it better, some where it has significant regressions ... it depends partly on what processor you're using. It's vaguely possible it's OK for x86, but it's clearly more miss than hit on several other processors.
- Dave
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/