[PATCH 0/16] use WARN

[PATCH 0/16] use WARN

[Julia Lawall]

These patches use WARN, which combines printk and WARN_ON(1), or WARN_ONCE,
which combines printk and WARN_ON_ONCE(1).  This does not appear to affect
the behavior, but makes the code a little more concise.

The semantic patch that makes this transformation is as follows
(http://coccinelle.lip6.fr/).  In particular, it only transforms the case
where the WARN_ON or WARN_ON_ONCE is preceded by a single printk.

// <smpl>
@bad1@
position p;
@@

printk(...);
printk@p(...);
WARN_ON(1);

@ok1@
expression list es;
position p != bad1.p;
@@

-printk@p(
+WARN(1,
  es);
-WARN_ON(1);

@@
expression list ok1.es;
@@

if (...)
- {
  WARN(1,es);
- }

@bad2@
position p;
@@

printk(...);
printk@p(...);
WARN_ON_ONCE(1);

@ok2@
expression list es;
position p != bad2.p;
@@

-printk@p(
+WARN_ONCE(1,
  es);
-WARN_ON_ONCE(1);

@@
expression list ok2.es;
@@

if (...)
- {
  WARN_ONCE(1,es);
- }
// </smpl>

[PATCH 1/16] drivers/gpu/drm/drm_cache.c: use WARN_ONCE

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use WARN_ONCE rather than printk followed by WARN_ON_ONCE(1), for conciseness.

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression list es;
@@

-printk(
+WARN_ONCE(1,
  es);
-WARN_ON_ONCE(1);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/gpu/drm/drm_cache.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c
index a575cb2..8df9a7b 100644
--- a/drivers/gpu/drm/drm_cache.c
+++ b/drivers/gpu/drm/drm_cache.c
@@ -94,8 +94,7 @@ drm_clflush_pages(struct page *pages[], unsigned long num_pages)
 		kunmap_atomic(page_virtual);
 	}
 #else
-	printk(KERN_ERR "Architecture has no drm_cache.c support\n");
-	WARN_ON_ONCE(1);
+	WARN_ONCE(1, KERN_ERR "Architecture has no drm_cache.c support\n");
 #endif
 }
 EXPORT_SYMBOL(drm_clflush_pages);
@@ -119,8 +118,7 @@ drm_clflush_sg(struct sg_table *st)
 	if (on_each_cpu(drm_clflush_ipi_handler, NULL, 1) != 0)
 		printk(KERN_ERR "Timed out waiting for cache flush.\n");
 #else
-	printk(KERN_ERR "Architecture has no drm_cache.c support\n");
-	WARN_ON_ONCE(1);
+	WARN_ONCE(1, KERN_ERR "Architecture has no drm_cache.c support\n");
 #endif
 }
 EXPORT_SYMBOL(drm_clflush_sg);
@@ -142,8 +140,7 @@ drm_clflush_virt_range(char *addr, unsigned long length)
 	if (on_each_cpu(drm_clflush_ipi_handler, NULL, 1) != 0)
 		printk(KERN_ERR "Timed out waiting for cache flush.\n");
 #else
-	printk(KERN_ERR "Architecture has no drm_cache.c support\n");
-	WARN_ON_ONCE(1);
+	WARN_ONCE(1, KERN_ERR "Architecture has no drm_cache.c support\n");
 #endif
 }
 EXPORT_SYMBOL(drm_clflush_virt_range);

[PATCH 2/16] fs/hfsplus/bnode.c: use WARN

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use WARN rather than printk followed by WARN_ON(1), for conciseness.

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression list es;
@@

-printk(
+WARN(1,
  es);
-WARN_ON(1);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 fs/hfsplus/bnode.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c
index 5c125ce..7a92c2c 100644
--- a/fs/hfsplus/bnode.c
+++ b/fs/hfsplus/bnode.c
@@ -588,8 +588,7 @@ struct hfs_bnode *hfs_bnode_create(struct hfs_btree *tree, u32 num)
 	node = hfs_bnode_findhash(tree, num);
 	spin_unlock(&tree->hash_lock);
 	if (node) {
-		printk(KERN_CRIT "new node %u already hashed?\n", num);
-		WARN_ON(1);
+		WARN(1, KERN_CRIT "new node %u already hashed?\n", num);
 		return node;
 	}
 	node = __hfs_bnode_create(tree, num);

Re: [PATCH 2/16] fs/hfsplus/bnode.c: use WARN

[Vyacheslav Dubeyko]

On Nov 3, 2012, at 1:58 PM, Julia Lawall wrote:

> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Use WARN rather than printk followed by WARN_ON(1), for conciseness.
> 
> A simplified version of the semantic patch that makes this transformation
> is as follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression list es;
> @@
> 
> -printk(
> +WARN(1,
>  es);
> -WARN_ON(1);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
> fs/hfsplus/bnode.c |    3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c
> index 5c125ce..7a92c2c 100644
> --- a/fs/hfsplus/bnode.c
> +++ b/fs/hfsplus/bnode.c
> @@ -588,8 +588,7 @@ struct hfs_bnode *hfs_bnode_create(struct hfs_btree *tree, u32 num)
> 	node = hfs_bnode_findhash(tree, num);
> 	spin_unlock(&tree->hash_lock);
> 	if (node) {
> -		printk(KERN_CRIT "new node %u already hashed?\n", num);
> -		WARN_ON(1);
> +		WARN(1, KERN_CRIT "new node %u already hashed?\n", num);
> 		return node;
> 	}
> 	node = __hfs_bnode_create(tree, num);
> 

Looks good.

Reviewed-by: Vyacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Vyacheslav Dubeyko.


> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 3/16] drivers/md/raid5.c: use WARN

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use WARN rather than printk followed by WARN_ON(1), for conciseness.

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression list es;
@@

-printk(
+WARN(1,
  es);
-WARN_ON(1);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/md/raid5.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 320df0c..8c3b9bb 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -373,13 +373,11 @@ static void init_stripe(struct stripe_head *sh, sector_t sector, int previous)
 		struct r5dev *dev = &sh->dev[i];
 
 		if (dev->toread || dev->read || dev->towrite || dev->written ||
-		    test_bit(R5_LOCKED, &dev->flags)) {
-			printk(KERN_ERR "sector=%llx i=%d %p %p %p %p %d\n",
+		    test_bit(R5_LOCKED, &dev->flags))
+			WARN(1, KERN_ERR "sector=%llx i=%d %p %p %p %p %d\n",
 			       (unsigned long long)sh->sector, i, dev->toread,
 			       dev->read, dev->towrite, dev->written,
 			       test_bit(R5_LOCKED, &dev->flags));
-			WARN_ON(1);
-		}
 		dev->flags = 0;
 		raid5_build_block(sh, i, previous);
 	}

[PATCH 4/16] drivers/usb/wusbcore: use WARN

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use WARN rather than printk followed by WARN_ON(1), for conciseness.

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression list es;
@@

-printk(
+WARN(1,
  es);
-WARN_ON(1);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/usb/wusbcore/wa-xfer.c |    3 +--
 drivers/usb/wusbcore/wusbhc.c  |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/wusbcore/wa-xfer.c b/drivers/usb/wusbcore/wa-xfer.c
index 57c01ab..1b80601 100644
--- a/drivers/usb/wusbcore/wa-xfer.c
+++ b/drivers/usb/wusbcore/wa-xfer.c
@@ -1124,9 +1124,8 @@ int wa_urb_dequeue(struct wahc *wa, struct urb *urb)
 		switch (seg->status) {
 		case WA_SEG_NOTREADY:
 		case WA_SEG_READY:
-			printk(KERN_ERR "xfer %p#%u: dequeue bad state %u\n",
+			WARN(1, KERN_ERR "xfer %p#%u: dequeue bad state %u\n",
 			       xfer, cnt, seg->status);
-			WARN_ON(1);
 			break;
 		case WA_SEG_DELAYED:
 			seg->status = WA_SEG_ABORTED;
diff --git a/drivers/usb/wusbcore/wusbhc.c b/drivers/usb/wusbcore/wusbhc.c
index 0faca16..bb5e649 100644
--- a/drivers/usb/wusbcore/wusbhc.c
+++ b/drivers/usb/wusbcore/wusbhc.c
@@ -435,9 +435,8 @@ static void __exit wusbcore_exit(void)
 		char buf[256];
 		bitmap_scnprintf(buf, sizeof(buf), wusb_cluster_id_table,
 				 CLUSTER_IDS);
-		printk(KERN_ERR "BUG: WUSB Cluster IDs not released "
+		WARN(1, KERN_ERR "BUG: WUSB Cluster IDs not released "
 		       "on exit: %s\n", buf);
-		WARN_ON(1);
 	}
 	usb_unregister_notify(&wusb_usb_notifier);
 	destroy_workqueue(wusbd);

[PATCH 5/16] drivers/scsi: use WARN

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use WARN rather than printk followed by WARN_ON(1), for conciseness.

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression list es;
@@

-printk(
+WARN(1,
  es);
-WARN_ON(1);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/scsi/initio.c   |    3 +--
 drivers/scsi/scsi_lib.c |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/initio.c b/drivers/scsi/initio.c
index dd741bc..1572860 100644
--- a/drivers/scsi/initio.c
+++ b/drivers/scsi/initio.c
@@ -2774,8 +2774,7 @@ static void i91uSCBPost(u8 * host_mem, u8 * cblk_mem)
 	host = (struct initio_host *) host_mem;
 	cblk = (struct scsi_ctrl_blk *) cblk_mem;
 	if ((cmnd = cblk->srb) == NULL) {
-		printk(KERN_ERR "i91uSCBPost: SRB pointer is empty\n");
-		WARN_ON(1);
+		WARN(1, KERN_ERR "i91uSCBPost: SRB pointer is empty\n");
 		initio_release_scb(host, cblk);	/* Release SCB for current channel */
 		return;
 	}
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index da36a3a..e5fdcae 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2573,10 +2573,9 @@ void *scsi_kmap_atomic_sg(struct scatterlist *sgl, int sg_count,
 	}
 
 	if (unlikely(i == sg_count)) {
-		printk(KERN_ERR "%s: Bytes in sg: %zu, requested offset %zu, "
+		WARN(1, KERN_ERR "%s: Bytes in sg: %zu, requested offset %zu, "
 			"elements %d\n",
 		       __func__, sg_len, *offset, sg_count);
-		WARN_ON(1);
 		return NULL;
 	}
 

[PATCH 6/16] drivers/infiniband/hw/cxgb4/cm.c: use WARN

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use WARN rather than printk followed by WARN_ON(1), for conciseness.

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression list es;
@@

-printk(
+WARN(1,
  es);
-WARN_ON(1);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/infiniband/hw/cxgb4/cm.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c
index 6cfd4d8..ed048b9 100644
--- a/drivers/infiniband/hw/cxgb4/cm.c
+++ b/drivers/infiniband/hw/cxgb4/cm.c
@@ -151,9 +151,8 @@ static void stop_ep_timer(struct c4iw_ep *ep)
 {
 	PDBG("%s ep %p\n", __func__, ep);
 	if (!timer_pending(&ep->timer)) {
-		printk(KERN_ERR "%s timer stopped when its not running! "
+		WARN(1, KERN_ERR "%s timer stopped when its not running! "
 		       "ep %p state %u\n", __func__, ep, ep->com.state);
-		WARN_ON(1);
 		return;
 	}
 	del_timer_sync(&ep->timer);
@@ -2551,9 +2550,8 @@ static void process_timeout(struct c4iw_ep *ep)
 		__state_set(&ep->com, ABORTING);
 		break;
 	default:
-		printk(KERN_ERR "%s unexpected state ep %p tid %u state %u\n",
+		WARN(1, KERN_ERR "%s unexpected state ep %p tid %u state %u\n",
 			__func__, ep, ep->hwtid, ep->com.state);
-		WARN_ON(1);
 		abort = 0;
 	}
 	mutex_unlock(&ep->com.mutex);

Re: [PATCH 6/16] drivers/infiniband/hw/cxgb4/cm.c: use WARN

[Steve Wise]

Acked-by: Steve Wise <swise@opengridcomputing.com>

[PATCH 7/16] drivers/scsi/gdth.c: use WARN

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use WARN rather than printk followed by WARN_ON(1), for conciseness.

If (count) is also merged into WARN, for further conciseness.

A simplified version of the semantic patch that makes part of this
transformation is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression list es;
@@

-printk(
+WARN(1,
  es);
-WARN_ON(1);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/scsi/gdth.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index 5d72274..0dbcb27 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -2318,11 +2318,10 @@ static void gdth_copy_internal_data(gdth_ha_str *ha, Scsi_Cmnd *scp,
                 break;
             buffer += cpnow;
         }
-    } else if (count) {
-        printk("GDT-HA %d: SCSI command with no buffers but data transfer expected!\n",
-               ha->hanum);
-        WARN_ON(1);
     }
+	else
+		WARN(count, "GDT-HA %d: SCSI command with no buffers but data transfer expected!\n",
+			     ha->hanum);
 }
 
 static int gdth_internal_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp)

[PATCH 8/16] drivers/infiniband/hw/cxgb3/iwch_cm.c: use WARN

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use WARN rather than printk followed by WARN_ON(1), for conciseness.

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression list es;
@@

-printk(
+WARN(1,
  es);
-WARN_ON(1);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/infiniband/hw/cxgb3/iwch_cm.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/cxgb3/iwch_cm.c b/drivers/infiniband/hw/cxgb3/iwch_cm.c
index aaf88ef..8baaf0d 100644
--- a/drivers/infiniband/hw/cxgb3/iwch_cm.c
+++ b/drivers/infiniband/hw/cxgb3/iwch_cm.c
@@ -128,9 +128,8 @@ static void stop_ep_timer(struct iwch_ep *ep)
 {
 	PDBG("%s ep %p\n", __func__, ep);
 	if (!timer_pending(&ep->timer)) {
-		printk(KERN_ERR "%s timer stopped when its not running!  ep %p state %u\n",
+		WARN(1, KERN_ERR "%s timer stopped when its not running!  ep %p state %u\n",
 			__func__, ep, ep->com.state);
-		WARN_ON(1);
 		return;
 	}
 	del_timer_sync(&ep->timer);
@@ -1756,9 +1755,8 @@ static void ep_timeout(unsigned long arg)
 		__state_set(&ep->com, ABORTING);
 		break;
 	default:
-		printk(KERN_ERR "%s unexpected state ep %p state %u\n",
+		WARN(1, KERN_ERR "%s unexpected state ep %p state %u\n",
 			__func__, ep, ep->com.state);
-		WARN_ON(1);
 		abort = 0;
 	}
 	spin_unlock_irqrestore(&ep->com.lock, flags);

Re: [PATCH 8/16] drivers/infiniband/hw/cxgb3/iwch_cm.c: use WARN

[Steve Wise]

Acked-by: Steve Wise <swise@opengridcomputing.com>

[PATCH 9/16] fs/ext4/indirect.c: use WARN

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use WARN rather than printk followed by WARN_ON(1), for conciseness.

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression list es;
@@

-printk(
+WARN(1,
  es);
-WARN_ON(1);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 fs/ext4/indirect.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
index 792e388..0cdd20f 100644
--- a/fs/ext4/indirect.c
+++ b/fs/ext4/indirect.c
@@ -354,9 +354,8 @@ static int ext4_alloc_blocks(handle_t *handle, struct inode *inode,
 			 * for the first direct block
 			 */
 			new_blocks[index] = current_block;
-			printk(KERN_INFO "%s returned more blocks than "
+			WARN(1, KERN_INFO "%s returned more blocks than "
 						"requested\n", __func__);
-			WARN_ON(1);
 			break;
 		}
 	}

Re: [PATCH 9/16] fs/ext4/indirect.c: use WARN

[Theodore Ts'o]

On Sat, Nov 03, 2012 at 11:58:30AM +0100, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Use WARN rather than printk followed by WARN_ON(1), for conciseness.
> 
> A simplified version of the semantic patch that makes this transformation
> is as follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression list es;
> @@
> 
> -printk(
> +WARN(1,
>   es);
> -WARN_ON(1);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Thanks, applied.

					- Ted

[PATCH 10/16] drivers/net/ethernet/ibm/emac/mal.c: use WARN

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use WARN rather than printk followed by WARN_ON(1), for conciseness.

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression list es;
@@

-printk(
+WARN(1,
  es);
-WARN_ON(1);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/net/ethernet/ibm/emac/mal.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/mal.c b/drivers/net/ethernet/ibm/emac/mal.c
index 479e43e..84c6b6c 100644
--- a/drivers/net/ethernet/ibm/emac/mal.c
+++ b/drivers/net/ethernet/ibm/emac/mal.c
@@ -738,13 +738,11 @@ static int __devexit mal_remove(struct platform_device *ofdev)
 	/* Synchronize with scheduled polling */
 	napi_disable(&mal->napi);
 
-	if (!list_empty(&mal->list)) {
+	if (!list_empty(&mal->list))
 		/* This is *very* bad */
-		printk(KERN_EMERG
+		WARN(1, KERN_EMERG
 		       "mal%d: commac list is not empty on remove!\n",
 		       mal->index);
-		WARN_ON(1);
-	}
 
 	dev_set_drvdata(&ofdev->dev, NULL);
 

Re: [PATCH 10/16] drivers/net/ethernet/ibm/emac/mal.c: use WARN

[walter harms]

Am 03.11.2012 11:58, schrieb Julia Lawall:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Use WARN rather than printk followed by WARN_ON(1), for conciseness.
> 
> A simplified version of the semantic patch that makes this transformation
> is as follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression list es;
> @@
> 
> -printk(
> +WARN(1,
>   es);
> -WARN_ON(1);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
>  drivers/net/ethernet/ibm/emac/mal.c |    6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/emac/mal.c b/drivers/net/ethernet/ibm/emac/mal.c
> index 479e43e..84c6b6c 100644
> --- a/drivers/net/ethernet/ibm/emac/mal.c
> +++ b/drivers/net/ethernet/ibm/emac/mal.c
> @@ -738,13 +738,11 @@ static int __devexit mal_remove(struct platform_device *ofdev)
>  	/* Synchronize with scheduled polling */
>  	napi_disable(&mal->napi);
>  
> -	if (!list_empty(&mal->list)) {
> +	if (!list_empty(&mal->list))
>  		/* This is *very* bad */
> -		printk(KERN_EMERG
> +		WARN(1, KERN_EMERG
>  		       "mal%d: commac list is not empty on remove!\n",
>  		       mal->index);
> -		WARN_ON(1);
> -	}
>  
>  	dev_set_drvdata(&ofdev->dev, NULL);
>  
> 

Hi Julia,
you are removing the {} behin the if. I prefer to be a bit conservative
about {}. There is suggest to keep them because WARN may be expanded in
future (with a second line) and that will cause subtle changes that do
no break the code. (Yes i know it is possible to write macros that
contain savely more than one line.)

re,
 wh

Re: [PATCH 10/16] drivers/net/ethernet/ibm/emac/mal.c: use WARN

[Julia Lawall]

On Sat, 3 Nov 2012, walter harms wrote:

>
>
> Am 03.11.2012 11:58, schrieb Julia Lawall:
>> From: Julia Lawall <Julia.Lawall@lip6.fr>
>>
>> Use WARN rather than printk followed by WARN_ON(1), for conciseness.
>>
>> A simplified version of the semantic patch that makes this transformation
>> is as follows: (http://coccinelle.lip6.fr/)
>>
>> // <smpl>
>> @@
>> expression list es;
>> @@
>>
>> -printk(
>> +WARN(1,
>>   es);
>> -WARN_ON(1);
>> // </smpl>
>>
>> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>>
>> ---
>>  drivers/net/ethernet/ibm/emac/mal.c |    6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ibm/emac/mal.c b/drivers/net/ethernet/ibm/emac/mal.c
>> index 479e43e..84c6b6c 100644
>> --- a/drivers/net/ethernet/ibm/emac/mal.c
>> +++ b/drivers/net/ethernet/ibm/emac/mal.c
>> @@ -738,13 +738,11 @@ static int __devexit mal_remove(struct platform_device *ofdev)
>>  	/* Synchronize with scheduled polling */
>>  	napi_disable(&mal->napi);
>>
>> -	if (!list_empty(&mal->list)) {
>> +	if (!list_empty(&mal->list))
>>  		/* This is *very* bad */
>> -		printk(KERN_EMERG
>> +		WARN(1, KERN_EMERG
>>  		       "mal%d: commac list is not empty on remove!\n",
>>  		       mal->index);
>> -		WARN_ON(1);
>> -	}
>>
>>  	dev_set_drvdata(&ofdev->dev, NULL);
>>
>>
>
> Hi Julia,
> you are removing the {} behin the if. I prefer to be a bit conservative
> about {}. There is suggest to keep them because WARN may be expanded in
> future (with a second line) and that will cause subtle changes that do
> no break the code. (Yes i know it is possible to write macros that
> contain savely more than one line.)

WARN is already multi-line, surrounded by ({ }).  It seems to be set up to 
be used as an expression.  Is it necessary to assume that it might someday 
be changed from safe to unsafe?

julia

Re: [PATCH 10/16] drivers/net/ethernet/ibm/emac/mal.c: use WARN

[walter harms]

Am 03.11.2012 15:14, schrieb Julia Lawall:
> On Sat, 3 Nov 2012, walter harms wrote:
> 
>>
>>
>> Am 03.11.2012 11:58, schrieb Julia Lawall:
>>> From: Julia Lawall <Julia.Lawall@lip6.fr>
>>>
>>> Use WARN rather than printk followed by WARN_ON(1), for conciseness.
>>>
>>> A simplified version of the semantic patch that makes this
>>> transformation
>>> is as follows: (http://coccinelle.lip6.fr/)
>>>
>>> // <smpl>
>>> @@
>>> expression list es;
>>> @@
>>>
>>> -printk(
>>> +WARN(1,
>>>   es);
>>> -WARN_ON(1);
>>> // </smpl>
>>>
>>> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>>>
>>> ---
>>>  drivers/net/ethernet/ibm/emac/mal.c |    6 ++----
>>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/ibm/emac/mal.c
>>> b/drivers/net/ethernet/ibm/emac/mal.c
>>> index 479e43e..84c6b6c 100644
>>> --- a/drivers/net/ethernet/ibm/emac/mal.c
>>> +++ b/drivers/net/ethernet/ibm/emac/mal.c
>>> @@ -738,13 +738,11 @@ static int __devexit mal_remove(struct
>>> platform_device *ofdev)
>>>      /* Synchronize with scheduled polling */
>>>      napi_disable(&mal->napi);
>>>
>>> -    if (!list_empty(&mal->list)) {
>>> +    if (!list_empty(&mal->list))
>>>          /* This is *very* bad */
>>> -        printk(KERN_EMERG
>>> +        WARN(1, KERN_EMERG
>>>                 "mal%d: commac list is not empty on remove!\n",
>>>                 mal->index);
>>> -        WARN_ON(1);
>>> -    }
>>>
>>>      dev_set_drvdata(&ofdev->dev, NULL);
>>>
>>>
>>
>> Hi Julia,
>> you are removing the {} behin the if. I prefer to be a bit conservative
>> about {}. There is suggest to keep them because WARN may be expanded in
>> future (with a second line) and that will cause subtle changes that do
>> no break the code. (Yes i know it is possible to write macros that
>> contain savely more than one line.)
> 
> WARN is already multi-line, surrounded by ({ }).  It seems to be set up
> to be used as an expression.  Is it necessary to assume that it might
> someday be changed from safe to unsafe?
> 

my bad,
NTL looks like a candidate for a function.

While looking i have noticed that a lot of drivers define there private "assert" macro.
It is very similar to warn.

(e.g.)
 #define RTL819x_DEBUG
 #ifdef RTL819x_DEBUG
 #define assert(expr) \
        if (!(expr)) {                                  \
                 printk( "Assertion failed! %s,%s,%s,line=%d\n", \
                #expr,__FILE__,__FUNCTION__,__LINE__);          \
        }

re,
 wh

Re: [PATCH 10/16] drivers/net/ethernet/ibm/emac/mal.c: use WARN

[Julia Lawall]

> While looking i have noticed that a lot of drivers define there private "assert" macro.
> It is very similar to warn.
>
> (e.g.)
> #define RTL819x_DEBUG
> #ifdef RTL819x_DEBUG
> #define assert(expr) \
>        if (!(expr)) {                                  \
>                 printk( "Assertion failed! %s,%s,%s,line=%d\n", \
>                #expr,__FILE__,__FUNCTION__,__LINE__);          \
>        }

WARN is more complicated.  At least with the right debugging options 
turned on, it dumps the stack, via warn_slowpath_common.

julia

Re: [PATCH 10/16] drivers/net/ethernet/ibm/emac/mal.c: use WARN

[David Miller]

From: Julia Lawall <Julia.Lawall@lip6.fr>
Date: Sat,  3 Nov 2012 11:58:31 +0100

> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Use WARN rather than printk followed by WARN_ON(1), for conciseness.
> 
> A simplified version of the semantic patch that makes this transformation
> is as follows: (http://coccinelle.lip6.fr/)
 ...
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Applied.

[PATCH 11/16] drivers/misc/kgdbts.c: use WARN

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use WARN rather than printk followed by WARN_ON(1), for conciseness.

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression list es;
@@

-printk(
+WARN(1,
  es);
-WARN_ON(1);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/misc/kgdbts.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c
index 3aa9a96..8b367db 100644
--- a/drivers/misc/kgdbts.c
+++ b/drivers/misc/kgdbts.c
@@ -114,9 +114,8 @@
 		touch_nmi_watchdog();	\
 	} while (0)
 #define eprintk(a...) do { \
-		printk(KERN_ERR a); \
-		WARN_ON(1); \
-	} while (0)
+		WARN(1, KERN_ERR a); \
+		} while (0)
 #define MAX_CONFIG_LEN		40
 
 static struct kgdb_io kgdbts_io_ops;

Re: [PATCH 11/16] drivers/misc/kgdbts.c: use WARN

[walter harms]

Am 03.11.2012 11:58, schrieb Julia Lawall:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Use WARN rather than printk followed by WARN_ON(1), for conciseness.
> 
> A simplified version of the semantic patch that makes this transformation
> is as follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression list es;
> @@
> 
> -printk(
> +WARN(1,
>   es);
> -WARN_ON(1);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
>  drivers/misc/kgdbts.c |    5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c
> index 3aa9a96..8b367db 100644
> --- a/drivers/misc/kgdbts.c
> +++ b/drivers/misc/kgdbts.c
> @@ -114,9 +114,8 @@
>  		touch_nmi_watchdog();	\
>  	} while (0)
>  #define eprintk(a...) do { \
> -		printk(KERN_ERR a); \
> -		WARN_ON(1); \
> -	} while (0)
> +		WARN(1, KERN_ERR a); \
> +		} while (0)
>  #define MAX_CONFIG_LEN		40
>  

A macro calling a macro ?
Is it possible to replace eprintk() ?

re,
 wh

[PATCH] drivers/misc/kgdbts.c: remove eprintk

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

eprintk is really just WARN(1, KERN_ERR ...).  Use WARN to be more
consistent with the rest of the code.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
  drivers/misc/kgdbts.c |   38 +++++++++++++++++---------------------
  1 file changed, 17 insertions(+), 21 deletions(-)

diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c
index 3aa9a96..433993b 100644
--- a/drivers/misc/kgdbts.c
+++ b/drivers/misc/kgdbts.c
@@ -113,10 +113,6 @@
  		printk(KERN_INFO a); \
  		touch_nmi_watchdog();	\
  	} while (0)
-#define eprintk(a...) do { \
-		printk(KERN_ERR a); \
-		WARN_ON(1); \
-	} while (0)
  #define MAX_CONFIG_LEN		40

  static struct kgdb_io kgdbts_io_ops;
@@ -323,7 +319,7 @@ static int check_and_rewind_pc(char *put_str, char *arg)
  		v2printk("Emul: rewind hit single step bp\n");
  		restart_from_top_after_write = 1;
  	} else if (strcmp(arg, "silent") && ip + offset != addr) {
-		eprintk("kgdbts: BP mismatch %lx expected %lx\n",
+		WARN(1, KERN_ERR "kgdbts: BP mismatch %lx expected %lx\n",
  			   ip + offset, addr);
  		return 1;
  	}
@@ -374,7 +370,7 @@ static int check_single_step(char *put_str, char *arg)
  continue_test:
  	matched_id = 0;
  	if (instruction_pointer(&kgdbts_regs) == addr) {
-		eprintk("kgdbts: SingleStep failed at %lx\n",
+		WARN(1, KERN_ERR "kgdbts: SingleStep failed at %lx\n",
  			   instruction_pointer(&kgdbts_regs));
  		return 1;
  	}
@@ -469,7 +465,7 @@ static void emul_sstep_get(char *arg)
  		break_helper("z0", NULL, sstep_addr);
  		break;
  	default:
-		eprintk("kgdbts: ERROR failed sstep get emulation\n");
+		WARN(1, KERN_ERR "kgdbts: ERROR failed sstep get emulation\n");
  	}
  	sstep_state++;
  }
@@ -496,13 +492,13 @@ static int emul_sstep_put(char *put_str, char *arg)
  		break;
  	case 2:
  		if (strncmp(put_str, "$OK", 3)) {
-			eprintk("kgdbts: failed sstep break set\n");
+			WARN(1, KERN_ERR "kgdbts: failed sstep break set\n");
  			return 1;
  		}
  		break;
  	case 3:
  		if (strncmp(put_str, "$T0", 3)) {
-			eprintk("kgdbts: failed continue sstep\n");
+			WARN(1, KERN_ERR "kgdbts: failed continue sstep\n");
  			return 1;
  		} else {
  			char *ptr = &put_str[11];
@@ -511,14 +507,14 @@ static int emul_sstep_put(char *put_str, char *arg)
  		break;
  	case 4:
  		if (strncmp(put_str, "$OK", 3)) {
-			eprintk("kgdbts: failed sstep break unset\n");
+			WARN(1, KERN_ERR "kgdbts: failed sstep break unset\n");
  			return 1;
  		}
  		/* Single step is complete so continue on! */
  		sstep_state = 0;
  		return 0;
  	default:
-		eprintk("kgdbts: ERROR failed sstep put emulation\n");
+		WARN(1, KERN_ERR "kgdbts: ERROR failed sstep put emulation\n");
  	}

  	/* Continue on the same test line until emulation is complete */
@@ -763,7 +759,7 @@ static int run_simple_test(int is_get_char, int chr)
  		}

  		if (get_buf[get_buf_cnt] == '\0') {
-			eprintk("kgdbts: ERROR GET: EOB on '%s' at %i\n",
+			WARN(1, KERN_ERR "kgdbts: ERROR GET: EOB on '%s' at %i\n",
  			   ts.name, ts.idx);
  			get_buf_cnt = 0;
  			fill_get_buf("D");
@@ -778,13 +774,13 @@ static int run_simple_test(int is_get_char, int chr)
  	 */
  	if (ts.tst[ts.idx].get[0] == '\0' && ts.tst[ts.idx].put[0] == '\0' &&
  	    !ts.tst[ts.idx].get_handler) {
-		eprintk("kgdbts: ERROR: beyond end of test on"
+		WARN(1, KERN_ERR "kgdbts: ERROR: beyond end of test on"
  			   " '%s' line %i\n", ts.name, ts.idx);
  		return 0;
  	}

  	if (put_buf_cnt >= BUFMAX) {
-		eprintk("kgdbts: ERROR: put buffer overflow on"
+		WARN(1, KERN_ERR "kgdbts: ERROR: put buffer overflow on"
  			   " '%s' line %i\n", ts.name, ts.idx);
  		put_buf_cnt = 0;
  		return 0;
@@ -799,7 +795,7 @@ static int run_simple_test(int is_get_char, int chr)
  	/* End of packet == #XX so look for the '#' */
  	if (put_buf_cnt > 3 && put_buf[put_buf_cnt - 3] == '#') {
  		if (put_buf_cnt >= BUFMAX) {
-			eprintk("kgdbts: ERROR: put buffer overflow on"
+			WARN(1, KERN_ERR "kgdbts: ERROR: put buffer overflow on"
  				" '%s' line %i\n", ts.name, ts.idx);
  			put_buf_cnt = 0;
  			return 0;
@@ -808,7 +804,7 @@ static int run_simple_test(int is_get_char, int chr)
  		v2printk("put%i: %s\n", ts.idx, put_buf);
  		/* Trigger check here */
  		if (ts.validate_put && ts.validate_put(put_buf)) {
-			eprintk("kgdbts: ERROR PUT: end of test "
+			WARN(1, KERN_ERR "kgdbts: ERROR PUT: end of test "
  			   "buffer on '%s' line %i expected %s got %s\n",
  			   ts.name, ts.idx, ts.tst[ts.idx].put, put_buf);
  		}
@@ -872,7 +868,7 @@ static void run_breakpoint_test(int is_hw_breakpoint)
  	if (test_complete)
  		return;

-	eprintk("kgdbts: ERROR %s test failed\n", ts.name);
+	WARN(1, KERN_ERR "kgdbts: ERROR %s test failed\n", ts.name);
  	if (is_hw_breakpoint)
  		hwbreaks_ok = 0;
  }
@@ -893,7 +889,7 @@ static void run_hw_break_test(int is_write_test)
  	hw_break_val_access();
  	if (is_write_test) {
  		if (test_complete == 2) {
-			eprintk("kgdbts: ERROR %s broke on access\n",
+			WARN(1, KERN_ERR "kgdbts: ERROR %s broke on access\n",
  				ts.name);
  			hwbreaks_ok = 0;
  		}
@@ -904,7 +900,7 @@ static void run_hw_break_test(int is_write_test)
  	if (test_complete == 1)
  		return;

-	eprintk("kgdbts: ERROR %s test failed\n", ts.name);
+	WARN(1, KERN_ERR "kgdbts: ERROR %s test failed\n", ts.name);
  	hwbreaks_ok = 0;
  }

@@ -922,12 +918,12 @@ static void run_nmi_sleep_test(int nmi_sleep)
  	touch_nmi_watchdog();
  	local_irq_restore(flags);
  	if (test_complete != 2)
-		eprintk("kgdbts: ERROR nmi_test did not hit nmi\n");
+		WARN(1, KERN_ERR "kgdbts: ERROR nmi_test did not hit nmi\n");
  	kgdb_breakpoint();
  	if (test_complete == 1)
  		return;

-	eprintk("kgdbts: ERROR %s test failed\n", ts.name);
+	WARN(1, KERN_ERR "kgdbts: ERROR %s test failed\n", ts.name);
  }

  static void run_bad_read_test(void)

Re: [PATCH] drivers/misc/kgdbts.c: remove eprintk

[Arnd Bergmann]

On Saturday 03 November 2012, Julia Lawall wrote:
> @@ -113,10 +113,6 @@
>                 printk(KERN_INFO a); \
>                 touch_nmi_watchdog();   \
>         } while (0)
> -#define eprintk(a...) do { \
> -               printk(KERN_ERR a); \
> -               WARN_ON(1); \
> -       } while (0)
>   #define MAX_CONFIG_LEN                40
> 
>   static struct kgdb_io kgdbts_io_ops;
> @@ -323,7 +319,7 @@ static int check_and_rewind_pc(char *put_str, char *arg)
>                 v2printk("Emul: rewind hit single step bp\n");
>                 restart_from_top_after_write = 1;
>         } else if (strcmp(arg, "silent") && ip + offset != addr) {
> -               eprintk("kgdbts: BP mismatch %lx expected %lx\n",
> +               WARN(1, KERN_ERR "kgdbts: BP mismatch %lx expected %lx\n",
>                            ip + offset, addr);
>                 return 1;
>         }

Hmm, I did not think that WARN() took a KERN_ERR argument, which should
really be implied here. Looking at the code, it really seems to be required
at the moment, but only 5 out of 117 callers use it this way.

Any idea what is going on here?

	Arnd

Re: [PATCH] drivers/misc/kgdbts.c: remove eprintk

[Julia Lawall]

On Sun, 4 Nov 2012, Arnd Bergmann wrote:

> On Saturday 03 November 2012, Julia Lawall wrote:
>> @@ -113,10 +113,6 @@
>>                 printk(KERN_INFO a); \
>>                 touch_nmi_watchdog();   \
>>         } while (0)
>> -#define eprintk(a...) do { \
>> -               printk(KERN_ERR a); \
>> -               WARN_ON(1); \
>> -       } while (0)
>>   #define MAX_CONFIG_LEN                40
>>
>>   static struct kgdb_io kgdbts_io_ops;
>> @@ -323,7 +319,7 @@ static int check_and_rewind_pc(char *put_str, char *arg)
>>                 v2printk("Emul: rewind hit single step bp\n");
>>                 restart_from_top_after_write = 1;
>>         } else if (strcmp(arg, "silent") && ip + offset != addr) {
>> -               eprintk("kgdbts: BP mismatch %lx expected %lx\n",
>> +               WARN(1, KERN_ERR "kgdbts: BP mismatch %lx expected %lx\n",
>>                            ip + offset, addr);
>>                 return 1;
>>         }
>
> Hmm, I did not think that WARN() took a KERN_ERR argument, which should
> really be implied here. Looking at the code, it really seems to be required
> at the moment, but only 5 out of 117 callers use it this way.
>
> Any idea what is going on here?

I'm not sure to understand the 5 and 117.  Using grep, I get 30 with 
KERN_ERR, 61 with some KERN thing, and 1207 without KERN.  If things are 
set up such that warn_slowpath_fmt is called, then that function adds
KERN_WARNING.  There is an alternate definition of __WARN_printf that just 
does a printk.

So if eprintk wants KERN_ERR, then it seems that rewriting it with WARN is 
not a good idea.  I will check whether this problems arises with the other 
printks and WARNs that I suggested to merge.

thanks,
julia

Re: [PATCH] drivers/misc/kgdbts.c: remove eprintk

[Arnd Bergmann]

On Sunday 04 November 2012, Julia Lawall wrote:

> > Hmm, I did not think that WARN() took a KERN_ERR argument, which should
> > really be implied here. Looking at the code, it really seems to be required
> > at the moment, but only 5 out of 117 callers use it this way.
> >
> > Any idea what is going on here?
> 
> I'm not sure to understand the 5 and 117.  Using grep, I get 30 with 
> KERN_ERR, 61 with some KERN thing, and 1207 without KERN. 

Right, I was using 'grep -w', which misses a lot of the instances, although
I see still much fewer in the last category.

> If things are 
> set up such that warn_slowpath_fmt is called, then that function adds
> KERN_WARNING.  There is an alternate definition of __WARN_printf that just 
> does a printk.

I don't see yet where that KERN_WARNING gets added. Looking at
warn_slowpath_common, there are two or three lines that get printed at 
KERN_WARNING level, followed by the format that got passed into WARN(),
which may or may not include a printk level, but I don't see one getting
added.


	Arnd

Re: [PATCH] drivers/misc/kgdbts.c: remove eprintk

[Julia Lawall]

On Sun, 4 Nov 2012, Arnd Bergmann wrote:

> On Sunday 04 November 2012, Julia Lawall wrote:
>
>>> Hmm, I did not think that WARN() took a KERN_ERR argument, which should
>>> really be implied here. Looking at the code, it really seems to be required
>>> at the moment, but only 5 out of 117 callers use it this way.
>>>
>>> Any idea what is going on here?
>>
>> I'm not sure to understand the 5 and 117.  Using grep, I get 30 with
>> KERN_ERR, 61 with some KERN thing, and 1207 without KERN.
>
> Right, I was using 'grep -w', which misses a lot of the instances, although
> I see still much fewer in the last category.
>
>> If things are
>> set up such that warn_slowpath_fmt is called, then that function adds
>> KERN_WARNING.  There is an alternate definition of __WARN_printf that just
>> does a printk.
>
> I don't see yet where that KERN_WARNING gets added. Looking at
> warn_slowpath_common, there are two or three lines that get printed at
> KERN_WARNING level, followed by the format that got passed into WARN(),
> which may or may not include a printk level, but I don't see one getting
> added.

OK, I agree.  There are lots of KERN_WARNINGs, but not on the string that 
was passed in.  Still, maybe it is not so good to pass a KERN_XXX for some 
other XXX to WARN.

julia

Re: [PATCH] drivers/misc/kgdbts.c: remove eprintk

[Arnd Bergmann]

On Sunday 04 November 2012, Julia Lawall wrote:
> >
> > I don't see yet where that KERN_WARNING gets added. Looking at
> > warn_slowpath_common, there are two or three lines that get printed at
> > KERN_WARNING level, followed by the format that got passed into WARN(),
> > which may or may not include a printk level, but I don't see one getting
> > added.
> 
> OK, I agree.  There are lots of KERN_WARNINGs, but not on the string that 
> was passed in.  Still, maybe it is not so good to pass a KERN_XXX for some 
> other XXX to WARN.

Given that most users don't pass anything here, and that those that do pass
something are somewhat inconsistent, could we make the messages always get
printed at KERN_WARNING level from WARN(), and kill off the instances
that already pass a level?

	Arnd

Re: [PATCH] drivers/misc/kgdbts.c: remove eprintk

[Julia Lawall]

On Mon, 5 Nov 2012, Arnd Bergmann wrote:

> On Sunday 04 November 2012, Julia Lawall wrote:
> > >
> > > I don't see yet where that KERN_WARNING gets added. Looking at
> > > warn_slowpath_common, there are two or three lines that get printed at
> > > KERN_WARNING level, followed by the format that got passed into WARN(),
> > > which may or may not include a printk level, but I don't see one getting
> > > added.
> >
> > OK, I agree.  There are lots of KERN_WARNINGs, but not on the string that
> > was passed in.  Still, maybe it is not so good to pass a KERN_XXX for some
> > other XXX to WARN.
>
> Given that most users don't pass anything here, and that those that do pass
> something are somewhat inconsistent, could we make the messages always get
> printed at KERN_WARNING level from WARN(), and kill off the instances
> that already pass a level?

OK, I could try proposing that, and if someone doesn't think it is the
right thing to do, they can ignore the patch.

Concretely, KERN_WARNING should be added in the printk called from WARN,
and all KERN information should be removed from the calls?

thanks,
julia

Re: [PATCH] drivers/misc/kgdbts.c: remove eprintk

[Arnd Bergmann]

On Monday 05 November 2012, Julia Lawall wrote:
> OK, I could try proposing that, and if someone doesn't think it is the
> right thing to do, they can ignore the patch.
> 
> Concretely, KERN_WARNING should be added in the printk called from WARN,
> and all KERN information should be removed from the calls?

I think that would be the right solution, yes. Unfortunately, I don't
know how to easily do this since we are already passing down a format
string into vprintk here. Maybe just add the KERN_WARNING at the point
where __WARN_printf gets called.

	Arnd

[PATCH 12/16] fs/logfs/gc.c: use WARN

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use WARN rather than printk followed by WARN_ON(1), for conciseness.

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression list es;
@@

-printk(
+WARN(1,
  es);
-WARN_ON(1);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 fs/logfs/gc.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/logfs/gc.c b/fs/logfs/gc.c
index d4efb06..62dee03 100644
--- a/fs/logfs/gc.c
+++ b/fs/logfs/gc.c
@@ -55,9 +55,8 @@ static u8 root_distance(struct super_block *sb, gc_level_t __gc_level)
 		/* inode file data or indirect blocks */
 		return super->s_ifile_levels - (gc_level - 6);
 	default:
-		printk(KERN_ERR"LOGFS: segment of unknown level %x found\n",
+		WARN(1, KERN_ERR "LOGFS: segment of unknown level %x found\n",
 				gc_level);
-		WARN_ON(1);
 		return super->s_ifile_levels + super->s_iblock_levels;
 	}
 }

[PATCH 13/16] fs/btrfs: use WARN

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use WARN rather than printk followed by WARN_ON(1), for conciseness.

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression list es;
@@

-printk(
+WARN(1,
  es);
-WARN_ON(1);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
Many of these end up with the form
if (somewhat_complex_condition) WARN(1, ...)
They could also be converted to WARN(somewhat_complex_condition, ...)
if that would be preferred.

 fs/btrfs/ctree.c       |   19 +++++++------------
 fs/btrfs/disk-io.c     |    6 ++----
 fs/btrfs/extent-tree.c |    7 +++----
 fs/btrfs/extent_io.c   |    9 +++------
 fs/btrfs/inode.c       |    3 +--
 fs/btrfs/transaction.c |   12 ++++--------
 fs/btrfs/volumes.c     |    3 +--
 7 files changed, 21 insertions(+), 38 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index cdfb4c4..adfa929 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1361,19 +1361,16 @@ noinline int btrfs_cow_block(struct btrfs_trans_handle *trans,
 	u64 search_start;
 	int ret;
 
-	if (trans->transaction != root->fs_info->running_transaction) {
-		printk(KERN_CRIT "trans %llu running %llu\n",
+	if (trans->transaction != root->fs_info->running_transaction)
+		WARN(1, KERN_CRIT "trans %llu running %llu\n",
 		       (unsigned long long)trans->transid,
 		       (unsigned long long)
 		       root->fs_info->running_transaction->transid);
-		WARN_ON(1);
-	}
-	if (trans->transid != root->fs_info->generation) {
-		printk(KERN_CRIT "trans %llu running %llu\n",
+
+	if (trans->transid != root->fs_info->generation)
+		WARN(1, KERN_CRIT "trans %llu running %llu\n",
 		       (unsigned long long)trans->transid,
 		       (unsigned long long)root->fs_info->generation);
-		WARN_ON(1);
-	}
 
 	if (!should_cow_block(trans, root, buf)) {
 		*cow_ret = buf;
@@ -3642,11 +3639,9 @@ static noinline int __push_leaf_left(struct btrfs_trans_handle *trans,
 	btrfs_set_header_nritems(left, old_left_nritems + push_items);
 
 	/* fixup right node */
-	if (push_items > right_nritems) {
-		printk(KERN_CRIT "push items %d nr %u\n", push_items,
+	if (push_items > right_nritems)
+		WARN(1, KERN_CRIT "push items %d nr %u\n", push_items,
 		       right_nritems);
-		WARN_ON(1);
-	}
 
 	if (push_items < right_nritems) {
 		push_space = btrfs_item_offset_nr(right, push_items - 1) -
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 22a0439..1769e7d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3383,14 +3383,12 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
 	int was_dirty;
 
 	btrfs_assert_tree_locked(buf);
-	if (transid != root->fs_info->generation) {
-		printk(KERN_CRIT "btrfs transid mismatch buffer %llu, "
+	if (transid != root->fs_info->generation)
+		WARN(1, KERN_CRIT "btrfs transid mismatch buffer %llu, "
 		       "found %llu running %llu\n",
 			(unsigned long long)buf->start,
 			(unsigned long long)transid,
 			(unsigned long long)root->fs_info->generation);
-		WARN_ON(1);
-	}
 	was_dirty = set_extent_buffer_dirty(buf);
 	if (!was_dirty) {
 		spin_lock(&root->fs_info->delalloc_lock);
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 3d3e2c1..37353eb 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -6292,10 +6292,9 @@ use_block_rsv(struct btrfs_trans_handle *trans,
 		static DEFINE_RATELIMIT_STATE(_rs,
 				DEFAULT_RATELIMIT_INTERVAL,
 				/*DEFAULT_RATELIMIT_BURST*/ 2);
-		if (__ratelimit(&_rs)) {
-			printk(KERN_DEBUG "btrfs: block rsv returned %d\n", ret);
-			WARN_ON(1);
-		}
+		if (__ratelimit(&_rs))
+			WARN(1, KERN_DEBUG "btrfs: block rsv returned %d\n",
+			     ret);
 		ret = reserve_metadata_bytes(root, block_rsv, blocksize, 0);
 		if (!ret) {
 			return block_rsv;
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 472873a..3c062c8 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -341,12 +341,10 @@ static int insert_state(struct extent_io_tree *tree,
 {
 	struct rb_node *node;
 
-	if (end < start) {
-		printk(KERN_ERR "btrfs end < start %llu %llu\n",
+	if (end < start)
+		WARN(1, KERN_ERR "btrfs end < start %llu %llu\n",
 		       (unsigned long long)end,
 		       (unsigned long long)start);
-		WARN_ON(1);
-	}
 	state->start = start;
 	state->end = end;
 
@@ -4721,10 +4719,9 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
 	}
 
 	if (start + min_len > eb->len) {
-		printk(KERN_ERR "btrfs bad mapping eb start %llu len %lu, "
+		WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, "
 		       "wanted %lu %lu\n", (unsigned long long)eb->start,
 		       eb->len, start, min_len);
-		WARN_ON(1);
 		return -EINVAL;
 	}
 
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 95542a1..505357b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5439,8 +5439,7 @@ again:
 				    extent_map_end(em) - 1, NULL, GFP_NOFS);
 		goto insert;
 	} else {
-		printk(KERN_ERR "btrfs unknown found_type %d\n", found_type);
-		WARN_ON(1);
+		WARN(1, KERN_ERR "btrfs unknown found_type %d\n", found_type);
 	}
 not_found:
 	em->start = start;
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 04bbfb1..d57cf89 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -145,16 +145,12 @@ loop:
 	 * the log must never go across transaction boundaries.
 	 */
 	smp_mb();
-	if (!list_empty(&fs_info->tree_mod_seq_list)) {
-		printk(KERN_ERR "btrfs: tree_mod_seq_list not empty when "
+	if (!list_empty(&fs_info->tree_mod_seq_list))
+		WARN(1, KERN_ERR "btrfs: tree_mod_seq_list not empty when "
 			"creating a fresh transaction\n");
-		WARN_ON(1);
-	}
-	if (!RB_EMPTY_ROOT(&fs_info->tree_mod_log)) {
-		printk(KERN_ERR "btrfs: tree_mod_log rb tree not empty when "
+	if (!RB_EMPTY_ROOT(&fs_info->tree_mod_log))
+		WARN(1, KERN_ERR "btrfs: tree_mod_log rb tree not empty when "
 			"creating a fresh transaction\n");
-		WARN_ON(1);
-	}
 	atomic_set(&fs_info->tree_mod_seq, 0);
 
 	spin_lock_init(&cur_trans->commit_lock);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0f5ebb7..2705ce0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3347,9 +3347,8 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 		cur = cur->next;
 
 		if (!device->writeable) {
-			printk(KERN_ERR
+			WARN(1, KERN_ERR
 			       "btrfs: read-only device in alloc_list\n");
-			WARN_ON(1);
 			continue;
 		}
 

Re: [PATCH 13/16] fs/btrfs: use WARN

[David Sterba]

On Sat, Nov 03, 2012 at 11:58:34AM +0100, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Use WARN rather than printk followed by WARN_ON(1), for conciseness.
> 
> A simplified version of the semantic patch that makes this transformation
> is as follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression list es;
> @@
> 
> -printk(
> +WARN(1,
>   es);
> -WARN_ON(1);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
Reviewed-by: David Sterba <dsterba@suse.cz>

[PATCH 14/16] drivers/ssb/main.c: use WARN

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use WARN rather than printk followed by WARN_ON(1), for conciseness.

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression list es;
@@

-printk(
+WARN(1,
  es);
-WARN_ON(1);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/ssb/main.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
index df0f145..519688d 100644
--- a/drivers/ssb/main.c
+++ b/drivers/ssb/main.c
@@ -1118,8 +1118,7 @@ static u32 ssb_tmslow_reject_bitmask(struct ssb_device *dev)
 	case SSB_IDLOW_SSBREV_27:     /* same here */
 		return SSB_TMSLOW_REJECT;	/* this is a guess */
 	default:
-		printk(KERN_INFO "ssb: Backplane Revision 0x%.8X\n", rev);
-		WARN_ON(1);
+		WARN(1, KERN_INFO "ssb: Backplane Revision 0x%.8X\n", rev);
 	}
 	return (SSB_TMSLOW_REJECT | SSB_TMSLOW_REJECT_23);
 }

[PATCH 15/16] drivers/parisc/pdc_stable.c: use WARN

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use WARN rather than printk followed by WARN_ON(1), for conciseness.

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression list es;
@@

-printk(
+WARN(1,
  es);
-WARN_ON(1);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/parisc/pdc_stable.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/parisc/pdc_stable.c b/drivers/parisc/pdc_stable.c
index 246a92f..0f54ab6 100644
--- a/drivers/parisc/pdc_stable.c
+++ b/drivers/parisc/pdc_stable.c
@@ -212,12 +212,10 @@ pdcspath_store(struct pdcspath_entry *entry)
 			entry, devpath, entry->addr);
 
 	/* addr, devpath and count must be word aligned */
-	if (pdc_stable_write(entry->addr, devpath, sizeof(*devpath)) != PDC_OK) {
-		printk(KERN_ERR "%s: an error occurred when writing to PDC.\n"
+	if (pdc_stable_write(entry->addr, devpath, sizeof(*devpath)) != PDC_OK)
+		WARN(1, KERN_ERR "%s: an error occurred when writing to PDC.\n"
 				"It is likely that the Stable Storage data has been corrupted.\n"
 				"Please check it carefully upon next reboot.\n", __func__);
-		WARN_ON(1);
-	}
 		
 	/* kobject is already registered */
 	entry->ready = 2;

[PATCH 16/16] drivers/infiniband/hw/nes: use WARN

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use WARN rather than printk followed by WARN_ON(1), for conciseness.

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression list es;
@@

-printk(
+WARN(1,
  es);
-WARN_ON(1);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/infiniband/hw/nes/nes_cm.c  |    6 ++----
 drivers/infiniband/hw/nes/nes_mgt.c |    6 ++----
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/hw/nes/nes_cm.c b/drivers/infiniband/hw/nes/nes_cm.c
index cfaacaf..8a15a1d 100644
--- a/drivers/infiniband/hw/nes/nes_cm.c
+++ b/drivers/infiniband/hw/nes/nes_cm.c
@@ -629,11 +629,9 @@ static void build_rdma0_msg(struct nes_cm_node *cm_node, struct nes_qp **nesqp_a
 
 	case SEND_RDMA_READ_ZERO:
 	default:
-		if (cm_node->send_rdma0_op != SEND_RDMA_READ_ZERO) {
-			printk(KERN_ERR "%s[%u]: Unsupported RDMA0 len operation=%u\n",
+		if (cm_node->send_rdma0_op != SEND_RDMA_READ_ZERO)
+			WARN(1, KERN_ERR "%s[%u]: Unsupported RDMA0 len operation=%u\n",
 				 __func__, __LINE__, cm_node->send_rdma0_op);
-			WARN_ON(1);
-		}
 		nes_debug(NES_DBG_CM, "Sending first rdma operation.\n");
 		wqe->wqe_words[NES_IWARP_SQ_WQE_MISC_IDX] =
 			cpu_to_le32(NES_IWARP_SQ_OP_RDMAR);
diff --git a/drivers/infiniband/hw/nes/nes_mgt.c b/drivers/infiniband/hw/nes/nes_mgt.c
index 3ba7be3..53bb88c 100644
--- a/drivers/infiniband/hw/nes/nes_mgt.c
+++ b/drivers/infiniband/hw/nes/nes_mgt.c
@@ -649,11 +649,9 @@ static void nes_chg_qh_handler(struct nes_device *nesdev, struct nes_cqp_request
 	nesqp = qh_chg->nesqp;
 
 	/* Should we handle the bad completion */
-	if (cqp_request->major_code) {
-		printk(KERN_ERR PFX "Invalid cqp_request major_code=0x%x\n",
+	if (cqp_request->major_code)
+		WARN(1, KERN_ERR PFX "Invalid cqp_request major_code=0x%x\n",
 		       cqp_request->major_code);
-		WARN_ON(1);
-	}
 
 	switch (nesqp->pau_state) {
 	case PAU_DEL_QH: