Kconfig: cleanup s390 v2.

Greetings,
I've added the results of the review to the Kconfig cleanup patches for s390. Patch #2 has been split, one half has all the HAS_IOMEM depends lines the other the remaining !S390 depends lines.

Andrew: I plan to add patches 1-5 to the for-andrew branch of the git390 repository if that is fine with you. The only thing that will be missing in the tree is the patch that disables wireless for s390. The code does compile but without hardware it is mute to have the config options. I'll wait until the git-wireless.patch is upstream. Patches 7-9 depend on patches found in -mm.

blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.

They all look good to me now

umm, OK. If it's Ok I think I'll duck it for now: -mm is full.

Over-full, really: I've been working basically continuously since Friday getting the current dungpile to compile and boot, and it's still miles away from that.

I understand. I'll wait until -mm is a little bit smaller again. It is just that someday I want to finish with the Kconfig cleanup, it has been sitting on my harddriver for ages now.

blue skies,              IBM Deutschland Entwicklung GmbH
   Martin                Vorsitzender des Aufsichtsrats: Johann Weihen
                         Geschäftsführung: Herbert Kircher
Martin Schwidefsky       Sitz der Gesellschaft: Böblingen
Linux on zSeries         Registergericht: Amtsgericht Stuttgart,
   Development           HRB 243294

"Reality continues to ruin my life." - Calvin.

and I continue to be concerned about the amount of patch reviews compared to new patch material overall (not just s390).

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

yes. I'm increasingly reluctant to merge things which have had no visible review from any third party. Nowadays I'll shove such patches into a pending folder and will wait a day or three to see if anyone has any feedback. If they don't I have to either ignore the patches or review them myself.

I expect (and hope) that more formal processes will come about here. Perhaps up to it-won't-be-merged-without-a-Reviewed-by:.

But that only applies to things which I merge. There's heaps of stuff coming in via the git trees which is obviously inadequately reviewed - look at all the instances of open-coded kernel_thread() which were merged after the kthread() API was introduced, for example.

And other basic stuff like "use mutexes, not semaphores":

box:/usr/src/25> grep '^+.*[ ]down[ ]*[(]' patches/git-*.patch | wc -l
32

Ever wonder where all those whitespace bugs are coming from?

box:/usr/src/25> grep '^+.*[ ]if[(]' patches/git-*.patch | wc -l
265
box:/usr/src/25> grep '^+.*[ ]while[(]' patches/git-*.patch | wc -l
35

Code which use spaces where it should be using tabs?

box:/usr/src/25> grep '^+ ' patches/git-*.patch | wc -l
1346

Heaven knows how many more serious problems are being snuck into the tree via this route.

What do we do?

It would be neat if someone could create and maintain a new scripts/spot-common-mistakes. Feed it a unified diff and it would complain about newly-added code (and only newly-added code) which has busted whitespace, adds new semaphores, adds new kernel_thread calls, etc, etc.

It would need to be fairly simple and easily-extensible, as I can imagine quite a few things getting added to it.

(Imagines a procmail rule which just bounces the email if spot-common-mistakes failed)

But it won't solve this problem.

http://patchstylecheck.googlecode.com/svn/trunk/patchstylecheckemail.pl
Might serve as a starting point for this. It doesn't have any semantic checks right now, but I guess they can be added.

	Arnd <><

	print "Your patch is now worthy to be reviewed by a real person\n";

heh. Yes, that looks like an ideal starting point.

Methinks it should do `exit 1' if anything was detected.

[Joel in case you'd not spotted this discussion, your patchstylecheckemail script was found ... Also this has produced a little patch series improving the tool. Where would you like that sent?]

As an experiment I took the -mm git repo and applied a modified versions of the patchstylecheckemail.pl to all the of the commits reported between 2.6.21-rc7 and 2.6.21-rc7-mm2. The git version of -mm links directly back to the real incoming git trees and so we get the individual commits there also.

2.6.21-rc7-mm2 appears to contian some 4313 commits in total!!! Of these some 886 failed the style check; over 20%. Obviously some of these will be false positives, or actually better as they are than made compliant. I did have a quick look over a sample of the errors and bad use of space at the start and end of line, plus overlength lines, and the lack of spaces round operators seem to be the predominant errors therein.

As these are coming from the git commits, we could consider sending an email to those authors. However, as we are only considering patches in isolation we cannot see if they got fixed later for instance.

In an ideal world we would do the same sort of analysis on the files as the appear in the HEAD of the tree. Any lines in error then would be attributed to an author and that author told to fix their mess. Hmmm, not so hard me thinks.

-apw

Heh. It's pretty crude but I've found it useful even as crude as it is. I have been doing library work instead of kernel work for a little more than a year so I haven't been improving it. That said, I'd be happy to see patches as I expect I'll do kernel work again in the future. I had made a project page for it here: http://code.google.com/p/patchstylecheck/

I'll maintain it if people want to send me patches. I also added Andy to the svn commiters list awhile back, so you could bug him as well.

Whitespace damage is the predominant one I've seen as well. When I was developing it I fed it a number of lkml patches at random and fixed all the false positives I encountered.

Had run this utility against my battery patches, and caught bunch of false positives (I believe).

+#define BATTERY_PROP(bat, prop) ({ \
+ void *value = bat->get_property(bat, BATTERY_PROP_##prop); \
+ value ? *(int*)value : 0; \
+})

Got: "Macros with multiple statements should be enclosed in a do - while loop"

I believed ({}) is equivalent for "do - while", it's widely used in kernel.

+ switch (bp) {
+ default: break;
+ };

Got "Gotos should not be indented", at "default: break;"

+static int bind_pst_to_psy(struct power_supplicant *pst,
+ struct power_supply *psy)
+{

Got "use tabs not spaces". Here spaces intentionally used for formatting purpose, not for the indenting.

oh man, every patch I review, every bug I fix, I dream of this.

Wishlist:

- wire it up to a robot which monitors all Linux mailing lists and sends machine-review comments back to originators. This will be a huge win by eliminating so much stupid crap.

- auto-detect wordwrapped and tab-replaced emails (oh glory)

- auto-detect code wider than 80-cols (swoon)

- auto-detect missing Signed-off-by:

- auto-check patch format and protocol, as per
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt and
http://linux.yyz.us/patch-format.html

- teach it about semantics:

- kthread instead of kernel_thread

- mutexes instead of semaphores

- the whole plethora of whitespace uckfuppednesses

- needlessly-initialised-to-zero-static-variables

- extern-decls-in-C

- EXPORT_SYMBOL(foo) is placed immediately after foo()'s closing brace

Hard to do? Could just whine about all EXPORT_SYMBOL's which aren't immediately preceded by ^}$ or by ;$

- new typedefs

- use of uint32_t and friends

- use of BUG_ON and BUG, frankly. The thing's a damn pest. Suggest WARN_ON+recover-from-it.

- use of `if ((var = expr()))' and similar

- large inlined functions?

- braces around single statements (advanced topic ;))

- StudlyCaps?

- anything called "tmp" or "temp"

- old-style struct initialisers

- non-ascii characters(?)

- lots more to come, I'm sure.

singing?

I made a short list last night, but yours is better/longer. Here are a few more items:

- check for new docs and/or kernel-doc
- storage class should be first
- don't use deprecated APIs
- no floating point
- no MIN/MAX
- printk wants KERN_* levels
- limit on new /proc files

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

years and years ago, when the dinosaurs roamed the land, I hacked up..
http://janitor.kernelnewbies.org/scripts/ and then left it by the wayside. Some of the checks it did are actually bogus, but I'm happy to pick that up again if there's interest in it being a useful tool.

In fact, I should probably munge it together with a similar thing I wrote at http://www.codemonkey.org.uk/projects/findbugs/
(Warning: scary regexps)

or a git checkin rule that refuses to commit if it fails ;-)

Dave

http://www.codemonkey.org.uk

Yep, I was going to mention your scripts but you beat me to it.

I'll be glad to help maintain such animals if wanted.

~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

wanted ;)

At least, it would be interesting to investigate the usefulness. I suspect it will prove to be very useful for the little things.

Heck, someone could subscribe a robot to all the mailing lists which sends nastygrams straight back at people who submit broken patches. We already need that for tab-replaced and word-wrapped patches. (ok, we have it - it's called akpm, but being robotic wearies one)

Yeah, the original script tried to do things like spinlock balancing checks, (badly). This was long before had sparse, and it was partly a "lets learn some perl" experience for myself. I'll toss that idea out now that we have better tools for that, and keep it to simple checks.

Ok, I've got a few different flavours of that script. I'll roll them all into one tomorrow and throw out some of the noisy silly ones (I don't think warning about strcpy->strncpy is really worthwhile for eg).

Additional regexps gratefully recieved.

Dave

http://www.codemonkey.org.uk

Randy and I got together and hashed out a first cut at this. (Randy actually gutted quite a lot of what I originally wrote, so deserves
much kudos for improving this beyond my initial crappy version).
You can find the script at http://www.codemonkey.org.uk/projects/checkpatch/
There's also a git clonable tree there (only http right now).

http://www.codemonkey.org.uk/projects/checkpatch/example.log shows what fell out of running it on my mbox of lkml from the past month. Some of them are kinda noisy, and perhaps should be moved under --pedantic

I'm all ears for additional regexps, bug reports or other suggestions.

Before wiring this up to a procmail rule to scan every patch, I think it's probably a better idea to flesh it out a bit more.

Dave

http://www.codemonkey.org.uk

Looks great... however I notice a few obvious false positives in the example log:

ummm?

that ain't zero...

and that ain't a static variable.

I guess trying to parse C in a regexp is a little tricky.

Also, it would be nice to be able to do something like

    git diff v2.6.20..|perl ~/checkpatch.pl -

rather than having to create a temp file -- as it stands that command produces

unknown option: -
usage: findbugs.pl [-options] file(s)
-allsource : check entire source file, not just '+' patch lines -pedantic : TBD
-style : TBD
-v, --verbose : verbose
-h, --help : this help text
Version: 002

And even worse

    git diff v2.6.20..|perl ~/checkpatch.pl

just silently does nothing (maybe a "no input files" warning would be a good clue for people).

- R.

perl ~/checkpatch.pl <(git diff v2.6.20..)

cu
Adrian

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

fixed

not? fixed.

ack, fixed.

I'll check into how to treat "-" as STDIN.

or print usage info?

Thanks.
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

hm.

box:/usr/src/25> ~/checkpatch.pl patches/slub-core.patch Checking patches/slub-core.patch: signoffs = 30 Use WARN_ON & Recovery code rather than BUG() and BUG_ON()

1588:+  VM_BUG_ON(!irqs_disabled());
1834:+  BUG_ON(flags & ~(GFP_DMA | GFP_LEVEL_MASK));
2538:+  BUG_ON(kmalloc_caches->size < sizeof(struct kmem_cache_node));
2544:+  BUG_ON(!page);
2546:+  BUG_ON(!n);

2736:+ BUG_ON(err);
2762:+ BUG_ON(flags & SLUB_UNIMPLEMENTED);
2777:+ BUG_ON(flags & (SLAB_RED_ZONE | SLAB_POISON |
2779:+ BUG_ON(ctor || dtor);

3054:+  BUG_ON(index < 0);
3118:+  BUG_ON(!page);
3120:+  BUG_ON(!s);
4062:+  BUG_ON(!name);
4083:+  BUG_ON(p > name + ID_STR_LENGTH - 1);

4188:+ BUG_ON(err);

15 warnings

surely we can do better than that ;)

box:/usr/src/25> ~/checkpatch.pl patches/git-ieee1394.patch
Checking patches/git-ieee1394.patch: signoffs = 291
Do not add new typedefs.
5239:+typedef int (*descriptor_callback_t)(struct context *ctx, 7254:+typedef void (*scsi_done_fn_t) (struct scsi_cmnd *); 8668:+typedef void (*fw_node_callback_t) (struct fw_card * card, 10077:+typedef void (*fw_packet_callback_t) (struct fw_packet *packet, 10080:+typedef void (*fw_transaction_callback_t)(struct fw_card *card, int rcode, 10085:+typedef void (*fw_address_callback_t)(struct fw_card *card, 10093:+typedef void (*fw_bus_reset_callback_t)(struct fw_card *handle, 10245:+typedef void (*fw_iso_callback_t) (struct fw_iso_context *context,

Use WARN_ON & Recovery code rather than BUG() and BUG_ON()

4342:+  BUG_ON(j >= ARRAY_SIZE(group->attrs));
9868:+  BUG_ON(retval < 0);
9872:+  BUG_ON(retval < 0);
9876:+  BUG_ON(retval < 0);
9878:+  BUG_ON(retval < 0);
10952:+ BUG_ON(!kv || !associate || kv->key.id == CSR1212_KV_ID_DESCRIPTOR ||
10983:+ BUG_ON(!kv || !dir || dir->key.type != CSR1212_KV_TYPE_DIRECTORY);
11396:+ BUG_ON(!csr || !csr->ops || !csr->ops->allocate_addr_range ||
11750:+ BUG_ON(!csr);
11802:+                 BUG_ON(csr->max_rom < 1);
12106:+ BUG_ON(!csr || !kv || csr->max_rom < 1);
12248:+ BUG_ON(!csr || !csr->ops || !csr->ops->bus_read);
14541:+         BUG_ON(max_payload < 512 - ETHER1394_GASP_OVERHEAD);
14567:+         BUG_ON(max_payload < 512 - ETHER1394_GASP_OVERHEAD);
15213:+         BUG_ON(!list_empty(&packet->driver_list) ||

23 warnings

ok.

box:/usr/src/25> ~/checkpatch.pl patches/git-net.patch
Checking patches/git-net.patch: signoffs = 831
Do not add new typedefs.
18871:+typedef unsigned int sk_buff_data_t;
18873:+typedef unsigned char *sk_buff_data_t;
20686:+typedef int (*rtnl_doit_func)(struct sk_buff *, struct nlmsghdr *, void *); 20687:+typedef int (*rtnl_dumpit_func)(struct sk_buff *, struct netlink_callback *);

Incorrect type usage for kernel code. Use __u32 etc. 21854:+uint32_t __attribute__((weak)) __div64_32(uint64_t *n, uint32_t base) 21865:+ uint32_t high, d;

Use WARN_ON & Recovery code rather than BUG() and BUG_ON()
11084:+ BUG_ON(ip_hdr(skb)->protocol != IPPROTO_TCP);
21600:+ BUG_ON(!wiphy);
21633:+ BUG_ON(!wdev);
25577:+ BUG_ON(r->ctarget != NULL);

26832:+ BUG_ON(msgindex < 0 || msgindex >= RTM_NR_MSGTYPES);
26882:+ BUG_ON(protocol < 0 || protocol >= NPROTO);
26936:+ BUG_ON(protocol < 0 || protocol >= NPROTO);
26959:+ BUG_ON(protocol < 0 || protocol >= NPROTO);

27772:+ BUG_ON(len);
30411:+ BUG_ON(hctx->ccid3hctx_p && !hctx->ccid3hctx_x_calc);
30626:+ BUG_ON(hctx == NULL);
32199:+ BUG_ON(ptr == NULL);
32217:+ BUG_ON(ptr == NULL);
58250:+ BUILD_BUG_ON(sizeof(struct illinois) > ICSK_CA_PRIV_SIZE); 61747:+ BUG_ON(sizeof(struct yeah) > ICSK_CA_PRIV_SIZE); 63079:+ BUG_ON(pad < 0);
69953:+ BUG_ON(sk == NULL);
69962:+ BUG_ON(self == NULL);
70883:+ BUG_ON(destroy == NULL);
80348:+ BUG_ON(!wiphy);

Don't init statics to 0/NULL:
61061:+static int port __read_mostly = 0;
70417:+static int hashbin_lock_depth = 0;

28 warnings

Bad David.

git-ocfs2.patch: couple fo new typedefs, zillions of BUG_ONs

box:/usr/src/25> ~/checkpatch.pl patches/git-libata-all.patch
Checking patches/git-libata-all.patch: signoffs = 167
Do not add new typedefs.
14867:+typedef int (*ata_prereset_fn_t)(struct ata_port *ap, unsigned long deadline); 14868:+typedef int (*ata_reset_fn_t)(struct ata_port *ap, unsigned int *classes,

Use WARN_ON & Recovery code rather than BUG() and BUG_ON() 5426:+ BUG_ON(!legacy_dr);

Don't init statics to 0/NULL:
2861:+static int ata_ignore_hpa = 0;

box:/usr/src/25> ~/checkpatch.pl patches/git-ia64.patch Checking patches/git-ia64.patch: signoffs = 38 Do not add new typedefs.
875:+typedef unsigned long u64;
876:+typedef unsigned int u32;
878:+typedef union err_type_info_u { 890:+typedef union err_struct_info_u { 930:+typedef union err_data_buffer_u { 954:+typedef union capabilities_u { 1009:+typedef struct resources_s { 1443:+typedef struct {

box:/usr/src/25> ~/checkpatch.pl patches/git-scsi-misc.patch Checking patches/git-scsi-misc.patch: signoffs = 165 Incorrect type usage for kernel code. Use __u32 etc.
7802:+ uint32_t len;
7803:+ uint32_t sense_len;

Don't init statics to 0/NULL:
4782:+static unsigned int ipr_transop_timeout = 0;

c'mon, this is scsi - it's full of low-hanging fruit.

box:/usr/src/25> ~/checkpatch.pl patches/git-mtd.patch Checking patches/git-mtd.patch: signoffs = 89 kmalloc return shouldn't be cast.

2085:+  msp_flash = (struct mtd_info **)kmalloc(
2087:+  msp_parts = (struct mtd_partition **)kmalloc(
2089:+  msp_maps = (struct map_info *)kmalloc(

2107:+ msp_parts[i] = (struct mtd_partition *)kmalloc(

Incorrect type usage for kernel code. Use __u32 etc.
4690:+uint32_t jffs2_truncate_fragtree(struct jffs2_sb_info *c, struct rb_root *list, uint32_t size)

5255:+  uint32_t highest_version;
5256:+  uint32_t latest_mctime;
5257:+  uint32_t mctime_ver;
5285:+uint32_t jffs2_truncate_fragtree (struct jffs2_sb_info *c, struct rb_root *list, uint32_t size);
5491:+  uint32_t crc, ofs, len;
5632:+static struct jffs2_tmp_dnode_info *jffs2_lookup_tn(struct rb_root *tn_root, uint32_t offset)
5680:+  uint32_t fn_end = tn->fn->ofs + tn->fn->size;
5908:+  uint32_t high_ver = 0;
6420:+  uint32_t crc, new_size;

6616:+ uint32_t empty_start, scan_end;
6620:+ scan_end = min_t(uint32_t, EMPTY_SCAN_SIZE(c->sector_size)/8, buf_len);
6628:+ if (unlikely(*(uint32_t *)(&buf[inbuf_ofs]) != 0xffffffff)) {
6680:+ uint32_t crc, ino = je32_to_cpu(ri->ino);

Use WARN_ON & Recovery code rather than BUG() and BUG_ON()

5494:+  BUG_ON(tn->csize == 0);
5611:+  BUG_ON(ref_obsolete(tn->fn->raw));
5859:+  BUG_ON(node->rb_right);

Don't init statics to 0/NULL:
1309:+static int nr_devices = 0; 2769:+static char *badblocks = NULL; 2770:+static char *weakblocks = NULL; 2771:+static char *weakpages = NULL; 2772:+static unsigned int bitflips = 0; 2773:+static char *gravepages = NULL; 2774:+static unsigned int rptwear = 0; 2775:+static unsigned int overridesize = 0; 2877:+static unsigned long *erase_block_wear = NULL; 2878:+static unsigned int wear_eb_count = 0; 2879:+static unsigned long total_wear = 0; 2880:+static unsigned int rptwear_cnt = 0;

33 warnings

That's more like it. MTD is a whole world of 120-column lines and type innovations.

box:/usr/src/25> ~/checkpatch.pl patches/git-powerpc.patch Checking patches/git-powerpc.patch: signoffs = 427 kmalloc return shouldn't be cast.
10969:+ char *out = (char*)kmalloc(count + 1, GFP_KERNEL);

Use 'inline' instead of '__inline__'26644:+static __inline__ void atomic_scrub(void *va, u32 size)

Do not add new typedefs.
7336:+typedef struct bd_info {

Use WARN_ON & Recovery code rather than BUG() and BUG_ON() 13629:+ BUG_ON(linear_map_hash_slots[lmi] & 0x80); 13641:+ BUG_ON(!(linear_map_hash_slots[lmi] & 0x80));
13880:+ BUG_ON(pte_val(*pg) & (_PAGE_PRESENT | _PAGE_HASHPTE));
13922:+ BUG_ON(PageHighMem(page));
15543:+ BUG_ON(mpic == NULL);
16671:+ BUG_ON(!tmp_np);
17745:+ BUG_ON(spu_coredump_calls != calls);
17887:+ BUG_ON(!list_empty(&ctx->rq));
19057:+ BUG_ON(list_empty(rq));
24237:+ BUG_ON(intsize != 2);
24271:+ BUG_ON(! device_is_compatible(node, "ibm,uic")); 24335:+ BUG_ON(!np); /* uic_init_tree() assumes there's a UIC as the 24383:+ BUG_ON(! primary_uic);

16 warnings

box:/usr/src/25> ~/checkpatch.pl patches/git-input.patch Checking patches/git-input.patch: signoffs = 85 Incorrect type usage for kernel code. Use __u32 etc. 4164:+ uint32_t mask;
4185:+ uint32_t status;

Use WARN_ON & Recovery code rather than BUG() and BUG_ON()

3504:+  BUG_ON(kbd == NULL);
3540:+  BUG_ON(kbd == NULL);
5595:+  BUG_ON(ptr == NULL);
5632:+  BUG_ON(ptr == NULL);
7873:+  BUG_ON(mlc->ddi >= 6);
8106:+  BUG_ON(down_trylock(&mlc->isem));
8142:+          BUG_ON(node->object.func == NULL);
8345:+  BUG_ON(map == NULL);
8353:+  BUG_ON(mlc == NULL);
8373:+  BUG_ON(drv == NULL);
8408:+  BUG_ON(map == NULL);
8415:+  BUG_ON(mlc == NULL);
8431:+  BUG_ON(map == NULL);
8438:+  BUG_ON(mlc == NULL);
8463:+  BUG_ON(mlc == NULL);
8511:+  BUG_ON(mlc == NULL);
9700:+  BUG_ON(down_trylock(&mlc->isem));
9701:+  BUG_ON(down_trylock(&mlc->osem));
9762:+  BUG_ON(down_trylock(&mlc->osem));
9781:+  BUG_ON(down_trylock(&mlc->csem));
9829:+  BUG_ON(mlc->opacket & HIL_CTRL_APE);

Don't init statics to 0/NULL:
11053:+static unsigned int channel_mask = 0xFFFF;

24 warnings

box:/usr/src/25> ~/checkpatch.pl patches/git-e1000.patch Checking patches/git-e1000.patch: signoffs = 44 Do not add new typedefs.
21237:+typedef enum {
33390:+typedef enum {
36694:+typedef enum {
36701:+typedef enum {

4 warnings

box:/usr/src/25> ~/checkpatch.pl patches/git-alsa.patch Checking patches/git-alsa.patch: signoffs = 297 0 warnings

whoa.

box:/usr/src/25> ~/checkpatch.pl patches/git-infiniband.patch Checking patches/git-infiniband.patch: signoffs = 113 Use WARN_ON & Recovery code rather than BUG() and BUG_ON() 8143:+ BUG_ON(mlx4_ib_alloc_db_from_pgdir(pgdir, db, order)); 12629:+ BUG_ON(cmd->free_head < 0);
16580:+ BUG_ON(index < dev->caps.num_mgms);
16665:+ BUG_ON(amgm_index_to_free < dev->caps.num_mgms);
16681:+ BUG_ON(index < dev->caps.num_mgms);

Don't init statics to 0/NULL:
10333:+ path->static_rate = 0;
15461:+static int msi_x = 0;
16113:+ static int mlx4_version_printed = 0;

8 warnings

box:/usr/src/25> ~/checkpatch.pl patches/git-wireless.patch Checking patches/git-wireless.patch: signoffs = 2009
kmalloc return shouldn't be cast.
62076:+ a16 = (zd_addr_t *)kmalloc(count16 * (sizeof(zd_addr_t) + sizeof(u16)),

Do not add new typedefs.
50317:+typedef void (debug_access_t)(struct rt2x00_dev *rt2x00dev, 64586:+typedef u16 __nocast zd_addr_t;
76135:+typedef enum { ALG_NONE, ALG_WEP, ALG_TKIP, ALG_CCMP, ALG_NULL } 76195:+typedef enum {
87265:+typedef enum {
87366:+typedef ieee80211_txrx_result (*ieee80211_tx_handler) 87369:+typedef ieee80211_txrx_result (*ieee80211_rx_handler) 87925:+typedef enum {
92694:+typedef enum { ParseOK = 0, ParseUnknown = 1, ParseFailed = -1 } ParseRes; 102286:+typedef int (*iw_compat_handler)(struct cfg80211_registered_device *drv,

Use WARN_ON & Recovery code rather than BUG() and BUG_ON()
23286:+ BUILD_BUG_ON(BCM43xx_SEC_KEYSIZE < ETH_ALEN);
33026:+ BUILD_BUG_ON(BCM43xx_TAB_ROTOR_SIZE != ARRAY_SIZE(bcm43xx_tab_rotor)); 33027:+ BUILD_BUG_ON(BCM43xx_TAB_RETARD_SIZE != ARRAY_SIZE(bcm43xx_tab_retard)); 33028:+ BUILD_BUG_ON(BCM43xx_TAB_FINEFREQA_SIZE != ARRAY_SIZE(bcm43xx_tab_finefreqa)); 33029:+ BUILD_BUG_ON(BCM43xx_TAB_FINEFREQG_SIZE != ARRAY_SIZE(bcm43xx_tab_finefreqg)); 33030:+ BUILD_BUG_ON(BCM43xx_TAB_NOISEA2_SIZE != ARRAY_SIZE(bcm43xx_tab_noisea2)); 33031:+ BUILD_BUG_ON(BCM43xx_TAB_NOISEA3_SIZE != ARRAY_SIZE(bcm43xx_tab_noisea3)); 33032:+ BUILD_BUG_ON(BCM43xx_TAB_NOISEG1_SIZE != ARRAY_SIZE(bcm43xx_tab_noiseg1)); 33033:+ BUILD_BUG_ON(BCM43xx_TAB_NOISEG2_SIZE != ARRAY_SIZE(bcm43xx_tab_noiseg2)); 33034:+ BUILD_BUG_ON(BCM43xx_TAB_NOISESCALEG_SIZE != ARRAY_SIZE(bcm43xx_tab_noisescaleg1)); 33035:+ BUILD_BUG_ON(BCM43xx_TAB_NOISESCALEG_SIZE != ARRAY_SIZE(bcm43xx_tab_noisescaleg2)); 33036:+ BUILD_BUG_ON(BCM43xx_TAB_NOISESCALEG_SIZE != ARRAY_SIZE(bcm43xx_tab_noisescaleg3)); 33037:+ BUILD_BUG_ON(BCM43xx_TAB_SIGMASQR_SIZE != ARRAY_SIZE(bcm43xx_tab_sigmasqr1)); 33038:+ BUILD_BUG_ON(BCM43xx_TAB_SIGMASQR_SIZE != ARRAY_SIZE(bcm43xx_tab_sigmasqr2));

49080:+ BUILD_BUG_ON(!(__mask) ||               \
49090:+ BUILD_BUG_ON(!(__mask) ||               \

70106:+ BUILD_BUG_ON(SSB_PCICORE_BCAST_ADDR != SSB_CHIPCO_BCAST_ADDR); 70107:+ BUILD_BUG_ON(SSB_PCICORE_BCAST_DATA != SSB_CHIPCO_BCAST_DATA);
74967:+ BUILD_BUG_ON(index >= SSB_EXTIF_NR_GPIOOUT); \
74971:+ BUILD_BUG_ON(index >= SSB_EXTIF_NR_GPIOOUT); \
76919:+ BUG_ON(!wiphy);
76952:+ BUG_ON(!wdev);
86065:+ BUILD_BUG_ON(sizeof(struct ieee80211_rx_status) > sizeof(skb->cb)); 86769:+ BUG_ON(local->reg_state != IEEE80211_DEV_REGISTERED); 86927:+ BUILD_BUG_ON(sizeof(struct ieee80211_tx_packet_data) > sizeof(skb->cb)); 88357:+ BUG_ON(dev == local->apdev);
99612:+ BUG_ON(IS_ERR(drv));
99875:+ BUG_ON(!wiphy);

Don't init statics to 0/NULL:
48899:+static int rt2x00_debug_level = 0; 88421:+static int ieee80211_regdom = 0x10; /* FCC */ 88431:+static int ieee80211_japan_5ghz /* = 0 */;
94013:+ static unsigned long last_tsf_debug = 0;

43 warnings

A good start, thanks. The BUG_ON() thing might be unpopular, but I think it's for the best. It should skip BUILD_BUG_ON though.

Yup, I ran this too.

I agree -- killing the kernel for a driver bug is dump. I'll remove all these BUGs before merging.

This is a false positive/opportunity for script improvement, obviously.

Already zapped these.

- R.

So you prefer to corrupt data instead? I don't think that's a good idea. Don't do it. Silent failures are really bad.

BUG_ONs are only fatal when you're in interrupt context anyways because it often ends up trying to kill the idle task. In process context while there might be some lost locks in most cases the system continues running happily.

At some point I had a hack to just restart idle to handle the interrupt case too. Perhaps that might be worth resurrecting if you want to solve this properly.

Would be imho far preferable over not having the BUGs imho.

-Andi

BTW, I missed this before -- BUILD_BUG_ON() is actually far better than WARN_ON(), I think.

Maybe something like this? (Although someone who knows perl probably has a better way)

---
Don't tell people to change BUILD_BUG_ON() to WARN_ON().

@@ -123,7 +123,7 @@
 	$warnings += search(qr/kernel_thread\(/, "Use kthread abstraction instead of kernel_thread()\n");
 	$warnings += search(qr/typedef/, "Do not add new typedefs.\n");
 	$warnings += search(qr/uint32_t/, "Incorrect type usage for kernel code. Use __u32 etc.\n");
-	$warnings += search(qr/BUG(_ON)\(/, "Use WARN_ON & Recovery code rather than BUG() and BUG_ON()\n");
+	$warnings += search(qr/(?<!BUILD_)BUG(_ON)\(/, "Use WARN_ON & Recovery code rather than BUG() and BUG_ON()\n");
 
 	# pedantic: Noisy regexps that aren't really fatal.
 	if ($opt_pedantic) {

There's not much to be said in favour of u32, really. Except it's shorter and I can never remember where the underscore goes in uint_32t.

If kernel used u_int32_t globally then the world would probably be a better place. But using just the one name has its advantages from a consistency POV.

box:/usr/src/linux-2.6.21> grep -r '[ \(]u32' . | wc -l
39599
box:/usr/src/linux-2.6.21> grep -r '[ \(]uint32_t' . | wc -l
5132

CodingStyle permits either variant, fwiw.

The underscore always goes before the final t (short for type I assume). The typedefs in the kernel do the same... kmem_cache_t (now replaced, though) as an example.

#undef u32
#undef u_int32_t

Once again, \b comes to the rescue! :)
[And it also works with u32 at SOL and EOLs, and -o counts the number of occurrences, not the number of lines]

grep -or '\b__uX\b' . | wc -l

.----..-------.------.---------.----------..------.------.--------.
| || uX | __uX | uintX_t | u_intX_t || sX | __sX | intX_t |

|  8 || 28004 | 6159 |    3883 |      422 ||  416 |   65 |     75 |
| 16 || 13734 | 3557 |    2613 |      426 ||  393 |  128 |     19 |
| 32 || 42971 | 6656 |    5416 |      879 || 1442 |  363 |    428 |
| 64 || 11219 | 1613 |     811 |       59 || 1061 |   81 |    111 |

`----^^-------^------^---------^----------^^------^------^--------'

The tendency towards __uX and uX is probably because these have been existent since decades, even before stdint.h and uintX_t got introduced. Which btw, is another point. Userspace programs that need to include files from /usr/include/linux sometimes have a hard time compiling because they want u8, which is defined (or not!) somewhere in /usr/include/asm. Maybe it is not a problem anymore today, but I had had it often enough to call it a problem. This is where uintX_t could come a little more handy, because userspace has a higher chance of already knowing it through stdint.h or some compiler foo.

Jan

Ack, I added \b. Thanks.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

The warning is bogus imho. How do you write recovery code for internal broken code logic?

-Andi

Yes, it is marginal. But people do very often reach for BUG_ON() where they could have at least partly recovered in some fashion - enough for the info to hit the logs so we have a better chance of fixing it.

BUG_ON() is of course sometimes the right thing to do, but the idea here is to suggest to the developers that they put a bit of thought into whether it was really justified.

This little checking tool should have both "error" and "warning" levels - AKA "fix this" and "think about this" levels. BUG_ON would be a warning thing.

At least one way to handle BUG_ON() type situations more cleanly (for some anyway) is to fake a hot-unplug/plug event. Thats something that would be nice to get into the PCI code as it'll let you recover from many things (even in some cases hardware fails if you go via D3). Does need some supporting logic (eg for networking to get back with the same config and online)

That would have a high risk of deadlock on some lost lock.

-Andi

Well I was assuming you'd code this up in the driver not arbitarily - and you need to do that for IRQ anyway. So something like

	writel(0xFFFFFFFF, &mdev->irq_mask);
	pci_mark_failed(pdev, PCI_TRY_REPLUG|PCI_TRY_D3);
	spin_unlock(&mylock);
	return -EXPLODED;

with pci_mark_failed firing off any replug via a work queue

Alan

There's a -pedantic option there just for this. I'll move BUG_ON under it. What's the consensus on the u32 thing, move that too, or drop completely?

Dave

http://www.codemonkey.org.uk

Not answering your question directly, but FWIW: In day to day coding and email, I strongly advocate the use of u8/u16/u32 over the C99 size-based types.

Simple, direct (read: easiest to review), and less typing.

Jeff

...

Fixed missing \n.

...

BUILD_BUG_ON is now skipped (in my script, not sent to DaveJ yet).

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Neat.

Does it check for:

functions marked extern?
pulling in external functions or variables without a header file? return used as a function, eg return(foo);?

Mathematics is the supreme nostalgia of our time.

These sound a little more tricky than just dumb regexps. I don't want to expand this to a fullblown C parser (given that we have sparse which can do a better job), but I don't object to adding some extra code to give the searches more context.

Dave

http://www.codemonkey.org.uk

The first is a straightforward one-line regexp, as is the last:

^extern \w.*\w\(
return\s?\(

The middle one is a bit trickier. Basically, we don't want people doing either:

extern int capital_of_assyria;
int sir_not_appearing_in_this_module();

in .c files. The first of those two is easy, provided you can figure out the file type. And both are possible with multiline regexps.

This whole thing would be quite a bit more powerful if your search function joined all the lines together, did a potentially multiline search, then calculated the line numbers from the search results. Then you could search for things like:

if (...)
{

if (...) {
single_statement;
}

Looking forward to fully-automated pedantry. This should probably live in scripts/.

Mathematics is the supreme nostalgia of our time.

data marked extern?

Just omit the ending \( to catch functions or data. Added.

Added. Thanks.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

It's perfectly reasonable to have a data extern declaration in a header file.

Mathematics is the supreme nostalgia of our time.

but it's not perfectly acceptable to have

extern unsigned long volatile jiffies;

in a .c file.

The biggest problem I'm seeing ATM is that this script is a bit too simplistic. It doesn't know what it's looking at. We'll have to address that, I think.

~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

If you can make it run a regexp over the whole file at once rather than a line at a time, you can deal with this with multi-line regexps. Roughly, match:

a +++ patch header followed by
any number of lines not starting with +++ followed by the actual target expression

So, approximately:

/+++ [\w\/]+.c(.*)\\n((([^+])|(\+[^+]))\n)*extern.*/

Or you could just make search remember what file it's in as it walks the list of lines. But as I mentioned, multiline regexps are useful for things other than just remembering what file we're in.

Mathematics is the supreme nostalgia of our time.

FYI, checkpatch.pl Version 003 is now available at the URL above.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Nice!

Here are a few more:
- Check for all of (u)int{8,16,32,64}_t
- Check for GNU extension __FUNCTION__
- Don't use space before tab
- Don't use trailing white space

index cbda29a..b44a3f3 100755
@@ -143,10 +143,26 @@ sub process {
 			"Bad variable name: tmp. Please use something more descriptive.\n");
 		$warnings += search(qr/temp(,|;)/,
 			"Bad variable name: temp. Please use something more descriptive.\n");
+		$warnings += search(qr/uint8_t/,
+			"Incorrect type usage for kernel code. Use __u8/u8\n");
+		$warnings += search(qr/uint16_t/,
+			"Incorrect type usage for kernel code. Use __u16/u16\n");
 		$warnings += search(qr/uint32_t/,
-			"Incorrect type usage for kernel code. Use __u32 etc.\n");
+			"Incorrect type usage for kernel code. Use __u32/u32\n");
+		$warnings += search(qr/uint64_t/,
+			"Incorrect type usage for kernel code. Use __u64/u64\n");
+		$warnings += search(qr/int8_t/,
+			"Incorrect type usage for kernel code. Use __s8/s8\n");
+		$warnings += search(qr/int16_t/,
+			"Incorrect type usage for kernel code. Use __s16/s16\n");
+		$warnings += search(qr/int32_t/,
+			"Incorrect type usage for kernel code. Use __s32/s32\n");
+		$warnings += search(qr/int64_t/,
+			"Incorrect type usage for kernel code. Use __s64/s64\n");
 		$warnings += search(qr/\b(BUG|BUG_ON)\b/,
 			"Try to use WARN_ON & Recovery code rather than BUG() or BUG_ON()\n");
+		$warnings += search(qr/__FUNCION__/,
+			"Should use C99 __func__ instead of GNU __FUNCTION__\n");
 	}
 
 	# coding style:

@@ -154,6 +170,8 @@ sub process {
 		$warnings += search(qr/,[^[:space:]]/, "Need space after comma:\n");
 		$warnings += search(qr/\([[:space:]]/, "Don't use space after '(':\n");
 		$warnings += search(qr/[[:space:]]\)/, "Don't use space before ')':\n");
+		$warnings += search(qr/ \t/, "Don't use space before tab:\n");
+		$warnings += search(qr/[[:space:]]$/, "Don't use trailing white space:\n");
 		$warnings += search(qr/if\(|for\(|while\(|switch\(/,
 				"Need space after if/for/while/switch:\n");
 		$warnings += search(qr/sizeof[[:space:]]/,

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE) [email protected] ------- The Corporate Village, Da Vincilaan 7-D1 Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

__FUNCTION__ is prefered over __func__

Is there a reason for that?
- __FUNCTION__ is a GNU extension
- __func__ is C99
- __func__ is shorter to type ;-)

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE) [email protected] ------- The Corporate Village, Da Vincilaan 7-D1 Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

In that case we should use __func__.

But we discussed this at some length 3-4 years ago and decided to use __FUNCTION__. I don't remember why. Perhaps problems with gcc support for __func__?

(It could have been that compile-time string concatenation was involved:

        printf("xxx" __FILE__);			/* works */
        printf("xxx" __FUNCTION__);		/* doesn't */

Or not.)

I tried gcc 2.95/3.2/3.3/3.4/4.0/4.1, they all recognize __func__ and __FUNCTION__, like in e.g. printf("%s", __func__);

Yep, when trying concatenation, I got:
- 2.95: works fine
- 3.2:
syntax error before "__func__"
warning: concatenation of string literals with __FUNCTION__ is deprecated
- 3.3:
error: syntax error before "__func__"
warning: concatenation of string literals with __FUNCTION__ is deprecated
- 3.4/4.0:
error: syntax error before "__func__"
error: syntax error before "__FUNCTION__"
- 4.1:
error: expected ')' before '__func__'
error: expected ')' before '__FUNCTION__'

Hence gcc 3.2 and up treat __func__ like the a variable, as per C99, while __FUNCTION__ has been moving from a virtual preprocessor definition in 2.95 to a variable, like __func__.

So in the end it doesn't matter, as concatenation has been fixed in the Linux source tree anyway.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE) [email protected] ------- The Corporate Village, Da Vincilaan 7-D1 Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

Great, thanks for working all that out.

So __func__ it is. In new code. However "convert __FUNCTION__ to __func__" patches will be cheerfully ignored - life is too short.

err, kernel.h has

/* Trap pasters of __FUNCTION__ at compile-time */ #define __FUNCTION__ (__func__)

info gcc tells:

__FUNCTION__' is another name for __func__'. Older versions of GCC
recognize only this name. However, it is not standardized. For maximum portability, we recommend you use `__func__', but provide a fallback definition with the preprocessor:[...]

Jan

I strongly disagree. These should be allowed, for they are (I think) C99.

Jan

Bummer... Of course!

Thx!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE) [email protected] ------- The Corporate Village, Da Vincilaan 7-D1 Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

Is this not the meaning of the Acked-by: ?

Perhaps its time for Linus to say he won't accept any patches which are not Acked and place the onus on getting those on the tree maintainers. In theory at least tree maintainers are supposed to be responsible for the stuff coming through their tree. They could be made responsible to ensuring only Ack'd stuff is committed. Automated checks could be made for that at least.

As for the white space errors. I think that we should perhaps run a spectrum of commits from each tree through the checked proposed later in the thread. Where any significant non-compliance is detected that should be sent to the tree maintainer and their 'upstream' and corrections expected.

Public shaming. A savaging from Linus' or any other respected community member is something to be avoided at all costs.

-apw

Is -mm less full now? I've seen you dropped your third patch bomb, so maybe there is room now ..
I regenerated the kconfig patches. They fit on top of todays git but they do have some rejects on 2.6.21-mm1. The rejects are trivial to fix, but there are about 10 of them.

So when will be a good time to add the Kconfig patches to git390? And do you prefer to let them rot^H^H^Hmature in -mm for a while before they can go upstream?

blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.

Near 500 patches, which is the lowest it's been in years. I do appear to have a few hundred patch-bearing emails saved away though.

Go wild.

Your call. I'd be inclined to push them early personally - Kconfig problems tend to be easy to spot and easy to fix and there's ample time to fine tune it.

Ok, that is my feeling as well. The patches won't get better by waiting. I will add them to the for-linus/for-andrew branches and include them in the next please-pull for Linus. Be prepared for some breakage in -mm..

blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.