Skip to contentSkip to conversation

spi: remove some spidev oops-on-rmmod paths

PATCH In review David Brownell 2 participants 3 messages May 22, 2008 2 messages not in the archive SPI

lore
Share & tools
lore
#0 lore raw

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_MINORS
 
 struct 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 __us
 		return -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 ch
 		return -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, struct
 	if (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, struct
 	default:
 		/* 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, struct
 		kfree(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]

#2 lore raw
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_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.

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]

#4 lore raw
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_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]> ...

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 the spidev_data itself.

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/