Re: [PATCH] ppc/iommu: prevent false TCE leak message

[Thadeu Lima de Souza Cascardo <cascardo@linux.vnet.ibm.com>]

On Fri, Dec 28, 2012 at 01:21:35PM +0800, Gavin Shan wrote:
> On Thu, Dec 27, 2012 at 02:28:06PM -0200, Thadeu Lima de Souza Cascardo wrote:
> >When a device DMA window includes the address 0, it's reserved in the
> >TCE bitmap to avoid returning that address to drivers.
> >
> >When the device is removed, the bitmap is checked for any mappings not
> >removed by the driver, indicating a possible DMA mapping leak. Since the
> >reserved address is not cleared, a message is printed, warning of such a
> >leak.
> >
> >Check for the reservation, and clear it before checking for any other
> >standing mappings.
> >
> >Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@linux.vnet.ibm.com>
> >---
> > arch/powerpc/kernel/iommu.c |    5 +++++
> > 1 files changed, 5 insertions(+), 0 deletions(-)
> >
> >diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> >index 8226c6c..226e9e5 100644
> >--- a/arch/powerpc/kernel/iommu.c
> >+++ b/arch/powerpc/kernel/iommu.c
> >@@ -717,6 +717,11 @@ void iommu_free_table(struct iommu_table *tbl, const char *node_name)
> > 		return;
> > 	}
> >
> >+	/* In case we have reserved the first bit, we should not emit
> >+	 * the warning below. */
> 
> At least, the comment would look like:
> 
> 	/*
> 	 * xxxxxxx
> 	 */
> 

Sure. New code should always follow coding style.  :-)

How do you suggest merging with the comment below? I think it's closer
to the code it comments about, so I'd rather keep it where it is.

> >+	if (tbl->it_offset == 0)
> >+		clear_bit(0, tbl->it_map);
> >+
> > 	/* verify that table contains no entries */
> > 	/* it_size is in entries, and we're examining 64 at a time */
> 
> The comment would be merged as well? :-)
> 

I also considered replacing this code by this:

        /* verify that table contains no entries */
-       /* it_size is in entries, and we're examining 64 at a time */
-       for (i = 0; i < (tbl->it_size/64); i++) {
-               if (tbl->it_map[i] != 0) {
+       if (find_first_bit(tbl->it_map, tbl->it_size) < tbl->it_size)
                        printk(KERN_WARNING "%s: Unexpected TCEs for
%s\n",
                                __func__, node_name);
-                       break;
-               }
-       }
 
I'll resend the "unreserving" patch with the fixed comment styling, but
without merging comments. And I will send this other patch for comments.

Regards.
Thadeu Cascardo.

 
> > 	for (i = 0; i < (tbl->it_size/64); i++) {
> 
> Thanks,
> Gavin