[RFC / PATCH] [SPI] get_module while using it.

[RFC / PATCH] [SPI] get_module while using it.

[Sebastian Siewior]

Without this it is possible to unload the SPI-driver while an
spidev user open()ed the spidev device.

Signed-off-by: Sebastian Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
 drivers/spi/atmel_spi.c       |    1 +
 drivers/spi/mpc52xx_psc_spi.c |    1 +
 drivers/spi/omap2_mcspi.c     |    1 +
 drivers/spi/pxa2xx_spi.c      |    1 +
 drivers/spi/spi_bfin5xx.c     |    1 +
 drivers/spi/spi_bitbang.c     |    2 ++
 drivers/spi/spi_imx.c         |    1 +
 drivers/spi/spi_txx9.c        |    1 +
 drivers/spi/spidev.c          |    3 ++-
 include/linux/spi/spi.h       |    1 +
 10 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/atmel_spi.c b/drivers/spi/atmel_spi.c
index 1749a27..e5dd509 100644
--- a/drivers/spi/atmel_spi.c
+++ b/drivers/spi/atmel_spi.c
@@ -756,6 +756,7 @@ static int __init atmel_spi_probe(struct platform_device *pdev)
 	dev_info(&pdev->dev, "Atmel SPI Controller at 0x%08lx (irq %d)\n",
 			(unsigned long)regs->start, irq);
 
+	master->owner = THIS_MODULE;
 	ret = spi_register_master(master);
 	if (ret)
 		goto out_reset_hw;
diff --git a/drivers/spi/mpc52xx_psc_spi.c b/drivers/spi/mpc52xx_psc_spi.c
index 9072946..c4fe30e 100644
--- a/drivers/spi/mpc52xx_psc_spi.c
+++ b/drivers/spi/mpc52xx_psc_spi.c
@@ -446,6 +446,7 @@ static int __init mpc52xx_psc_spi_do_probe(struct device *dev, u32 regaddr,
 		goto free_irq;
 	}
 
+	master->owner = THIS_MODULE;
 	ret = spi_register_master(master);
 	if (ret < 0)
 		goto unreg_master;
diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
index b1cc148..0de7a59 100644
--- a/drivers/spi/omap2_mcspi.c
+++ b/drivers/spi/omap2_mcspi.c
@@ -1042,6 +1042,7 @@ static int __init omap2_mcspi_probe(struct platform_device *pdev)
 	if (omap2_mcspi_reset(mcspi) < 0)
 		goto err4;
 
+	master->owner = THIS_MODULE;
 	status = spi_register_master(master);
 	if (status < 0)
 		goto err4;
diff --git a/drivers/spi/pxa2xx_spi.c b/drivers/spi/pxa2xx_spi.c
index 147e26a..d59443a 100644
--- a/drivers/spi/pxa2xx_spi.c
+++ b/drivers/spi/pxa2xx_spi.c
@@ -1452,6 +1452,7 @@ static int __init pxa2xx_spi_probe(struct platform_device *pdev)
 
 	/* Register with the SPI framework */
 	platform_set_drvdata(pdev, drv_data);
+	master->owner = THIS_MODULE;
 	status = spi_register_master(master);
 	if (status != 0) {
 		dev_err(&pdev->dev, "problem registering spi master\n");
diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index a9ac1fd..019e001 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -1302,6 +1302,7 @@ static int __init bfin5xx_spi_probe(struct platform_device *pdev)
 
 	/* Register with the SPI framework */
 	platform_set_drvdata(pdev, drv_data);
+	master->owner = THIS_MODULE;
 	status = spi_register_master(master);
 	if (status != 0) {
 		dev_err(dev, "problem registering spi master\n");
diff --git a/drivers/spi/spi_bitbang.c b/drivers/spi/spi_bitbang.c
index 71e8814..e589428 100644
--- a/drivers/spi/spi_bitbang.c
+++ b/drivers/spi/spi_bitbang.c
@@ -481,6 +481,8 @@ int spi_bitbang_start(struct spi_bitbang *bitbang)
 		goto err1;
 	}
 
+	bitbang->master->owner = THIS_MODULE;
+
 	/* driver may get busy before register() returns, especially
 	 * if someone registered boardinfo for devices
 	 */
diff --git a/drivers/spi/spi_imx.c b/drivers/spi/spi_imx.c
index d4ba640..9a2f74d 100644
--- a/drivers/spi/spi_imx.c
+++ b/drivers/spi/spi_imx.c
@@ -1588,6 +1588,7 @@ static int __init spi_imx_probe(struct platform_device *pdev)
 
 	/* Register with the SPI framework */
 	platform_set_drvdata(pdev, drv_data);
+	master->owner = THIS_MODULE;
 	status = spi_register_master(master);
 	if (status != 0) {
 		dev_err(&pdev->dev, "probe - problem registering spi master\n");
diff --git a/drivers/spi/spi_txx9.c b/drivers/spi/spi_txx9.c
index 2296f37..9619a10 100644
--- a/drivers/spi/spi_txx9.c
+++ b/drivers/spi/spi_txx9.c
@@ -413,6 +413,7 @@ static int __init txx9spi_probe(struct platform_device *dev)
 		 (unsigned long long)res->start, irq,
 		 (c->baseclk + 500000) / 1000000);
 
+	master->owner = THIS_MODULE;
 	master->bus_num = dev->id;
 	master->setup = txx9spi_setup;
 	master->transfer = txx9spi_transfer;
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index a9bbf12..622f907 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -401,7 +401,7 @@ static int spidev_open(struct inode *inode, struct file *filp)
 			break;
 		}
 	}
-	if (status == 0) {
+	if (status == 0 && try_module_get(spidev->spi->master->owner)) {
 		if (!spidev->buffer) {
 			spidev->buffer = kmalloc(bufsiz, GFP_KERNEL);
 			if (!spidev->buffer) {
@@ -434,6 +434,7 @@ static int spidev_release(struct inode *inode, struct file *filp)
 		kfree(spidev->buffer);
 		spidev->buffer = NULL;
 	}
+	module_put(spidev->spi->master->owner);
 	mutex_unlock(&device_list_lock);
 
 	return status;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 920e837..0871b06 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -224,6 +224,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
  */
 struct spi_master {
 	struct device	dev;
+	struct module *owner;
 
 	/* other than negative (== assign one dynamically), bus_num is fully
 	 * board-specific.  usually that simplifies to being SOC-specific.
-- 
1.5.4.3


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [RFC / PATCH] [SPI] get_module while using it.

[David Brownell]

On Monday 14 April 2008, Sebastian Siewior wrote:
> Without this it is possible to unload the SPI-driver while an
> spidev user open()ed the spidev device.

Does that misbehave in any way?  That sort of unloading of
adapter code is possible with USB and I2C and hasn't been
perceived as much of a problem.  I'm not sure I see a strong
reason for SPI to act very different.

Also, the issue wouldn't be unique to "spidev"; any SPI device
will be removed along with its adapter.  Like USB and I2C...


> +       master->owner = THIS_MODULE;
>         ret = spi_register_master(master);

If this is really needed, what I most dislike is the way it
causes changes in all the adapter drivers.  So rather than
changing the API like that, how about switching its current
implementation a bit ... more like

static inline int spi_register_master(struct spi_master *master)
{
	return __spi_register_master(master, THIS_MODULE);
}

That's pretty much what i2c_add_driver() does.

- Dave



-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [RFC / PATCH] [SPI] get_module while using it.

[Sebastian Siewior]

* David Brownell | 2008-04-14 16:23:36 [-0700]:

>On Monday 14 April 2008, Sebastian Siewior wrote:
>> Without this it is possible to unload the SPI-driver while an
>> spidev user open()ed the spidev device.
>
>Does that misbehave in any way?  That sort of unloading of
Yes. Unload, read() -> Ooops.

>adapter code is possible with USB and I2C and hasn't been
>perceived as much of a problem.  I'm not sure I see a strong
>reason for SPI to act very different.
True. usb returns with -ENODEV after read/whatever. We could add
something like this to spi/spidev. This might also fit better into
hot-plugging.

>Also, the issue wouldn't be unique to "spidev"; any SPI device
>will be removed along with its adapter.  Like USB and I2C...
>
>
>> +       master->owner = THIS_MODULE;
>>         ret = spi_register_master(master);
>
>If this is really needed, what I most dislike is the way it
>causes changes in all the adapter drivers.  So rather than
>changing the API like that, how about switching its current
>implementation a bit ... more like
>
>static inline int spi_register_master(struct spi_master *master)
>{
>	return __spi_register_master(master, THIS_MODULE);
>}
>
>That's pretty much what i2c_add_driver() does.
This looks definitely better.

>- Dave
Sebatian

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [RFC / PATCH] [SPI] get_module while using it.

[David Brownell]

On Tuesday 15 April 2008, Sebastian Siewior wrote:
> * David Brownell | 2008-04-14 16:23:36 [-0700]:
> 
> >On Monday 14 April 2008, Sebastian Siewior wrote:
> >> Without this it is possible to unload the SPI-driver while an
> >> spidev user open()ed the spidev device.
> >
> >Does that misbehave in any way?  That sort of unloading of
>
> Yes. Unload, read() -> Ooops.

What's the oops, and with which controller driver?

If the adapter is unloaded it's *supposed* to first complete
all pending transfers (and prevent submission of new ones).

And removing the subsidiary devices is supposed to unbind
them ... though looking at it quickly, I suspect that the
"spidev" driver doesn't check for unbound devices in its
various I/O paths.

So this sounds like a bug in the controller driver, or else
in how spidev handles unbind.

- Dave


 
> >adapter code is possible with USB and I2C and hasn't been
> >perceived as much of a problem.  I'm not sure I see a strong
> >reason for SPI to act very different.
>
> True. usb returns with -ENODEV after read/whatever. We could add
> something like this to spi/spidev. This might also fit better into
> hot-plugging.

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [RFC / PATCH] [SPI] get_module while using it.

[Sebastian Siewior]

* David Brownell | 2008-04-15 11:09:43 [-0700]:

>On Tuesday 15 April 2008, Sebastian Siewior wrote:
>> * David Brownell | 2008-04-14 16:23:36 [-0700]:
>> 
>> >On Monday 14 April 2008, Sebastian Siewior wrote:
>> >> Without this it is possible to unload the SPI-driver while an
>> >> spidev user open()ed the spidev device.
>> >
>> >Does that misbehave in any way?  That sort of unloading of
>>
>> Yes. Unload, read() -> Ooops.
>
>What's the oops, and with which controller driver?
I can't dump the Ooops right now. The conter driver is my own which is
based on mpc52xx_psc_spi.c.

>If the adapter is unloaded it's *supposed* to first complete
>all pending transfers (and prevent submission of new ones).
Yes, but the problem here is (I thing) that spidev doesn't know that the
driver is gone. spi_async() (what is called straight from spidev_read()
with no checks in between) is always using spi->master->* even if the
master is gone.

>And removing the subsidiary devices is supposed to unbind
>them ... though looking at it quickly, I suspect that the
>"spidev" driver doesn't check for unbound devices in its
>various I/O paths.
Aha. So you mean that the read function which was spidev_read() while
the controller was available should point to something else once the
controller is gone?

>
>So this sounds like a bug in the controller driver, or else
>in how spidev handles unbind.
I try to look later at spidev once I find something that does a similar
job and works as expected....

>- Dave
Sebastian

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [RFC / PATCH] [SPI] get_module while using it.

[David Brownell]

On Thursday 24 April 2008, Sebastian Siewior wrote:
> >And removing the subsidiary devices is supposed to unbind
> >them ... though looking at it quickly, I suspect that the
> >"spidev" driver doesn't check for unbound devices in its
> >various I/O paths.
> 
> Aha. So you mean that the read function which was spidev_read() while
> the controller was available should point to something else once the
> controller is gone?

No, I mean that the issue is most likely a spidev bug.

> >So this sounds like a bug in the controller driver, or else
> >in how spidev handles unbind.
>
> I try to look later at spidev once I find something that does a similar
> job and works as expected....

They're all over the place, though not with SPI.  The unbind
method just updates some state that's checked in the I/O ops.

- Dave
 



-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [RFC / PATCH] [SPI] get_module while using it.

[David Brownell]

On Thursday 24 April 2008, David Brownell wrote:
> 
> > Aha. So you mean that the read function which was spidev_read() while
> > the controller was available should point to something else once the
> > controller is gone?
> 
> No, I mean that the issue is most likely a spidev bug.
> 

Does the appended patch resolve the problem you observed?

- Dave


============ CUT HERE
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> ...

RFC: NYET-Signed-off-by: author
---
 drivers/spi/spidev.c |   82 ++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 72 insertions(+), 10 deletions(-)

--- g26.orig/drivers/spi/spidev.c	2008-05-01 10:33:50.000000000 -0700
+++ g26/drivers/spi/spidev.c	2008-05-01 11:02:57.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;
 
@@ -488,6 +545,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 +584,17 @@ static int spidev_remove(struct spi_devi
 {
 	struct spidev_data	*spidev = dev_get_drvdata(&spi->dev);
 
-	mutex_lock(&device_list_lock);
+	/* first make sure ops on existing fds can abort cleanly */
+	spin_lock_irq(&spidev->spi_lock);
+	spidev->spi = NULL;
+	spin_unlock_irq(&spidev->spi_lock);
 
+	/* then 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 the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [RFC / PATCH] [SPI] get_module while using it.

[Sebastian Siewior]

* David Brownell | 2008-05-01 11:08:38 [-0700]:

>On Thursday 24 April 2008, David Brownell wrote:
>> 
>> > Aha. So you mean that the read function which was spidev_read() while
>> > the controller was available should point to something else once the
>> > controller is gone?
>> 
>> No, I mean that the issue is most likely a spidev bug.
>> 
>
>Does the appended patch resolve the problem you observed?
Nope. The backtrace:

Sending SPI_IOC_[    4.294967] ------------[ cut here ]------------
[    4.294967] Badness at /home/bigeasy/git/linux-2.6-powerpc/kernel/mutex.c:134
[    4.294967] NIP: c025a0f8 LR: c025a0e0 CTR: 00000000
[    4.294967] REGS: df273d60 TRAP: 0700   Not tainted  (2.6.25)
[    4.294967] MSR: 00021000 <ME>  CR: 24000482  XER: 20000000
[    4.294967] TASK = df2c1840[2307] 'spidev_test' THREAD: df272000
[    4.294967] GPR00: 00000000 df273e10 df2c1840 00000001 00000057 00000057 df2491fc c0be2920 
[    4.294967] GPR08: c02e0010 c0390000 00000001 c0360000 00000000 10019ad4 28004422 00000000 
[    4.294967] GPR16: 100e99a8 100d0000 100b0000 100d0000 df2491c0 df06b3c4 100d7b48 df06b230 
[    4.294967] GPR24: de833140 00000001 00000000 80206b00 df2c1840 df273e58 00029000 df06b3c4 
[    4.294967] NIP [c025a0f8] __mutex_lock_slowpath+0x80/0x1f4
[    4.294967] LR [c025a0e0] __mutex_lock_slowpath+0x68/0x1f4
[    4.294967] Call Trace:
[    4.294967] [df273e10] [00000001] 0x1 (unreliable)
[    4.294967] [df273e50] [c01b5148] spidev_ioctl+0x4c8/0x6ec
[    4.294967] [df273ec0] [c00861c0] vfs_ioctl+0x88/0xa8
[    4.294967] [df273ee0] [c00865e0] do_vfs_ioctl+0x400/0x444
[    4.294967] [df273f10] [c0086664] sys_ioctl+0x40/0x70
[    4.294967] [df273f40] [c000ded8] ret_from_syscall+0x0/0x3c
[    4.294967] Instruction dump:
[    4.294967] 801f0024 7f80f800 38000000 901f0004 41be0024 4bee6481 2f830000 419e0018 
[    4.294967] 3d20c039 8009871c 2f800000 409e0008 <0fe00000> 3ba10008 7fe3fb78 7fa4eb78 
[    4.294967] Unable to handle kernel paging request for data at address 0x6b6b6b6b

I started my spidev user, removed the module and then I started to write
what results in an ioctl. spidev_ioctl+0x4c8/0x6ec is
drivers/spi/spidev.c:228 and that is the first mutex_lock() in
spidev_message(). However you are touching spidev->spi and this is gone
isn't it?

Regards
Sebastian

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [RFC / PATCH] [SPI] get_module while using it.

[David Brownell]

On Friday 02 May 2008, Sebastian Siewior wrote:

> >> No, I mean that the issue is most likely a spidev bug.
> >
> > Does the appended patch resolve the problem you observed?
> 
> Nope. The backtrace:

... shows *new and different* behavior -- right? 

 
> Sending SPI_IOC_[    4.294967] ------------[ cut here ]------------
> [    4.294967] Badness at /home/bigeasy/git/linux-2.6-powerpc/kernel/mutex.c:134
> 			...
> [    4.294967] Call Trace:
> [    4.294967] [df273e10] [00000001] 0x1 (unreliable)
> [    4.294967] [df273e50] [c01b5148] spidev_ioctl+0x4c8/0x6ec
> [    4.294967] [df273ec0] [c00861c0] vfs_ioctl+0x88/0xa8
> [    4.294967] [df273ee0] [c00865e0] do_vfs_ioctl+0x400/0x444
> [    4.294967] [df273f10] [c0086664] sys_ioctl+0x40/0x70
> [    4.294967] [df273f40] [c000ded8] ret_from_syscall+0x0/0x3c
> 			... 
> [    4.294967] Unable to handle kernel paging request for data at address 0x6b6b6b6b
> 
> I started my spidev user, removed the module and then I started to write
> what results in an ioctl. spidev_ioctl+0x4c8/0x6ec is
> drivers/spi/spidev.c:228 and that is the first mutex_lock() in
> spidev_message().

Hmm, now it's referencing freed memory:  0x6b6b6b6b means it
used a pointer and got memory filled with POISON_FREE.  With
the particular memory dedicated to managing the spidev state.

That memory is freed only by spidev_classdev_release(), so
it looks like this particular issue is a refcounting bug.
I'll look at it later (unless you make time for it first).


> However you are touching spidev->spi and this is gone isn't it?

The appended update should catch a few spidev_ioctl() references
which the initial patch overlooked ... it goes on top of the patch
I sent first time.

But such a reference *COULD NOT* cause references to a mutex
in memory which has been deleted.

- Dave


--- g26.orig/drivers/spi/spidev.c	2008-05-02 10:16:04.000000000 -0700
+++ g26/drivers/spi/spidev.c	2008-05-02 10:14:39.000000000 -0700
@@ -326,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 */
@@ -413,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) {
@@ -442,6 +452,7 @@ spidev_ioctl(struct inode *inode, struct
 		kfree(ioc);
 		break;
 	}
+	spi_dev_put(spi);
 	return retval;
 }
 


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [RFC / PATCH] [SPI] get_module while using it.

[Sebastian Siewior]

* David Brownell | 2008-05-02 10:55:11 [-0700]:

>On Friday 02 May 2008, Sebastian Siewior wrote:
>
>> >> No, I mean that the issue is most likely a spidev bug.
>> >
>> > Does the appended patch resolve the problem you observed?
>> 
>> Nope. The backtrace:
>
>... shows *new and different* behavior -- right? 
>
> 
>> Sending SPI_IOC_[    4.294967] ------------[ cut here ]------------
>> [    4.294967] Badness at /home/bigeasy/git/linux-2.6-powerpc/kernel/mutex.c:134
>> 			...
>> [    4.294967] Call Trace:
>> [    4.294967] [df273e10] [00000001] 0x1 (unreliable)
>> [    4.294967] [df273e50] [c01b5148] spidev_ioctl+0x4c8/0x6ec
>> [    4.294967] [df273ec0] [c00861c0] vfs_ioctl+0x88/0xa8
>> [    4.294967] [df273ee0] [c00865e0] do_vfs_ioctl+0x400/0x444
>> [    4.294967] [df273f10] [c0086664] sys_ioctl+0x40/0x70
>> [    4.294967] [df273f40] [c000ded8] ret_from_syscall+0x0/0x3c
>> 			... 
>> [    4.294967] Unable to handle kernel paging request for data at address 0x6b6b6b6b
>> 
>> I started my spidev user, removed the module and then I started to write
>> what results in an ioctl. spidev_ioctl+0x4c8/0x6ec is
>> drivers/spi/spidev.c:228 and that is the first mutex_lock() in
>> spidev_message().
>
>Hmm, now it's referencing freed memory:  0x6b6b6b6b means it
>used a pointer and got memory filled with POISON_FREE.  With
>the particular memory dedicated to managing the spidev state.
Right. That mutex should still be available since only the
spidev->spi is gone.

>
>That memory is freed only by spidev_classdev_release(), so
>it looks like this particular issue is a refcounting bug.
>I'll look at it later (unless you make time for it first).
I try what I can do tomorrow but I probably can't before sunday.

>> However you are touching spidev->spi and this is gone isn't it?
>
>The appended update should catch a few spidev_ioctl() references
>which the initial patch overlooked ... it goes on top of the patch
>I sent first time.
>
>But such a reference *COULD NOT* cause references to a mutex
>in memory which has been deleted.
Right.

>
>- Dave
>
>
>--- g26.orig/drivers/spi/spidev.c	2008-05-02 10:16:04.000000000 -0700
>+++ g26/drivers/spi/spidev.c	2008-05-02 10:14:39.000000000 -0700
>@@ -326,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;
-ESHUTDOWN 

> 
> 	switch (cmd) {
> 	/* read requests */
>@@ -413,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) {
>@@ -442,6 +452,7 @@ spidev_ioctl(struct inode *inode, struct
> 		kfree(ioc);
> 		break;
> 	}
>+	spi_dev_put(spi);
> 	return retval;
> }
> 

Sebastian

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [RFC / PATCH] [SPI] get_module while using it.

[David Brownell]

On Friday 02 May 2008, Sebastian Siewior wrote:
> >That memory is freed only by spidev_classdev_release(), so
> >it looks like this particular issue is a refcounting bug.
> >I'll look at it later (unless you make time for it first).
>
> I try what I can do tomorrow but I probably can't before sunday.

Looks to me like the refcounting on spidev->users is insufficient.
That covers the buffer.  Separately, each open and each release
should probably update the refcount on spidev->dev itself.

- dave


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [RFC / PATCH] [SPI] get_module while using it.

[Sebastian Siewior]

* David Brownell | 2008-05-02 12:58:06 [-0700]:

>On Friday 02 May 2008, Sebastian Siewior wrote:
>> >That memory is freed only by spidev_classdev_release(), so
>> >it looks like this particular issue is a refcounting bug.
>> >I'll look at it later (unless you make time for it first).
>>
>> I try what I can do tomorrow but I probably can't before sunday.
>
>Looks to me like the refcounting on spidev->users is insufficient.
>That covers the buffer.  Separately, each open and each release
>should probably update the refcount on spidev->dev itself.

Yep, spidev->users protects only spidev->buffer while the spidev struct
is unprotected. Here an example: I remove my spi device driver (the
pointer is the address of my spidev struct):

| Removing: df159460
| ------------[ cut here ]------------
| Badness at /home/bigeasy/git/linux-2.6-powerpc/drivers/spi/spidev.c:617
| NIP: c01b4518 LR: c01b4518 CTR: 00000000
| REGS: df1f9c40 TRAP: 0700   Not tainted  (2.6.25)
| MSR: 00029000 <EE,ME>  CR: 28000488  XER: 20000000
| TASK = df894000[2155] 'rmmod' THREAD: df1f8000
| GPR00: c01b4518 df1f9cf0 df894000 00000025 bbbbbbbb 00000004 df395bcc c0be52a0 
| GPR08: 00000000 00000000 00000040 df1f8000 00000000 1001a2a8 28004422 00000000 
| GPR16: 100e9fa8 100d0000 100b0000 100d0000 00000000 100b0000 10013008 00000000 
| GPR24: bf8082f0 00000000 e1065158 df159690 df159460 df159690 c037bc54 df159690 
| NIP [c01b4518] spidev_remove+0x28/0xcc
| LR [c01b4518] spidev_remove+0x28/0xcc
| Call Trace:
| [df1f9cf0] [c01b4518] spidev_remove+0x28/0xcc (unreliable)
| [df1f9d10] [c01b433c] spi_drv_remove+0x2c/0x3c
| [df1f9d20] [c0185f08] __device_release_driver+0x88/0xb4
| [df1f9d30] [c0186024] device_release_driver+0x28/0x44
| [df1f9d50] [c0185208] bus_remove_device+0x8c/0xb8
| [df1f9d70] [c018389c] device_del+0xf8/0x16c
| [df1f9d90] [c0183928] device_unregister+0x18/0x30
| [df1f9db0] [c01b3d38] __unregister+0x20/0x34
| [df1f9dc0] [c01830dc] device_for_each_child+0x30/0x74
| [df1f9df0] [c01b3cfc] spi_unregister_master+0x28/0x44
| [df1f9e10] [e10641ac] fpga_spi_of_remove+0x48/0xb0 [fpga_spi]
| [df1f9e30] [c01e27fc] of_platform_device_remove+0x30/0x44
| [df1f9e40] [c0185f08] __device_release_driver+0x88/0xb4
| [df1f9e50] [c0185fd4] driver_detach+0xa0/0xc8
| [df1f9e70] [c018509c] bus_remove_driver+0x98/0xd4
| [df1f9e90] [c01864f8] driver_unregister+0x48/0x5c
| [df1f9eb0] [c01e2908] of_unregister_driver+0x14/0x24
| [df1f9ec0] [e1064c04] fpga_spi_exit+0x18/0x28 [fpga_spi]
| [df1f9ed0] [c004c38c] sys_delete_module+0x1b4/0x204
| [df1f9f40] [c000ded8] ret_from_syscall+0x0/0x3c
| Instruction dump:
| 7c0803a6 4e800020 9421ffe0 7c0802a6 bf410008 7c7b1b78 90010024 8383010c 
| 3c60c031 3863a4d8 7f84e378 4be720fd <0fe00000> 3bbc0178 7fa3eb78 3b400000 

Now, I use my userspace program to do an ioctl:

| Touching: df159460
| BUG: spinlock bad magic on CPU#0, spidev_test/2153
| Unable to handle kernel paging request for data at address 0x6b6b6d5f
| Faulting instruction address: 0xc0142d74
| PREEMPT 
| Modules linked in: bluetooth libertas [last unloaded: fg
| REGS: df2efd50 TRAP: 0300   Not tainted  (2.6.25)
| MSR: 00021000 <ME>  CR: 22000424  XER: 20000000
| TASK = df895840[2153] 'spidev_test' THREAD: df2ee000
| GPR00: c0142d5c df2efe00 df895840 00000045 00000001 6b6b6b6b c02e64e4 ffffffff 
| GPR08: 00000000 c02e0000 00005634 df2ee000 ffffe3c0 10019ad4 28004422 00000000 
| GPR16: 100e9ac8 100d0000 100b0000 100d0000 00000000 100b0000 100e9948 100e9be8 
| GPR24: 00000000 100e0008 df159460 80206b00 00000004 c02fa3f8 df1595d8 6b6b6b6b 
| NIP [c0142d74] spin_bug+0x6c/0xd0
| LR [c0142d5c] spin_bug+0x54/0xd0
| Call Trace:
| [df2efe00] [c0142d5c] spin_bug+0x54/0xd0 (unreliable)
| [df2efe20] [c0142f18] _raw_spin_lock+0x34/0x118
| [df2efe40] [c025e50c] _spin_lock_irq+0x24/0x34
| [df2efe50] [c01b4d50] spidev_ioctl+0xac/0x748
| [df2efec0] [c00861c0] vfs_ioctl+0x88/0xa8
| [df2efee0] [c00865e0] do_vfs_ioctl+0x400/0x444
| [df2eff10] [c0086664] sys_ioctl+0x40/0x70
| [df2eff40] [c000ded8] ret_from_syscall+0x0/0x3c
| Instruction dump:
| 386aa41c 38c20304 38a00000 419d0048 80e201f4 4bee38b9 2f9f0000 3d20c02e 
| 80be0004 38c964e4 38e0ffff 419e000c <80ff01f4> 38df0304 811e0008 3c60c030 
| ---[ end trace 4ccbf52b28cfbf39 ]---

"Touching" is just before the spinlock.
So, spidev is all gone while userspace has still an open handle. Now,
what do recommend? Solving this by refcounting in spidev_remove() or
spi_drv_remove()?

>
>- dave
Sebastian

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [RFC / PATCH] [SPI] get_module while using it.

[David Brownell]

On Thursday 08 May 2008, Sebastian Siewior wrote:
> * David Brownell | 2008-05-02 12:58:06 [-0700]:
> 
> >On Friday 02 May 2008, Sebastian Siewior wrote:
> >> >That memory is freed only by spidev_classdev_release(), so
> >> >it looks like this particular issue is a refcounting bug.
> >> >I'll look at it later (unless you make time for it first).
> >>
> >> I try what I can do tomorrow but I probably can't before sunday.
> >
> >Looks to me like the refcounting on spidev->users is insufficient.
> >That covers the buffer.  Separately, each open and each release
> >should probably update the refcount on spidev->dev itself.
> 
> Yep, spidev->users protects only spidev->buffer while the spidev struct
> is unprotected. Here an example: I remove my spi device driver (the
> pointer is the address of my spidev struct):
> 
> | Removing: df159460
> | ------------[ cut here ]------------
> | Badness at /home/bigeasy/git/linux-2.6-powerpc/drivers/spi/spidev.c:617
> | NIP: c01b4518 LR: c01b4518 CTR: 00000000
> | REGS: df1f9c40 TRAP: 0700   Not tainted  (2.6.25)
> | MSR: 00029000 <EE,ME>  CR: 28000488  XER: 20000000
> | TASK = df894000[2155] 'rmmod' THREAD: df1f8000
> | GPR00: c01b4518 df1f9cf0 df894000 00000025 bbbbbbbb 00000004 df395bcc c0be52a0 
> | GPR08: 00000000 00000000 00000040 df1f8000 00000000 1001a2a8 28004422 00000000 
> | GPR16: 100e9fa8 100d0000 100b0000 100d0000 00000000 100b0000 10013008 00000000 
> | GPR24: bf8082f0 00000000 e1065158 df159690 df159460 df159690 c037bc54 df159690 
> | NIP [c01b4518] spidev_remove+0x28/0xcc
> | LR [c01b4518] spidev_remove+0x28/0xcc
> | Call Trace:
> | [df1f9cf0] [c01b4518] spidev_remove+0x28/0xcc (unreliable)
> | [df1f9d10] [c01b433c] spi_drv_remove+0x2c/0x3c
> | [df1f9d20] [c0185f08] __device_release_driver+0x88/0xb4
> | [df1f9d30] [c0186024] device_release_driver+0x28/0x44
> | [df1f9d50] [c0185208] bus_remove_device+0x8c/0xb8
> | [df1f9d70] [c018389c] device_del+0xf8/0x16c
> | [df1f9d90] [c0183928] device_unregister+0x18/0x30
> | [df1f9db0] [c01b3d38] __unregister+0x20/0x34
> | [df1f9dc0] [c01830dc] device_for_each_child+0x30/0x74
> | [df1f9df0] [c01b3cfc] spi_unregister_master+0x28/0x44
> | [df1f9e10] [e10641ac] fpga_spi_of_remove+0x48/0xb0 [fpga_spi]
> | [df1f9e30] [c01e27fc] of_platform_device_remove+0x30/0x44
> | [df1f9e40] [c0185f08] __device_release_driver+0x88/0xb4
> | [df1f9e50] [c0185fd4] driver_detach+0xa0/0xc8
> | [df1f9e70] [c018509c] bus_remove_driver+0x98/0xd4
> | [df1f9e90] [c01864f8] driver_unregister+0x48/0x5c
> | [df1f9eb0] [c01e2908] of_unregister_driver+0x14/0x24
> | [df1f9ec0] [e1064c04] fpga_spi_exit+0x18/0x28 [fpga_spi]
> | [df1f9ed0] [c004c38c] sys_delete_module+0x1b4/0x204
> | [df1f9f40] [c000ded8] ret_from_syscall+0x0/0x3c
> | Instruction dump:
> | 7c0803a6 4e800020 9421ffe0 7c0802a6 bf410008 7c7b1b78 90010024 8383010c 
> | 3c60c031 3863a4d8 7f84e378 4be720fd <0fe00000> 3bbc0178 7fa3eb78 3b400000 
> 
> Now, I use my userspace program to do an ioctl:
> 
> | Touching: df159460
> | BUG: spinlock bad magic on CPU#0, spidev_test/2153
> | Unable to handle kernel paging request for data at address 0x6b6b6d5f
> | Faulting instruction address: 0xc0142d74
> | PREEMPT 
> | Modules linked in: bluetooth libertas [last unloaded: fg
> | REGS: df2efd50 TRAP: 0300   Not tainted  (2.6.25)
> | MSR: 00021000 <ME>  CR: 22000424  XER: 20000000
> | TASK = df895840[2153] 'spidev_test' THREAD: df2ee000
> | GPR00: c0142d5c df2efe00 df895840 00000045 00000001 6b6b6b6b c02e64e4 ffffffff 
> | GPR08: 00000000 c02e0000 00005634 df2ee000 ffffe3c0 10019ad4 28004422 00000000 
> | GPR16: 100e9ac8 100d0000 100b0000 100d0000 00000000 100b0000 100e9948 100e9be8 
> | GPR24: 00000000 100e0008 df159460 80206b00 00000004 c02fa3f8 df1595d8 6b6b6b6b 
> | NIP [c0142d74] spin_bug+0x6c/0xd0
> | LR [c0142d5c] spin_bug+0x54/0xd0
> | Call Trace:
> | [df2efe00] [c0142d5c] spin_bug+0x54/0xd0 (unreliable)
> | [df2efe20] [c0142f18] _raw_spin_lock+0x34/0x118
> | [df2efe40] [c025e50c] _spin_lock_irq+0x24/0x34
> | [df2efe50] [c01b4d50] spidev_ioctl+0xac/0x748
> | [df2efec0] [c00861c0] vfs_ioctl+0x88/0xa8
> | [df2efee0] [c00865e0] do_vfs_ioctl+0x400/0x444
> | [df2eff10] [c0086664] sys_ioctl+0x40/0x70
> | [df2eff40] [c000ded8] ret_from_syscall+0x0/0x3c
> | Instruction dump:
> | 386aa41c 38c20304 38a00000 419d0048 80e201f4 4bee38b9 2f9f0000 3d20c02e 
> | 80be0004 38c964e4 38e0ffff 419e000c <80ff01f4> 38df0304 811e0008 3c60c030 
> | ---[ end trace 4ccbf52b28cfbf39 ]---
> 
> "Touching" is just before the spinlock.
> So, spidev is all gone while userspace has still an open handle. Now,
> what do recommend? Solving this by refcounting in spidev_remove() or
> spi_drv_remove()?

Here's a patch I've had sitting in my tree for a while,
untested ... the only convenient "spidev" testcase I
have doesn't actually let me do the "rmmod spi_master"
test you're doing.

It's probably way more than needed for this issue alone,
since it should be fixable without switching the classdev
creation around.

(Goes on top of the previous patch...)

- Dave


====================== CUT HERE
--- g26.orig/drivers/spi/spidev.c	2008-05-04 18:15:02.000000000 -0700
+++ g26/drivers/spi/spidev.c	2008-05-04 01:27:17.000000000 -0700
@@ -25,6 +25,7 @@
 #include <linux/ioctl.h>
 #include <linux/fs.h>
 #include <linux/device.h>
+#include <linux/err.h>
 #include <linux/list.h>
 #include <linux/errno.h>
 #include <linux/mutex.h>
@@ -67,11 +68,12 @@ static unsigned long	minors[N_SPI_MINORS
 				| SPI_LSB_FIRST | SPI_3WIRE | SPI_LOOP)
 
 struct spidev_data {
-	struct device		dev;
+	dev_t			devt;
 	spinlock_t		spi_lock;
 	struct spi_device	*spi;
 	struct list_head	device_entry;
 
+	/* buffer is NULL unless this device is open (users > 0) */
 	struct mutex		buf_lock;
 	unsigned		users;
 	u8			*buffer;
@@ -464,7 +466,7 @@ static int spidev_open(struct inode *ino
 	mutex_lock(&device_list_lock);
 
 	list_for_each_entry(spidev, &device_list, device_entry) {
-		if (spidev->dev.devt == inode->i_rdev) {
+		if (spidev->devt == inode->i_rdev) {
 			status = 0;
 			break;
 		}
@@ -497,10 +499,22 @@ static int spidev_release(struct inode *
 	mutex_lock(&device_list_lock);
 	spidev = filp->private_data;
 	filp->private_data = NULL;
+
+	/* last close? */
 	spidev->users--;
 	if (!spidev->users) {
+		int		dofree;
+
 		kfree(spidev->buffer);
 		spidev->buffer = NULL;
+
+		/* ... after underlying device went away? */
+		spin_lock_irq(&spidev->spi_lock);
+		dofree = (spidev->spi == NULL);
+		spin_unlock_irq(&spidev->spi_lock);
+
+		if (dofree)
+			kfree(spidev);
 	}
 	mutex_unlock(&device_list_lock);
 
@@ -527,19 +541,7 @@ static struct file_operations spidev_fop
  * It also simplifies memory management.
  */
 
-static void spidev_classdev_release(struct device *dev)
-{
-	struct spidev_data	*spidev;
-
-	spidev = container_of(dev, struct spidev_data, dev);
-	kfree(spidev);
-}
-
-static struct class spidev_class = {
-	.name		= "spidev",
-	.owner		= THIS_MODULE,
-	.dev_release	= spidev_classdev_release,
-};
+static struct class *spidev_class;
 
 /*-------------------------------------------------------------------------*/
 
@@ -567,20 +569,20 @@ static int spidev_probe(struct spi_devic
 	mutex_lock(&device_list_lock);
 	minor = find_first_zero_bit(minors, N_SPI_MINORS);
 	if (minor < N_SPI_MINORS) {
-		spidev->dev.parent = &spi->dev;
-		spidev->dev.class = &spidev_class;
-		spidev->dev.devt = MKDEV(SPIDEV_MAJOR, minor);
-		snprintf(spidev->dev.bus_id, sizeof spidev->dev.bus_id,
+		struct device *dev;
+
+		spidev->devt = MKDEV(SPIDEV_MAJOR, minor);
+		dev = device_create(spidev_class, &spi->dev, spidev->devt,
 				"spidev%d.%d",
 				spi->master->bus_num, spi->chip_select);
-		status = device_register(&spidev->dev);
+		status = IS_ERR(dev) ? PTR_ERR(dev) : 0;
 	} else {
 		dev_dbg(&spi->dev, "no minor number available!\n");
 		status = -ENODEV;
 	}
 	if (status == 0) {
 		set_bit(minor, minors);
-		dev_set_drvdata(&spi->dev, spidev);
+		spi_set_drvdata(spi, spidev);
 		list_add(&spidev->device_entry, &device_list);
 	}
 	mutex_unlock(&device_list_lock);
@@ -593,19 +595,21 @@ static int spidev_probe(struct spi_devic
 
 static int spidev_remove(struct spi_device *spi)
 {
-	struct spidev_data	*spidev = dev_get_drvdata(&spi->dev);
+	struct spidev_data	*spidev = spi_get_drvdata(spi);
 
 	/* make sure ops on existing fds can abort cleanly */
 	spin_lock_irq(&spidev->spi_lock);
 	spidev->spi = NULL;
+	spi_set_drvdata(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);
+	device_destroy(spidev_class, spidev->devt);
+	clear_bit(MINOR(spidev->devt), minors);
+	if (spidev->users == 0)
+		kfree(spidev);
 	mutex_unlock(&device_list_lock);
 
 	return 0;
@@ -641,15 +645,15 @@ static int __init spidev_init(void)
 	if (status < 0)
 		return status;
 
-	status = class_register(&spidev_class);
-	if (status < 0) {
+	spidev_class = class_create(THIS_MODULE, "spidev");
+	if (IS_ERR(spidev_class)) {
 		unregister_chrdev(SPIDEV_MAJOR, spidev_spi.driver.name);
-		return status;
+		return PTR_ERR(spidev_class);
 	}
 
 	status = spi_register_driver(&spidev_spi);
 	if (status < 0) {
-		class_unregister(&spidev_class);
+		class_destroy(spidev_class);
 		unregister_chrdev(SPIDEV_MAJOR, spidev_spi.driver.name);
 	}
 	return status;
@@ -659,7 +663,7 @@ module_init(spidev_init);
 static void __exit spidev_exit(void)
 {
 	spi_unregister_driver(&spidev_spi);
-	class_unregister(&spidev_class);
+	class_destroy(spidev_class);
 	unregister_chrdev(SPIDEV_MAJOR, spidev_spi.driver.name);
 }
 module_exit(spidev_exit);

-------------------------------------------------------------------------
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/