Re: dma-debug: add a check dma memory leaks

Re: dma-debug: add a check dma memory leaks

[David Woodhouse]

On Mon, 2009-03-30 at 21:01 +0000, Linux Kernel Mailing List wrote:
> Gitweb:     http://git.kernel.org/linus/41531c8f5f05aba5ec645d9770557eedbf75b422
> Commit:     41531c8f5f05aba5ec645d9770557eedbf75b422

>     dma-debug: add a check dma memory leaks
>     
>     Impact: allow architectures to monitor busses for dma mem leakage
>     
>     This patch adds checking code to detect if a device has pending DMA
>     operations when it is about to be unbound from its device driver.
>     
>     Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>


> +static int dma_debug_device_change(struct notifier_block *nb,
> +				    unsigned long action, void *data)
> +{
> +	struct device *dev = data;
> +	int count;
> +
> +
> +	switch (action) {
> +	case BUS_NOTIFY_UNBIND_DRIVER:
> +		count = device_dma_allocations(dev);
> +		if (count == 0)
> +			break;
> +		err_printk(dev, NULL, "DMA-API: device driver has pending "
> +				"DMA allocations while released from device "
> +				"[count=%d]\n", count);

Hm, cute... but not quite functioning as you intended. If you look at
__device_release_driver() in drivers/base/dd.c you'll see it actually
calls the notifier _before_ calling into the driver's ->remove() method.
So it's hardly surprising that not everything has been freed yet...

Reported by Reinette when it bit iwlwifi.

Ben, can we get away with changing the order so that the ->remove() is
called before the notifier, in this case? 

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: dma-debug: add a check dma memory leaks

[Joerg Roedel]

On Fri, Apr 17, 2009 at 10:51:05PM +0100, David Woodhouse wrote:
> On Mon, 2009-03-30 at 21:01 +0000, Linux Kernel Mailing List wrote:
> > Gitweb:     http://git.kernel.org/linus/41531c8f5f05aba5ec645d9770557eedbf75b422
> > Commit:     41531c8f5f05aba5ec645d9770557eedbf75b422
> 
> >     dma-debug: add a check dma memory leaks
> >     
> >     Impact: allow architectures to monitor busses for dma mem leakage
> >     
> >     This patch adds checking code to detect if a device has pending DMA
> >     operations when it is about to be unbound from its device driver.
> >     
> >     Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> 
> 
> > +static int dma_debug_device_change(struct notifier_block *nb,
> > +				    unsigned long action, void *data)
> > +{
> > +	struct device *dev = data;
> > +	int count;
> > +
> > +
> > +	switch (action) {
> > +	case BUS_NOTIFY_UNBIND_DRIVER:
> > +		count = device_dma_allocations(dev);
> > +		if (count == 0)
> > +			break;
> > +		err_printk(dev, NULL, "DMA-API: device driver has pending "
> > +				"DMA allocations while released from device "
> > +				"[count=%d]\n", count);
> 
> Hm, cute... but not quite functioning as you intended. If you look at
> __device_release_driver() in drivers/base/dd.c you'll see it actually
> calls the notifier _before_ calling into the driver's ->remove() method.
> So it's hardly surprising that not everything has been freed yet...
> 
> Reported by Reinette when it bit iwlwifi.
> 
> Ben, can we get away with changing the order so that the ->remove() is
> called before the notifier, in this case?

Ben? I would like to keep this check. If its not possible to move this
one behind the drivers ->remove function it may be an option to add
another notifier?

Joerg

-- 
           | Advanced Micro Devices GmbH
 Operating | Karl-Hammerschmidt-Str. 34, 85609 Dornach bei München
 System    | 
 Research  | Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
 Center    | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
           | Registergericht München, HRB Nr. 43632

Re: dma-debug: add a check dma memory leaks

[David Woodhouse]

Subject: Add BUS_NOTIFY_UNBOUND_DRIVER callback after driver removal

This adds a notifier callback which happens _after_ the driver has been
unbound from the device, needed for things like the DMA debugging API
which want to check that all DMA mappings have been correctly torn down.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
On Tue, 2009-04-21 at 11:20 +0200, Joerg Roedel wrote:
> On Fri, Apr 17, 2009 at 10:51:05PM +0100, David Woodhouse wrote:
> > Hm, cute... but not quite functioning as you intended. If you look at
> > __device_release_driver() in drivers/base/dd.c you'll see it actually
> > calls the notifier _before_ calling into the driver's ->remove() method.
> > So it's hardly surprising that not everything has been freed yet...
> > 
> > Reported by Reinette when it bit iwlwifi.
> > 
> > Ben, can we get away with changing the order so that the ->remove() is
> > called before the notifier, in this case?
> 
> Ben? I would like to keep this check. If its not possible to move this
> one behind the drivers ->remove function it may be an option to add
> another notifier?

It doesn't look like it's possible to move the UNBIND callback. Let's
add an UNBOUND callback instead... I've tested this and it fixes the
false positives.

Jörg, should you be using UNBOUND instead of UNBIND in the amd_iommu
notifier callback too?

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 742cbe6..efd00de 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -320,6 +320,10 @@ static void __device_release_driver(struct device *dev)
 		devres_release_all(dev);
 		dev->driver = NULL;
 		klist_remove(&dev->p->knode_driver);
+		if (dev->bus)
+			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
+						     BUS_NOTIFY_UNBOUND_DRIVER,
+						     dev);
 	}
 }
 
diff --git a/include/linux/device.h b/include/linux/device.h
index 6a69caa..4ded2ae 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -116,6 +116,7 @@ extern int bus_unregister_notifier(struct bus_type *bus,
 #define BUS_NOTIFY_BOUND_DRIVER		0x00000003 /* driver bound to device */
 #define BUS_NOTIFY_UNBIND_DRIVER	0x00000004 /* driver about to be
 						      unbound */
+#define BUS_NOTIFY_UNBOUND_DRIVER	0x00000005 /* driver has been unbound */
 
 extern struct kset *bus_get_kset(struct bus_type *bus);
 extern struct klist *bus_get_device_klist(struct bus_type *bus);


-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: dma-debug: add a check dma memory leaks

[Benjamin Herrenschmidt]

On Sun, 2009-05-10 at 00:59 +0100, David Woodhouse wrote:
> Subject: Add BUS_NOTIFY_UNBOUND_DRIVER callback after driver removal
> 
> This adds a notifier callback which happens _after_ the driver has been
> unbound from the device, needed for things like the DMA debugging API
> which want to check that all DMA mappings have been correctly torn down.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> ---
> On Tue, 2009-04-21 at 11:20 +0200, Joerg Roedel wrote:
> > On Fri, Apr 17, 2009 at 10:51:05PM +0100, David Woodhouse wrote:
> > > Hm, cute... but not quite functioning as you intended. If you look at
> > > __device_release_driver() in drivers/base/dd.c you'll see it actually
> > > calls the notifier _before_ calling into the driver's ->remove() method.
> > > So it's hardly surprising that not everything has been freed yet...
> > > 
> > > Reported by Reinette when it bit iwlwifi.
> > > 
> > > Ben, can we get away with changing the order so that the ->remove() is
> > > called before the notifier, in this case?
> > 
> > Ben? I would like to keep this check. If its not possible to move this
> > one behind the drivers ->remove function it may be an option to add
> > another notifier?
> 
> It doesn't look like it's possible to move the UNBIND callback. Let's
> add an UNBOUND callback instead... I've tested this and it fixes the
> false positives.
> 
> Jörg, should you be using UNBOUND instead of UNBIND in the amd_iommu
> notifier callback too?
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 742cbe6..efd00de 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -320,6 +320,10 @@ static void __device_release_driver(struct device *dev)
>  		devres_release_all(dev);
>  		dev->driver = NULL;
>  		klist_remove(&dev->p->knode_driver);
> +		if (dev->bus)
> +			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> +						     BUS_NOTIFY_UNBOUND_DRIVER,
> +						     dev);
>  	}
>  }
>  
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 6a69caa..4ded2ae 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -116,6 +116,7 @@ extern int bus_unregister_notifier(struct bus_type *bus,
>  #define BUS_NOTIFY_BOUND_DRIVER		0x00000003 /* driver bound to device */
>  #define BUS_NOTIFY_UNBIND_DRIVER	0x00000004 /* driver about to be
>  						      unbound */
> +#define BUS_NOTIFY_UNBOUND_DRIVER	0x00000005 /* driver has been unbound */
>  
>  extern struct kset *bus_get_kset(struct bus_type *bus);
>  extern struct klist *bus_get_device_klist(struct bus_type *bus);
> 
> 

Re: dma-debug: add a check dma memory leaks

[Greg KH]

On Sun, May 10, 2009 at 12:59:12AM +0100, David Woodhouse wrote:
> Subject: Add BUS_NOTIFY_UNBOUND_DRIVER callback after driver removal
> 
> This adds a notifier callback which happens _after_ the driver has been
> unbound from the device, needed for things like the DMA debugging API
> which want to check that all DMA mappings have been correctly torn down.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

I don't understand.  Who wrote this patch, Joerg or you?  I have this in
my tree already as written by Joerg.

confused,

greg k-h

Re: dma-debug: add a check dma memory leaks

[David Woodhouse]

On Sat, 2009-05-09 at 19:09 -0700, Greg KH wrote:
> On Sun, May 10, 2009 at 12:59:12AM +0100, David Woodhouse wrote:
> > Subject: Add BUS_NOTIFY_UNBOUND_DRIVER callback after driver removal
> > 
> > This adds a notifier callback which happens _after_ the driver has been
> > unbound from the device, needed for things like the DMA debugging API
> > which want to check that all DMA mappings have been correctly torn down.
> > 
> > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> 
> I don't understand.  Who wrote this patch, Joerg or you?  I have this in
> my tree already as written by Joerg.

In that case we probably both did it, independently. I did it last night
after finally catching up with Ben and talking about the possibility of
moving the existing notifiers. Jörg may have done it sooner.

You can drop my version, then.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: dma-debug: add a check dma memory leaks

[Benjamin Herrenschmidt]

On Sun, 2009-05-10 at 09:49 +0100, David Woodhouse wrote:
> > I don't understand.  Who wrote this patch, Joerg or you?  I have
> this in
> > my tree already as written by Joerg.
> 
> In that case we probably both did it, independently. I did it last
> night
> after finally catching up with Ben and talking about the possibility
> of
> moving the existing notifiers. Jörg may have done it sooner.
> 
> You can drop my version, then.

As long as it's strictly identical :-) IE. I don't want to -move- the
existing notifier, but adding a "BOUND" one is fine.

Cheers,
Ben.

Re: dma-debug: add a check dma memory leaks

[Greg KH]

On Sun, May 10, 2009 at 07:32:12PM +1000, Benjamin Herrenschmidt wrote:
> On Sun, 2009-05-10 at 09:49 +0100, David Woodhouse wrote:
> > > I don't understand.  Who wrote this patch, Joerg or you?  I have
> > this in
> > > my tree already as written by Joerg.
> > 
> > In that case we probably both did it, independently. I did it last
> > night
> > after finally catching up with Ben and talking about the possibility
> > of
> > moving the existing notifiers. Jörg may have done it sooner.
> > 
> > You can drop my version, then.
> 
> As long as it's strictly identical :-) IE. I don't want to -move- the
> existing notifier, but adding a "BOUND" one is fine.

Look in the linux-next tree to verify that it is the same.

Or here's a direct link to the patch:
	http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-03-driver-core/driver-core-add-bus_notify_unbound_driver-event.patch

thanks,

greg k-h