* Re: [Cbe-oss-dev] [RFC] Cell: shutdown method for spu_sysdev_class [not found] <463D1A2F.10708@am.sony.com> @ 2007-05-29 18:23 ` Arnd Bergmann 2007-05-30 9:58 ` Christoph Hellwig 0 siblings, 1 reply; 3+ messages in thread From: Arnd Bergmann @ 2007-05-29 18:23 UTC (permalink / raw) To: cbe-oss-dev; +Cc: Geoff Levand, linux-kernel, gregkh On Sunday 06 May 2007, Geoff Levand wrote: > --- ps3-linux-dev.orig/arch/powerpc/platforms/cell/spu_base.c > +++ ps3-linux-dev/arch/powerpc/platforms/cell/spu_base.c > @@ -463,8 +463,21 @@ void spu_free(struct spu *spu) > } > EXPORT_SYMBOL_GPL(spu_free); > > +static int spu_shutdown(struct sys_device *sysdev) > +{ > + struct spu *spu = container_of(sysdev, struct spu, sysdev); > + > + // what else here??? > + > + spu_free_irqs(spu); > + spu_destroy_spu(spu); > + kfree(spu); > + return 0; > +} > + > struct sysdev_class spu_sysdev_class = { > - set_kset_name("spu") > + set_kset_name("spu"), > + .shutdown = spu_shutdown, > }; > > int spu_add_sysdev_attr(struct sysdev_attribute *attr) After some debugging, I found that this patch creates an oops when slab debugging is enabled, the reason for that being that sysdev_shutdown() iterates over all system devices using list_for_each_entry(), not list_for_each_entry_safe(). There are two ways of fixing this: - use list_for_each_entry_safe() to go over all devices so they can be freed in their ->shutdown method. - not free the device, because we know the system is going down anyway. Is it documented or implied somewhere that ->shutdown must not free the device? If not, the first option is probably the safer choice. Arnd <>< ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Cbe-oss-dev] [RFC] Cell: shutdown method for spu_sysdev_class 2007-05-29 18:23 ` [Cbe-oss-dev] [RFC] Cell: shutdown method for spu_sysdev_class Arnd Bergmann @ 2007-05-30 9:58 ` Christoph Hellwig 2007-05-30 11:03 ` Arnd Bergmann 0 siblings, 1 reply; 3+ messages in thread From: Christoph Hellwig @ 2007-05-30 9:58 UTC (permalink / raw) To: Arnd Bergmann; +Cc: cbe-oss-dev, gregkh, linux-kernel On Tue, May 29, 2007 at 08:23:49PM +0200, Arnd Bergmann wrote: > On Sunday 06 May 2007, Geoff Levand wrote: > > --- ps3-linux-dev.orig/arch/powerpc/platforms/cell/spu_base.c > > +++ ps3-linux-dev/arch/powerpc/platforms/cell/spu_base.c > > @@ -463,8 +463,21 @@ void spu_free(struct spu *spu) > > } > > EXPORT_SYMBOL_GPL(spu_free); > > > > +static int spu_shutdown(struct sys_device *sysdev) > > +{ > > + struct spu *spu = container_of(sysdev, struct spu, sysdev); > > + > > + // what else here??? > > + > > + spu_free_irqs(spu); > > + spu_destroy_spu(spu); > > + kfree(spu); > > + return 0; > > +} > > + > > struct sysdev_class spu_sysdev_class = { > > - set_kset_name("spu") > > + set_kset_name("spu"), > > + .shutdown = spu_shutdown, > > }; > > > > int spu_add_sysdev_attr(struct sysdev_attribute *attr) > > After some debugging, I found that this patch creates an oops when slab > debugging is enabled, the reason for that being that sysdev_shutdown() > iterates over all system devices using list_for_each_entry(), not > list_for_each_entry_safe(). > > There are two ways of fixing this: > > - use list_for_each_entry_safe() to go over all devices so they > can be freed in their ->shutdown method. > - not free the device, because we know the system is going down > anyway. > > Is it documented or implied somewhere that ->shutdown must not free > the device? If not, the first option is probably the safer choice. It's not documented anywhere, but implicitly assumed. I don't know of any shutdown implementation that frees the device. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Cbe-oss-dev] [RFC] Cell: shutdown method for spu_sysdev_class 2007-05-30 9:58 ` Christoph Hellwig @ 2007-05-30 11:03 ` Arnd Bergmann 0 siblings, 0 replies; 3+ messages in thread From: Arnd Bergmann @ 2007-05-30 11:03 UTC (permalink / raw) To: Christoph Hellwig; +Cc: cbe-oss-dev, gregkh, linux-kernel, Geoff Levand On Wednesday 30 May 2007, Christoph Hellwig wrote: > > > Is it documented or implied somewhere that ->shutdown must not free > > the device? If not, the first option is probably the safer choice. > > It's not documented anywhere, but implicitly assumed. I don't know > of any shutdown implementation that frees the device. Right, I found the explanation now myself: The ->shutdown method is called for system devices that are still part of a linked linked list. Freeing the object would destroy that list. Geoff, please merge the patch below in your tree. Arnd <>< --- Subject: cell: don't free spu objects in sysdev shutdown From: Arnd Bergmann <arnd.bergmann@de.ibm.com> System devices are accessed after they are shut down, so we must not free the data structures. Signed-off-by: Arnd Bergmann <arnd.bergmann@de.ibm.com> Index: linux-2.6/arch/powerpc/platforms/cell/spu_base.c =================================================================== --- linux-2.6.orig/arch/powerpc/platforms/cell/spu_base.c +++ linux-2.6/arch/powerpc/platforms/cell/spu_base.c @@ -517,7 +517,6 @@ static int spu_shutdown(struct sys_devic spu_free_irqs(spu); spu_destroy_spu(spu); - kfree(spu); return 0; } ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2007-05-30 11:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <463D1A2F.10708@am.sony.com>
2007-05-29 18:23 ` [Cbe-oss-dev] [RFC] Cell: shutdown method for spu_sysdev_class Arnd Bergmann
2007-05-30 9:58 ` Christoph Hellwig
2007-05-30 11:03 ` Arnd Bergmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox