* [patch 2.6.26-rc3] spi: remove some spidev oops-on-rmmod paths
@ 2008-05-22 1:29 David Brownell
[not found] ` <200805211829.07189.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: David Brownell @ 2008-05-22 1:29 UTC (permalink / raw)
To: Andrew Morton
Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Sebastian Siewior
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 <bigeasy-kttTfShzVuc@public.gmane.org> ...
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.
Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
drivers/spi/spidev.c | 82 ++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 72 insertions(+), 10 deletions(-)
--- g26.orig/drivers/spi/spidev.c 2008-05-02 03:46:01.000000000 -0700
+++ g26/drivers/spi/spidev.c 2008-05-02 10:57:05.000000000 -0700
@@ -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/
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch 2.6.26-rc3] spi: remove some spidev oops-on-rmmod paths
[not found] ` <200805211829.07189.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
@ 2008-05-22 3:53 ` Andrew Morton
[not found] ` <20080521205327.5aa27048.akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Morton @ 2008-05-22 3:53 UTC (permalink / raw)
To: David Brownell
Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Sebastian Siewior
On Wed, 21 May 2008 18:29:07 -0700 David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> 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 <bigeasy-kttTfShzVuc@public.gmane.org> ...
>
> 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?
> +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/
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch 2.6.26-rc3] spi: remove some spidev oops-on-rmmod paths
[not found] ` <20080521205327.5aa27048.akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
@ 2008-05-22 4:36 ` David Brownell
0 siblings, 0 replies; 4+ messages in thread
From: David Brownell @ 2008-05-22 4:36 UTC (permalink / raw)
To: Andrew Morton
Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Sebastian Siewior
On Wednesday 21 May 2008, Andrew Morton wrote:
> On Wed, 21 May 2008 18:29:07 -0700 David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
>
> > 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 <bigeasy-kttTfShzVuc@public.gmane.org> ...
> >
> > 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.
> > +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".
> 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/
^ permalink raw reply [flat|nested] 4+ messages in thread
* [patch 2.6.26-rc3] spi: remove some spidev oops-on-rmmod paths
@ 2008-05-22 19:33 David Brownell
0 siblings, 0 replies; 4+ messages in thread
From: David Brownell @ 2008-05-22 19:33 UTC (permalink / raw)
To: Andrew Morton
Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Sebastian Siewior
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 <bigeasy-kttTfShzVuc@public.gmane.org> ...
This is a partial fix, adding handshaking between the lower
level (SPI messaging) and the file operations using the spi_dev.
(It also fixes an issue where reads and writes didn't return
the number of bytes sent or received.)
There's still a refcounting issue to be addressed (separately).
Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
UPDATED: use ssize_t, return number of bytes read/written, and
don't accidentally return positive errno.
drivers/spi/spidev.c | 102 ++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 89 insertions(+), 13 deletions(-)
--- a/drivers/spi/spidev.c 2008-05-20 21:24:51.000000000 -0700
+++ b/drivers/spi/spidev.c 2008-05-22 12:18:56.000000000 -0700
@@ -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,75 @@ 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 ssize_t
+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;
+ if (status == 0)
+ status = message->actual_length;
+ }
+ return status;
+}
+
+static inline ssize_t
+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 ssize_t
+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 +162,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 +185,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 +193,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 +214,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 +275,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 +329,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 +424,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 +455,7 @@ spidev_ioctl(struct inode *inode, struct
kfree(ioc);
break;
}
+ spi_dev_put(spi);
return retval;
}
@@ -488,6 +559,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 +598,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/
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-05-22 19:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-05-22 1:29 [patch 2.6.26-rc3] spi: remove some spidev oops-on-rmmod paths David Brownell
[not found] ` <200805211829.07189.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-05-22 3:53 ` Andrew Morton
[not found] ` <20080521205327.5aa27048.akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
2008-05-22 4:36 ` David Brownell
2008-05-22 19:33 David Brownell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).