[PATCH] spi/spi: don't release the spi device twice

[PATCH] spi/spi: don't release the spi device twice

[Sebastian Andrzej Siewior]

While rmmoding pxa2xx_spi I hit:

|BUG: unable to handle kernel paging request at 6b6b6b9b
|IP: [<c115bf61>] device_del+0x11/0x140
|Call Trace:
| [<c115c09b>] ? device_unregister+0xb/0x20
| [<c118fe33>] ? spi_unregister_master+0x93/0xc0
| [<f806c554>] ? pxa2xx_spi_remove+0x84/0xc0 [pxa2xx_spi]

According to my debug printks, the loop

| dummy = device_for_each_child(master->dev.parent, &master->dev,
| 		__unregister);

calls __unregister for all childs of spi devicee (spidev in my case) and
the spi device itself. So calling device_unregister() for the device
itself leads to trouble.
This seems to be comming from  3486008 aka ("spi: free children in
spi_unregister_master, not siblings") so therefore I cc stable for v36.

Cc: <stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> # .36.x
Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
This is on v2.6.37-rc1. Unless this got fixed somewhere else in the
meantime it is still there.

 drivers/spi/spi.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 709c836..3c8ff6f 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -586,7 +586,6 @@ void spi_unregister_master(struct spi_master *master)
 
 	dummy = device_for_each_child(master->dev.parent, &master->dev,
 					__unregister);
-	device_unregister(&master->dev);
 }
 EXPORT_SYMBOL_GPL(spi_unregister_master);
 
-- 
1.7.3.2


------------------------------------------------------------------------------
Beautiful is writing same markup. Internet Explorer 9 supports
standards for HTML5, CSS3, SVG 1.1,  ECMAScript5, and DOM L2 & L3.
Spend less time writing and  rewriting code and more time creating great
experiences on the web. Be a part of the beta today
http://p.sf.net/sfu/msIE9-sfdev2dev

Re: spi/spi: don't release the spi device twice

[David Lamparter]

On Mon, Nov 22, 2010 at 01:35:03PM -0000, Sebastian Andrzej Siewior wrote:
> According to my debug printks, the loop
> 
> | dummy = device_for_each_child(master->dev.parent, &master->dev,
> | 		__unregister);
> 
> calls __unregister for all childs of spi devicee (spidev in my case) and
> the spi device itself. So calling device_unregister() for the device
> itself leads to trouble.
> This seems to be comming from  3486008 aka ("spi: free children in
> spi_unregister_master, not siblings") so therefore I cc stable for v36.

This code is the old code, before patch 3486008 which you're citing.
3486008 does:

-       dummy = device_for_each_child(master->dev.parent, &master->dev,
-                                       __unregister);
+       dummy = device_for_each_child(&master->dev, NULL, __unregister);

Considering that this patch is in 2.6.36 (and 36.1), you seem to have
mixed up your sources. Please make sure your checkout is current and
unbroken...

> This is on v2.6.37-rc1. Unless this got fixed somewhere else in the
> meantime it is still there.

"Mu."

> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -586,7 +586,6 @@ void spi_unregister_master(struct spi_master *master)
>  
>  	dummy = device_for_each_child(master->dev.parent, &master->dev,
>  					__unregister);
> -	device_unregister(&master->dev);
>  }

This patch does, consequently, not apply on 2.6.37-rc, since the code
doesn't look like that anymore after 3486008...


-David


------------------------------------------------------------------------------
Increase Visibility of Your 3D Game App & Earn a Chance To Win $500!
Tap into the largest installed PC base & get more eyes on your game by
optimizing for Intel(R) Graphics Technology. Get started today with the
Intel(R) Software Partner Program. Five $500 cash prizes are up for grabs.
http://p.sf.net/sfu/intelisp-dev2dev

Re: spi/spi: don't release the spi device twice

[Sebastian Andrzej Siewior]

David Lamparter wrote:
> This code is the old code, before patch 3486008 which you're citing.
> 3486008 does:
> 
> -       dummy = device_for_each_child(master->dev.parent, &master->dev,
> -                                       __unregister);
> +       dummy = device_for_each_child(&master->dev, NULL, __unregister);
> 
> Considering that this patch is in 2.6.36 (and 36.1), you seem to have
> mixed up your sources. Please make sure your checkout is current and
> unbroken...
Hmmm.
# git describe --long
v2.6.37-rc3-0-g3561d43

After looking at spi_unregister_master() in drivers/spi/spi.c, I see:

      dummy = device_for_each_child(master->dev.parent, &master->dev,
                      __unregister);
      device_unregister(&master->dev);
  }

This change got back in by:

commit 2b9603a0d7e395fb844af90fba71448bc8019077
Author: Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date:   Mon Aug 2 15:52:15 2010 +0800

     spi: enable spi_board_info to be registered after spi_master

which is v2.6.37-rc1~2^2~4. So I probably mixed up you with Feng.

This thread starts at
http://www.mail-archive.com/spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org/msg05437.html

>David

Sebastian

------------------------------------------------------------------------------
Increase Visibility of Your 3D Game App & Earn a Chance To Win $500!
Tap into the largest installed PC base & get more eyes on your game by
optimizing for Intel(R) Graphics Technology. Get started today with the
Intel(R) Software Partner Program. Five $500 cash prizes are up for grabs.
http://p.sf.net/sfu/intelisp-dev2dev

Re: spi/spi: don't release the spi device twice

[Feng Tang]

Hi Sebastian,

On Tue, 23 Nov 2010 18:33:03 +0800
Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:

> David Lamparter wrote:
> > This code is the old code, before patch 3486008 which you're citing.
> > 3486008 does:
> > 
> > -       dummy = device_for_each_child(master->dev.parent,
> > &master->dev,
> > -                                       __unregister);
> > +       dummy = device_for_each_child(&master->dev, NULL,
> > __unregister);
> > 
> > Considering that this patch is in 2.6.36 (and 36.1), you seem to
> > have mixed up your sources. Please make sure your checkout is
> > current and unbroken...
> Hmmm.
> # git describe --long
> v2.6.37-rc3-0-g3561d43
> 
> After looking at spi_unregister_master() in drivers/spi/spi.c, I see:
> 
>       dummy = device_for_each_child(master->dev.parent, &master->dev,
>                       __unregister);
>       device_unregister(&master->dev);
>   }
> 
> This change got back in by:
> 
> commit 2b9603a0d7e395fb844af90fba71448bc8019077
> Author: Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date:   Mon Aug 2 15:52:15 2010 +0800
> 
>      spi: enable spi_board_info to be registered after spi_master
> 
> which is v2.6.37-rc1~2^2~4. So I probably mixed up you with Feng.
> 
> This thread starts at
> http://www.mail-archive.com/spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org/msg05437.html
> 
I checked my original patch which didn't touch the logic of spi_unregister_master() as
-----------------------------------------
@@ -568,6 +592,10 @@  void spi_unregister_master(struct spi_master *master)
 {
 	int dummy;
 
+	mutex_lock(&board_lock);
+	list_del(&master->list);
+	mutex_unlock(&board_lock);
+
 	dummy = device_for_each_child(master->dev.parent, &master->dev,
 					__unregister);
 	device_unregister(&master->dev);
-----------------------------------------

So this should be a merge problem, which corrupt the commit from David's commit
3486008 "spi: free children in spi_unregister_master, not siblings"

Thanks,
Feng

------------------------------------------------------------------------------
Increase Visibility of Your 3D Game App & Earn a Chance To Win $500!
Tap into the largest installed PC base & get more eyes on your game by
optimizing for Intel(R) Graphics Technology. Get started today with the
Intel(R) Software Partner Program. Five $500 cash prizes are up for grabs.
http://p.sf.net/sfu/intelisp-dev2dev

[PATCH v2] spi/spi: don't release the spi device twice

[Sebastian Andrzej Siewior]

This was fixed by David Lamparter in v2.6.36-rc5 3486008
("spi: free children in spi_unregister_master, not siblings") and broken
again in v2.6.37-rc1~2^2~4 during the merge of 2b9603a0 ("spi: enable
spi_board_info to be registered after spi_master").

Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
Okay, Feng. So here is the merge fixup.

 drivers/spi/spi.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 709c836..b02d0cb 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -584,8 +584,7 @@ void spi_unregister_master(struct spi_master *master)
 	list_del(&master->list);
 	mutex_unlock(&board_lock);
 
-	dummy = device_for_each_child(master->dev.parent, &master->dev,
-					__unregister);
+	dummy = device_for_each_child(&master->dev, NULL, __unregister);
 	device_unregister(&master->dev);
 }
 EXPORT_SYMBOL_GPL(spi_unregister_master);
-- 
1.7.3.2

------------------------------------------------------------------------------
Increase Visibility of Your 3D Game App & Earn a Chance To Win $500!
Tap into the largest installed PC base & get more eyes on your game by
optimizing for Intel(R) Graphics Technology. Get started today with the
Intel(R) Software Partner Program. Five $500 cash prizes are up for grabs.
http://p.sf.net/sfu/intelisp-dev2dev

Re: [PATCH v2] spi/spi: don't release the spi device twice

[David Lamparter]

> This was fixed by David Lamparter in v2.6.36-rc5 3486008
> ("spi: free children in spi_unregister_master, not siblings") and broken
> again in v2.6.37-rc1~2^2~4 during the merge of 2b9603a0 ("spi: enable
> spi_board_info to be registered after spi_master").
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> ---
> -	dummy = device_for_each_child(master->dev.parent, &master->dev,
> -					__unregister);
> +	dummy = device_for_each_child(&master->dev, NULL, __unregister);

Signed-off-by: David Lamparter <equinox-cEeocvCgqOOsTnJN9+BGXg@public.gmane.org>

very simple merge/rebase/forward-port breakage...
Feng, can you check 2b9603a0 for whether anything else got broken?


------------------------------------------------------------------------------
Increase Visibility of Your 3D Game App & Earn a Chance To Win $500!
Tap into the largest installed PC base & get more eyes on your game by
optimizing for Intel(R) Graphics Technology. Get started today with the
Intel(R) Software Partner Program. Five $500 cash prizes are up for grabs.
http://p.sf.net/sfu/intelisp-dev2dev

Re: [PATCH v2] spi/spi: don't release the spi device twice

[Feng Tang]

On Wed, 24 Nov 2010 00:45:20 +0800
David Lamparter <equinox-cEeocvCgqOOsTnJN9+BGXg@public.gmane.org> wrote:

> > This was fixed by David Lamparter in v2.6.36-rc5 3486008
> > ("spi: free children in spi_unregister_master, not siblings") and
> > broken again in v2.6.37-rc1~2^2~4 during the merge of 2b9603a0
> > ("spi: enable spi_board_info to be registered after spi_master").
> > 
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> > ---
> > -	dummy = device_for_each_child(master->dev.parent,
> > &master->dev,
> > -					__unregister);
> > +	dummy = device_for_each_child(&master->dev, NULL,
> > __unregister);
> 
> Signed-off-by: David Lamparter <equinox-cEeocvCgqOOsTnJN9+BGXg@public.gmane.org>
> 
> very simple merge/rebase/forward-port breakage...
> Feng, can you check 2b9603a0 for whether anything else got broken?
> 

I just checked, all other parts should be ok.

Thanks,
Feng

------------------------------------------------------------------------------
Increase Visibility of Your 3D Game App & Earn a Chance To Win $500!
Tap into the largest installed PC base & get more eyes on your game by
optimizing for Intel(R) Graphics Technology. Get started today with the
Intel(R) Software Partner Program. Five $500 cash prizes are up for grabs.
http://p.sf.net/sfu/intelisp-dev2dev

Re: [v2] spi/spi: don't release the spi device twice

[Sebastian Andrzej Siewior]

* Sebastian Andrzej Siewior | 2010-11-23 14:59:10 [-0000]:

>This was fixed by David Lamparter in v2.6.36-rc5 3486008
>("spi: free children in spi_unregister_master, not siblings") and broken
>again in v2.6.37-rc1~2^2~4 during the merge of 2b9603a0 ("spi: enable
>spi_board_info to be registered after spi_master").
>
>Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
>---

ping

>
> drivers/spi/spi.c |    3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>index 709c836..b02d0cb 100644
>--- a/drivers/spi/spi.c
>+++ b/drivers/spi/spi.c
>@@ -584,8 +584,7 @@ void spi_unregister_master(struct spi_master *master)
> 	list_del(&master->list);
> 	mutex_unlock(&board_lock);
> 
>-	dummy = device_for_each_child(master->dev.parent, &master->dev,
>-					__unregister);
>+	dummy = device_for_each_child(&master->dev, NULL, __unregister);
> 	device_unregister(&master->dev);
> }
> EXPORT_SYMBOL_GPL(spi_unregister_master);

------------------------------------------------------------------------------