[PATCH/RFC] spi: Fix device unregistration when unregistering the bus master

[PATCH/RFC] spi: Fix device unregistration when unregistering the bus master

[Laurent Pinchart]

Device are added as children of the bus master's parent device, but
spi_unregister_master() looks for devices to unregister in the bus
master's children. This results in the child devices not being
unregistered.

Fix this by iterating over the bus master's parent device when
unregistering the master.

Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
 drivers/spi/spi.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

Hi,

I came across this issue when developing an SPI master driver for a platform
device. Board code registers SPI board information for a flash device, an SPI
device was created when I loaded my SPI master driver, but it stayed there when
the SPI master driver was removed.

SPI master unregistration has been patched a couple of times:

- 97dbf37d ("drivers/spi/spi.c: don't release the spi device twice")
- 3486008 ("spi: free children in spi_unregister_master, not siblings")
- 2b9603a0 ("spi: enable spi_board_info to be registered after spi_master")

I'm not too familiar with the way the SPI subsystem plugs its devices in the
Linux device model, so I'm not sure what the right fix is. An alternative would
be to register the SPI devices as children of the SPI master, not as siblings.

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 2e13a14..081e0a1 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -638,7 +638,8 @@ EXPORT_SYMBOL_GPL(spi_register_master);
 
 static int __unregister(struct device *dev, void *null)
 {
-	spi_unregister_device(to_spi_device(dev));
+	if (dev->bus == &spi_bus_type)
+		spi_unregister_device(to_spi_device(dev));
 	return 0;
 }
 
@@ -660,7 +661,7 @@ void spi_unregister_master(struct spi_master *master)
 	list_del(&master->list);
 	mutex_unlock(&board_lock);
 
-	dummy = device_for_each_child(&master->dev, NULL, __unregister);
+	dummy = device_for_each_child(master->dev.parent, NULL, __unregister);
 	device_unregister(&master->dev);
 }
 EXPORT_SYMBOL_GPL(spi_unregister_master);
-- 
Regards,

Laurent Pinchart


------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure 
contains a definitive record of customers, application performance, 
security threats, fraudulent activity, and more. Splunk takes this 
data and makes sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-novd2d

Re: [PATCH/RFC] spi: Fix device unregistration when unregistering the bus master

[Linus Walleij]

On Mon, Nov 28, 2011 at 2:23 PM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:

> I'm not too familiar with the way the SPI subsystem plugs its devices in the
> Linux device model, so I'm not sure what the right fix is. An alternative would
> be to register the SPI devices as children of the SPI master, not as siblings.

I think that is actually the right solution, so the bug is in spi_alloc_device()
where it sets:

        struct spi_device       *spi;
        struct device           *dev = master->dev.parent;

The latter should be:

        struct device           *dev = master->dev;

Just my €0.01...

Yours,
Linus Walleij

------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure 
contains a definitive record of customers, application performance, 
security threats, fraudulent activity, and more. Splunk takes this 
data and makes sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-novd2d

Re: [PATCH/RFC] spi: Fix device unregistration when unregistering the bus master

[Laurent Pinchart]

Hi Linus,

On Friday 02 December 2011 13:14:31 Linus Walleij wrote:
> On Mon, Nov 28, 2011 at 2:23 PM, Laurent Pinchart wrote:
> > I'm not too familiar with the way the SPI subsystem plugs its devices in
> > the Linux device model, so I'm not sure what the right fix is. An
> > alternative would be to register the SPI devices as children of the SPI
> > master, not as siblings.
> 
> I think that is actually the right solution, so the bug is in
> spi_alloc_device() where it sets:
> 
>         struct spi_device       *spi;
>         struct device           *dev = master->dev.parent;
> 
> The latter should be:
> 
>         struct device           *dev = master->dev;
> 
> Just my €0.01...

I agree with you, that was my initial approach as well. However, it changes 
the parent-child relationships and might break applications relying on that 
relationship in sysfs for instance. Would that be acceptable ?

-- 
Regards,

Laurent Pinchart

------------------------------------------------------------------------------
Cloud Services Checklist: Pricing and Packaging Optimization
This white paper is intended to serve as a reference, checklist and point of 
discussion for anyone considering optimizing the pricing and packaging model 
of a cloud services business. Read Now!
http://www.accelacomm.com/jaw/sfnl/114/51491232/

Re: [PATCH/RFC] spi: Fix device unregistration when unregistering the bus master

[Linus Walleij]

On Tue, Dec 6, 2011 at 5:22 PM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:
> Hi Linus,
>> The latter should be:
>>
>>         struct device           *dev = master->dev;
>>
>> Just my €0.01...
>
> I agree with you, that was my initial approach as well. However, it changes
> the parent-child relationships and might break applications relying on that
> relationship in sysfs for instance. Would that be acceptable ?

In my experience people use the /sys/class/* interfaces and don't
traverse hierarchies much.

But I'm not to speak about such things...

I would have put the patch in and see if somebody screams ...

Linus Walleij

------------------------------------------------------------------------------
Cloud Services Checklist: Pricing and Packaging Optimization
This white paper is intended to serve as a reference, checklist and point of 
discussion for anyone considering optimizing the pricing and packaging model 
of a cloud services business. Read Now!
http://www.accelacomm.com/jaw/sfnl/114/51491232/

[PATCH v2] spi: Fix device unregistration when unregistering the bus master

[Laurent Pinchart]

Device are added as children of the bus master's parent device, but
spi_unregister_master() looks for devices to unregister in the bus
master's children. This results in the child devices not being
unregistered.

Fix this by registering devices as direct children of the bus master.

Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
 drivers/spi/spi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 2e13a14..b423fe9 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -318,7 +318,7 @@ struct spi_device *spi_alloc_device(struct spi_master *master)
 	}
 
 	spi->master = master;
-	spi->dev.parent = dev;
+	spi->dev.parent = &master->dev;
 	spi->dev.bus = &spi_bus_type;
 	spi->dev.release = spidev_release;
 	device_initialize(&spi->dev);
-- 
Regards,

Laurent Pinchart


------------------------------------------------------------------------------
Learn Windows Azure Live!  Tuesday, Dec 13, 2011
Microsoft is holding a special Learn Windows Azure training event for 
developers. It will provide a great way to learn Windows Azure and what it 
provides. You can attend the event by watching it streamed LIVE online.  
Learn more at http://p.sf.net/sfu/ms-windowsazure

Re: [PATCH v2] spi: Fix device unregistration when unregistering the bus master

[Linus Walleij]

On Mon, Dec 12, 2011 at 1:15 AM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:

> Device are added as children of the bus master's parent device, but
> spi_unregister_master() looks for devices to unregister in the bus
> master's children. This results in the child devices not being
> unregistered.
>
> Fix this by registering devices as direct children of the bus master.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Thanks,
Linus Walleij

------------------------------------------------------------------------------
Systems Optimization Self Assessment
Improve efficiency and utilization of IT resources. Drive out cost and 
improve service delivery. Take 5 minutes to use this Systems Optimization 
Self Assessment. http://www.accelacomm.com/jaw/sdnl/114/51450054/

Re: [PATCH v2] spi: Fix device unregistration when unregistering the bus master

[Grant Likely]

On Mon, Dec 12, 2011 at 01:15:06AM +0100, Laurent Pinchart wrote:
> Device are added as children of the bus master's parent device, but
> spi_unregister_master() looks for devices to unregister in the bus
> master's children. This results in the child devices not being
> unregistered.
> 
> Fix this by registering devices as direct children of the bus master.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> ---
>  drivers/spi/spi.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 2e13a14..b423fe9 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -318,7 +318,7 @@ struct spi_device *spi_alloc_device(struct spi_master *master)
>  	}
>  
>  	spi->master = master;
> -	spi->dev.parent = dev;
> +	spi->dev.parent = &master->dev;
>  	spi->dev.bus = &spi_bus_type;
>  	spi->dev.release = spidev_release;
>  	device_initialize(&spi->dev);

Yes, I agree and I think this is the correct fix.  However, I'd like
to make sure it behaves correctly.  Can you test and confirm that
with this patch in place and an spi_device in use (bound to a driver)
that the kernel will either refuse to unbind the driver, or correctly
release the spi_driver from the spi_device?

Thanks,
g.


------------------------------------------------------------------------------
Cloud Computing - Latest Buzzword or a Glimpse of the Future?
This paper surveys cloud computing today: What are the benefits? 
Why are businesses embracing it? What are its payoffs and pitfalls?
http://www.accelacomm.com/jaw/sdnl/114/51425149/

Re: [PATCH v2] spi: Fix device unregistration when unregistering the bus master

[Laurent Pinchart]

Hi Grant,

On Wednesday 14 December 2011 00:45:20 Grant Likely wrote:
> On Mon, Dec 12, 2011 at 01:15:06AM +0100, Laurent Pinchart wrote:
> > Device are added as children of the bus master's parent device, but
> > spi_unregister_master() looks for devices to unregister in the bus
> > master's children. This results in the child devices not being
> > unregistered.
> > 
> > Fix this by registering devices as direct children of the bus master.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> > ---
> > 
> >  drivers/spi/spi.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index 2e13a14..b423fe9 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -318,7 +318,7 @@ struct spi_device *spi_alloc_device(struct spi_master
> > *master)
> > 
> >  	}
> >  	
> >  	spi->master = master;
> > 
> > -	spi->dev.parent = dev;
> > +	spi->dev.parent = &master->dev;
> > 
> >  	spi->dev.bus = &spi_bus_type;
> >  	spi->dev.release = spidev_release;
> >  	device_initialize(&spi->dev);
> 
> Yes, I agree and I think this is the correct fix.  However, I'd like
> to make sure it behaves correctly.  Can you test and confirm that
> with this patch in place and an spi_device in use (bound to a driver)
> that the kernel will either refuse to unbind the driver, or correctly
> release the spi_driver from the spi_device?

$ ls -l /sys/bus/spi/devices/spi0.0/
lrwxrwxrwx    1 root     root            0 Jan  1 01:23 driver -> 
../../../../../../bus/spi/drivers/m25p80
-r--r--r--    1 root     root         4096 Jan  1 01:23 modalias
drwxr-xr-x    4 root     root            0 Jan  1 01:23 mtd
drwxr-xr-x    2 root     root            0 Jan  1 01:23 power
lrwxrwxrwx    1 root     root            0 Jan  1 01:23 subsystem -> 
../../../../../../bus/spi
-rw-r--r--    1 root     root         4096 Jan  1 01:23 uevent

$ ls -l /dev/mtd1*
crw-rw----    1 root     root      90,   2 Jan  1 01:23 /dev/mtd1
crw-rw----    1 root     root      90,   3 Jan  1 01:23 /dev/mtd1ro

$ echo spi0.0 > /sys/bus/spi/drivers/m25p80/unbind

$ ls -l /sys/bus/spi/devices/spi0.0/
-r--r--r--    1 root     root         4096 Jan  1 01:23 modalias
drwxr-xr-x    2 root     root            0 Jan  1 01:23 power
lrwxrwxrwx    1 root     root            0 Jan  1 01:23 subsystem -> 
../../../../../../bus/spi
-rw-r--r--    1 root     root         4096 Jan  1 01:23 uevent

$ ls -l /dev/mtd1*
ls: /dev/mtd1*: No such file or directory

Is that what you were asking for ?

-- 
Regards,

Laurent Pinchart

------------------------------------------------------------------------------
Learn Windows Azure Live!  Tuesday, Dec 13, 2011
Microsoft is holding a special Learn Windows Azure training event for 
developers. It will provide a great way to learn Windows Azure and what it 
provides. You can attend the event by watching it streamed LIVE online.  
Learn more at http://p.sf.net/sfu/ms-windowsazure

Re: [PATCH v2] spi: Fix device unregistration when unregistering the bus master

[Grant Likely]

On Fri, Dec 16, 2011 at 9:52 AM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:
> Hi Grant,
>
> On Wednesday 14 December 2011 00:45:20 Grant Likely wrote:
>> On Mon, Dec 12, 2011 at 01:15:06AM +0100, Laurent Pinchart wrote:
>> > Device are added as children of the bus master's parent device, but
>> > spi_unregister_master() looks for devices to unregister in the bus
>> > master's children. This results in the child devices not being
>> > unregistered.
>> >
>> > Fix this by registering devices as direct children of the bus master.
>> >
>> > Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>> > ---
>> >
>> >  drivers/spi/spi.c |    2 +-
>> >  1 files changed, 1 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> > index 2e13a14..b423fe9 100644
>> > --- a/drivers/spi/spi.c
>> > +++ b/drivers/spi/spi.c
>> > @@ -318,7 +318,7 @@ struct spi_device *spi_alloc_device(struct spi_master
>> > *master)
>> >
>> >     }
>> >
>> >     spi->master = master;
>> >
>> > -   spi->dev.parent = dev;
>> > +   spi->dev.parent = &master->dev;
>> >
>> >     spi->dev.bus = &spi_bus_type;
>> >     spi->dev.release = spidev_release;
>> >     device_initialize(&spi->dev);
>>
>> Yes, I agree and I think this is the correct fix.  However, I'd like
>> to make sure it behaves correctly.  Can you test and confirm that
>> with this patch in place and an spi_device in use (bound to a driver)
>> that the kernel will either refuse to unbind the driver, or correctly
>> release the spi_driver from the spi_device?
>
> $ ls -l /sys/bus/spi/devices/spi0.0/
> lrwxrwxrwx    1 root     root            0 Jan  1 01:23 driver ->
> ../../../../../../bus/spi/drivers/m25p80
> -r--r--r--    1 root     root         4096 Jan  1 01:23 modalias
> drwxr-xr-x    4 root     root            0 Jan  1 01:23 mtd
> drwxr-xr-x    2 root     root            0 Jan  1 01:23 power
> lrwxrwxrwx    1 root     root            0 Jan  1 01:23 subsystem ->
> ../../../../../../bus/spi
> -rw-r--r--    1 root     root         4096 Jan  1 01:23 uevent
>
> $ ls -l /dev/mtd1*
> crw-rw----    1 root     root      90,   2 Jan  1 01:23 /dev/mtd1
> crw-rw----    1 root     root      90,   3 Jan  1 01:23 /dev/mtd1ro
>
> $ echo spi0.0 > /sys/bus/spi/drivers/m25p80/unbind
>
> $ ls -l /sys/bus/spi/devices/spi0.0/
> -r--r--r--    1 root     root         4096 Jan  1 01:23 modalias
> drwxr-xr-x    2 root     root            0 Jan  1 01:23 power
> lrwxrwxrwx    1 root     root            0 Jan  1 01:23 subsystem ->
> ../../../../../../bus/spi
> -rw-r--r--    1 root     root         4096 Jan  1 01:23 uevent
>
> $ ls -l /dev/mtd1*
> ls: /dev/mtd1*: No such file or directory
>
> Is that what you were asking for ?

Not really, I was referring to having an spi_device in use, and then
attempting to unbind/remove the spi_master driver.

g.

------------------------------------------------------------------------------
Learn Windows Azure Live!  Tuesday, Dec 13, 2011
Microsoft is holding a special Learn Windows Azure training event for 
developers. It will provide a great way to learn Windows Azure and what it 
provides. You can attend the event by watching it streamed LIVE online.  
Learn more at http://p.sf.net/sfu/ms-windowsazure

Re: [PATCH v2] spi: Fix device unregistration when unregistering the bus master

[Laurent Pinchart]

Hi Grant,

On Friday 16 December 2011 17:59:20 Grant Likely wrote:
> On Fri, Dec 16, 2011 at 9:52 AM, Laurent Pinchart wrote:
> > On Wednesday 14 December 2011 00:45:20 Grant Likely wrote:
> >> On Mon, Dec 12, 2011 at 01:15:06AM +0100, Laurent Pinchart wrote:
> >> > Device are added as children of the bus master's parent device, but
> >> > spi_unregister_master() looks for devices to unregister in the bus
> >> > master's children. This results in the child devices not being
> >> > unregistered.
> >> > 
> >> > Fix this by registering devices as direct children of the bus master.
> >> > 
> >> > Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> >> > ---
> >> > 
> >> >  drivers/spi/spi.c |    2 +-
> >> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >> > 
> >> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> >> > index 2e13a14..b423fe9 100644
> >> > --- a/drivers/spi/spi.c
> >> > +++ b/drivers/spi/spi.c
> >> > @@ -318,7 +318,7 @@ struct spi_device *spi_alloc_device(struct
> >> > spi_master *master)
> >> > 
> >> >     }
> >> > 
> >> >     spi->master = master;
> >> > 
> >> > -   spi->dev.parent = dev;
> >> > +   spi->dev.parent = &master->dev;
> >> > 
> >> >     spi->dev.bus = &spi_bus_type;
> >> >     spi->dev.release = spidev_release;
> >> >     device_initialize(&spi->dev);
> >> 
> >> Yes, I agree and I think this is the correct fix.  However, I'd like
> >> to make sure it behaves correctly.  Can you test and confirm that
> >> with this patch in place and an spi_device in use (bound to a driver)
> >> that the kernel will either refuse to unbind the driver, or correctly
> >> release the spi_driver from the spi_device?
> > 
> > $ ls -l /sys/bus/spi/devices/spi0.0/
> > lrwxrwxrwx    1 root     root            0 Jan  1 01:23 driver ->
> > ../../../../../../bus/spi/drivers/m25p80
> > -r--r--r--    1 root     root         4096 Jan  1 01:23 modalias
> > drwxr-xr-x    4 root     root            0 Jan  1 01:23 mtd
> > drwxr-xr-x    2 root     root            0 Jan  1 01:23 power
> > lrwxrwxrwx    1 root     root            0 Jan  1 01:23 subsystem ->
> > ../../../../../../bus/spi
> > -rw-r--r--    1 root     root         4096 Jan  1 01:23 uevent
> > 
> > $ ls -l /dev/mtd1*
> > crw-rw----    1 root     root      90,   2 Jan  1 01:23 /dev/mtd1
> > crw-rw----    1 root     root      90,   3 Jan  1 01:23 /dev/mtd1ro
> > 
> > $ echo spi0.0 > /sys/bus/spi/drivers/m25p80/unbind
> > 
> > $ ls -l /sys/bus/spi/devices/spi0.0/
> > -r--r--r--    1 root     root         4096 Jan  1 01:23 modalias
> > drwxr-xr-x    2 root     root            0 Jan  1 01:23 power
> > lrwxrwxrwx    1 root     root            0 Jan  1 01:23 subsystem ->
> > ../../../../../../bus/spi
> > -rw-r--r--    1 root     root         4096 Jan  1 01:23 uevent
> > 
> > $ ls -l /dev/mtd1*
> > ls: /dev/mtd1*: No such file or directory
> > 
> > Is that what you were asking for ?
> 
> Not really, I was referring to having an spi_device in use, and then
> attempting to unbind/remove the spi_master driver.

Sorry.

My board has a single SPI device, which is an MTD flash. I've tried keeping 
/dev/mtd1 open and unloading the SPI master driver. This unfortunately 
succeed, and accessing /dev/mtd1 then results in a kernel crash.

However, a probably even worse news is that my patch doesn't make things 
worse: the problem can be reproduced with a vanilla v3.0.

-- 
Regards,

Laurent Pinchart

------------------------------------------------------------------------------
Learn Windows Azure Live!  Tuesday, Dec 13, 2011
Microsoft is holding a special Learn Windows Azure training event for 
developers. It will provide a great way to learn Windows Azure and what it 
provides. You can attend the event by watching it streamed LIVE online.  
Learn more at http://p.sf.net/sfu/ms-windowsazure