LinuxLists.cc - [PATCH 0/16] use WARN

2012-11-03 10:58:47

Subject: [PATCH 0/16] use WARN

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>

2012-11-03 10:58:52

by Julia Lawall

[permalink] [raw]

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

2012-11-03 10:59:04

by Julia Lawall

[permalink] [raw]

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

2012-11-03 10:59:01

by Julia Lawall

[permalink] [raw]

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

2012-11-03 10:58:59

by Julia Lawall

[permalink] [raw]

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

2012-11-03 10:58:57

by Julia Lawall

[permalink] [raw]

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

2012-11-03 10:58:56

by Julia Lawall

[permalink] [raw]

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

2012-11-03 11:00:00

by Julia Lawall

[permalink] [raw]

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

2012-11-03 11:00:44

by Julia Lawall

[permalink] [raw]

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

2012-11-03 11:01:03

by Julia Lawall

[permalink] [raw]

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

2012-11-03 11:01:32

by Julia Lawall

[permalink] [raw]

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

2012-11-03 11:01:30

by Julia Lawall

[permalink] [raw]

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

From: Julia Lawall <[email protected]>

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 <[email protected]>

---
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;
}

2012-11-03 11:02:25

by Julia Lawall

[permalink] [raw]

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

From: Julia Lawall <[email protected]>

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 <[email protected]>

---
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)

2012-11-03 11:02:23

by Julia Lawall

[permalink] [raw]

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

2012-11-03 11:02:59

by Julia Lawall

[permalink] [raw]

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

2012-11-03 11:03:22

by Julia Lawall

[permalink] [raw]

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

2012-11-03 11:03:42

by Julia Lawall

[permalink] [raw]

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

From: Julia Lawall <[email protected]>

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 <[email protected]>

---
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);

2012-11-03 11:30:33

by walter harms

[permalink] [raw]

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

Am 03.11.2012 11:58, schrieb Julia Lawall:
> From: Julia Lawall <[email protected]>
>
> 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 <[email protected]>
>
> ---
> 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

2012-11-03 12:11:41

by walter harms

[permalink] [raw]

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

2012-11-03 13:10:55

by Steve Wise

[permalink] [raw]

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

Acked-by: Steve Wise <[email protected]>

2012-11-03 13:10:54

by Steve Wise

[permalink] [raw]

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

Acked-by: Steve Wise <[email protected]>

2012-11-03 14:14:56

by Julia Lawall

[permalink] [raw]

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

On Sat, 3 Nov 2012, walter harms wrote:

>
>
> Am 03.11.2012 11:58, schrieb Julia Lawall:
>> From: Julia Lawall <[email protected]>
>>
>> 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 <[email protected]>
>>
>> ---
>> 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

2012-11-03 14:27:04

by Julia Lawall

[permalink] [raw]

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

From: Julia Lawall <[email protected]>

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 <[email protected]>

---
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)

2012-11-03 16:26:44

by walter harms

[permalink] [raw]

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

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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>>
>>> ---
>>> 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

2012-11-03 16:35:13

by Julia Lawall

[permalink] [raw]

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

> 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

2012-11-03 19:43:37

by David Miller

[permalink] [raw]

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

From: Julia Lawall <[email protected]>
Date: Sat, 3 Nov 2012 11:58:31 +0100

> From: Julia Lawall <[email protected]>
>
> 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 <[email protected]>

Applied.

2012-11-04 10:49:04

by Viacheslav Dubeyko

[permalink] [raw]

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

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

> From: Julia Lawall <[email protected]>
>
> 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 <[email protected]>
>
> ---
> 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 <[email protected]>

Thanks,
Vyacheslav Dubeyko.

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

2012-11-04 19:40:15

by Arnd Bergmann

[permalink] [raw]

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

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

2012-11-04 19:58:24

by Julia Lawall

[permalink] [raw]

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

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

2012-11-04 20:52:06

by Arnd Bergmann

[permalink] [raw]

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

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

2012-11-04 21:04:07

by Julia Lawall

[permalink] [raw]

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

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

2012-11-05 15:38:58

by David Sterba

[permalink] [raw]

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

On Sat, Nov 03, 2012 at 11:58:34AM +0100, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: David Sterba <[email protected]>

2012-11-05 16:26:46

by Arnd Bergmann

[permalink] [raw]

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

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

2012-11-05 16:57:56

by Julia Lawall

[permalink] [raw]

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

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

2012-11-05 20:02:09

by Arnd Bergmann

[permalink] [raw]

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

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