Skip to contentSkip to conversation

parisc: BUG_ON() cleanup

PATCH In review Helge Deller 3 participants 4 messages Jan 6, 2009 PA-RISC architecture

lore
Share & tools
lore

Helge Deller

author maintainer
#0 lore raw

PATCH 1/1 parisc: BUG_ON() cleanup

- convert a few "if (xx) BUG();" to BUG_ON(xx)
- remove a few printk()s, as we get a backtrace with BUG_ON() anyway

Signed-off-by Helge Deller <[email protected]>
 kernel/cache.c   |    5 +----
 kernel/pci-dma.c |   22 ++++++----------------
 mm/init.c        |    6 ++----
 3 files changed, 9 insertions(+), 24 deletions(-)
arch/parisc/kernel/cache.c +1 -4
index 5259d8c..837530e 100644
@@ -551,10 +551,7 @@ void flush_cache_range(struct vm_area_struct *vma,
 {
 	int sr3;
 
-	if (!vma->vm_mm->context) {
-		BUG();
-		return;
-	}
+	BUG_ON(!vma->vm_mm->context);
 
 	sr3 = mfsp(3);
 	if (vma->vm_mm->context == sr3) {
arch/parisc/kernel/pci-dma.c +6 -16
index ccd61b9..7fae3a8 100644
@@ -447,10 +447,7 @@ static void pa11_dma_free_consistent (struct device *dev, size_t size, void *vad
 
 static dma_addr_t pa11_dma_map_single(struct device *dev, void *addr, size_t size, enum dma_data_direction direction)
 {
-	if (direction == DMA_NONE) {
-		printk(KERN_ERR "pa11_dma_map_single(PCI_DMA_NONE) called by %p\n", __builtin_return_address(0));
-		BUG();
-	}
+	BUG_ON(direction == DMA_NONE);
 
 	flush_kernel_dcache_range((unsigned long) addr, size);
 	return virt_to_phys(addr);

@@ -458,10 +455,7 @@ static dma_addr_t pa11_dma_map_single(struct device *dev, void *addr, size_t siz
 
 static void pa11_dma_unmap_single(struct device *dev, dma_addr_t dma_handle, size_t size, enum dma_data_direction direction)
 {
-	if (direction == DMA_NONE) {
-		printk(KERN_ERR "pa11_dma_unmap_single(PCI_DMA_NONE) called by %p\n", __builtin_return_address(0));
-		BUG();
-	}
+	BUG_ON(direction == DMA_NONE);
 
 	if (direction == DMA_TO_DEVICE)
 	    return;

@@ -480,8 +474,7 @@ static int pa11_dma_map_sg(struct device *dev, struct scatterlist *sglist, int n
 {
 	int i;
 
-	if (direction == DMA_NONE)
-	    BUG();
+	BUG_ON(direction == DMA_NONE);
 
 	for (i = 0; i < nents; i++, sglist++ ) {
 		unsigned long vaddr = sg_virt_addr(sglist);

@@ -496,8 +489,7 @@ static void pa11_dma_unmap_sg(struct device *dev, struct scatterlist *sglist, in
 {
 	int i;
 
-	if (direction == DMA_NONE)
-	    BUG();
+	BUG_ON(direction == DMA_NONE);
 
 	if (direction == DMA_TO_DEVICE)
 	    return;

@@ -511,16 +503,14 @@ static void pa11_dma_unmap_sg(struct device *dev, struct scatterlist *sglist, in
 
 static void pa11_dma_sync_single_for_cpu(struct device *dev, dma_addr_t dma_handle, unsigned long offset, size_t size, enum dma_data_direction direction)
 {
-	if (direction == DMA_NONE)
-	    BUG();
+	BUG_ON(direction == DMA_NONE);
 
 	flush_kernel_dcache_range((unsigned long) phys_to_virt(dma_handle) + offset, size);
 }
 
 static void pa11_dma_sync_single_for_device(struct device *dev, dma_addr_t dma_handle, unsigned long offset, size_t size, enum dma_data_direction direction)
 {
-	if (direction == DMA_NONE)
-	    BUG();
+	BUG_ON(direction == DMA_NONE);
 
 	flush_kernel_dcache_range((unsigned long) phys_to_virt(dma_handle) + offset, size);
 }
arch/parisc/mm/init.c +2 -4
index 7c155c2..9d704d9 100644
@@ -304,10 +304,8 @@ static void __init setup_bootmem(void)
 	 */
 	max_low_pfn = max_pfn;
 
-	if ((bootmap_pfn - bootmap_start_pfn) != bootmap_pages) {
-		printk(KERN_WARNING "WARNING! bootmap sizing is messed up!\n");
-		BUG();
-	}
+	/* bootmap sizing messed up? */
+	BUG_ON((bootmap_pfn - bootmap_start_pfn) != bootmap_pages);
 
 	/* reserve PAGE0 pdc memory, kernel text/data/bss & bootmap */
maintainer
#1 lore raw
Quoted 1 line · Helge Deller

- convert a few "if (xx) BUG();" to BUG_ON(xx)

This is fine

Quoted 1 line

- remove a few printk()s, as we get a backtrace with BUG_ON() anyway

This is less helpful. Each of the printks explains why the bug triggers. In theory you can work this out from the BUG_ON line number, but *only* if your source files match those of the reporter, which isn't the case in an annoyingly large number of bug reports ... speaking as someone who seems to get to diagnose large numbers of bugs, it makes my life harder.

James

author maintainer
#2 lore raw
Quoted 5 lines · James Bottomley
Quoted 1 line · Helge Deller

- convert a few "if (xx) BUG();" to BUG_ON(xx)

This is fine

Quoted 8 lines
Quoted 1 line

- remove a few printk()s, as we get a backtrace with BUG_ON() anyway

This is less helpful. Each of the printks explains why the bug triggers. In theory you can work this out from the BUG_ON line number, but *only* if your source files match those of the reporter, which isn't the case in an annoyingly large number of bug reports ... speaking as someone who seems to get to diagnose large numbers of bugs, it makes my life harder.

I fully understand your point.

Regarding arch/parisc/kernel/pci-dma.c:
- each of the pa11_dma_map_* functions where I removed the printks only had one BUG() anyway. Should be easy to find it in backtraces.
- a few of the pa11_dma_map_* functions have printks, the others don't. I just made it consistent.

Regarding arch/parisc/mm/init.c:
- setup_bootmem() is a huge function. I agree that keeping this printk would make sense.

So, would you be OK with the patch if I drop the init.c change and keep the pci-dma.c changes?

Helge

#3 lore raw
In reply to James Bottomley
Quoted 5 lines · On Tuesday 06 January 2009 10:20:33 James Bottomley wrote:
Quoted 1 line · Helge Deller

- convert a few "if (xx) BUG();" to BUG_ON(xx)

This is fine

Quoted 8 lines
Quoted 1 line

- remove a few printk()s, as we get a backtrace with BUG_ON() anyway

This is less helpful. Each of the printks explains why the bug triggers. In theory you can work this out from the BUG_ON line number, but *only* if your source files match those of the reporter, which isn't the case in an annoyingly large number of bug reports ... speaking as someone who seems to get to diagnose large numbers of bugs, it makes my life harder.

i think those comments go beyond parisc. perhaps we should add a BUG_ON_ANNOTATED() that includes another printk ?
-mike