From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1755985AbcH2SBP convert rfc822-to-8bit (ORCPT <rfc822;w@1wt.eu>);
        Mon, 29 Aug 2016 14:01:15 -0400
Received: from smtp.codeaurora.org ([198.145.29.96]:43841 "EHLO
        smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1755616AbcH2SBO (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 29 Aug 2016 14:01:14 -0400
DMARC-Filter: OpenDMARC Filter v1.3.1 smtp.codeaurora.org 69E3B61D7C
Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none header.from=codeaurora.org
Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=pass smtp.mailfrom=kvalo@codeaurora.org
From: Kalle Valo <kvalo@codeaurora.org>
To: Joe Perches <joe@perches.com>
Cc: "Levin\, Alexander" <alexander.levin@verizon.com>,
        Sasha Levin <levinsasha928@gmail.com>,
        Greg KH <gregkh@linuxfoundation.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "ksummit-discuss\@lists.linuxfoundation.org" 
        <ksummit-discuss@lists.linuxfoundation.org>
Subject: Re: checkkpatch (in)sanity ?
References: <1472330452.26978.23.camel@perches.com>
        <20160828005636.GB19088@sasha-lappy>
        <1472348579.26978.47.camel@perches.com>
        <874m634yip.fsf@purkki.adurom.net>
        <1472473855.3425.18.camel@perches.com>
Date: Mon, 29 Aug 2016 21:01:06 +0300
In-Reply-To: <1472473855.3425.18.camel@perches.com> (Joe Perches's message of
        "Mon, 29 Aug 2016 05:30:55 -0700")
Message-ID: <87lgzfihel.fsf@kamboji.qca.qualcomm.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Joe Perches <joe@perches.com> writes:

> On Mon, 2016-08-29 at 14:15 +0300, Kalle Valo wrote:
>> I wish that checkpatch would have a way to enable/disable warnings per
>> directory (or file). For example, there would be
>> drivers/net/wireless/ath/ath10k/.checkpatch which would disable the
>> warnings are not suitable for ath10k for one reason or another:
>> 
>> 'MSLEEP',
>> 'USLEEP_RANGE',
>> 'PRINTK_WITHOUT_KERN_LEVEL',
>> 'NETWORKING_BLOCK_COMMENT_STYLE',
>> 'BLOCK_COMMENT_STYLE',
>> 'LINUX_VERSION_CODE',
>> 'COMPLEX_MACRO',
>> 'PREFER_DEV_LEVEL',
>> 'PREFER_PR_LEVEL',
>> 'COMPARISON_TO_NULL',
>> 'BIT_MACRO',
>> 'CONSTANT_COMPARISON',
>> 'MACRO_WITH_FLOW_CONTROL'
>> 
>> Currently my workaround is to have a custom ath10k-check script[1] which
>> runs checkpatch with those checks disabled. Oh, and it also filters out
>> some of the warnings based on the symbol it is located in.
>> 
>> https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath10k/ath10k-check
>
> Hey Kalle:
>
> I looked at your script (which also does compilation
> and sparse checking) I don't see how a .checkpatch_conf
> hierarchy helps you much there as you've added all those
> long symbol name long line avoidance bits.

Yeah, it would not completely replace my script. But there's now quite a
difference with checkpatch parameters what other people use and what I
use.

> Also, there'd be a lot of rework to the globals in
> checkpatch for per-directory specific overrides if someone
> fed it files in multiple directories like
>
> checkpatch.pl <patchfile touching lib/kernel/include>

Oh, that's a good point. ath10k patches only touch one directory so I
didn't think of this.

> A couple btw's:
>
> Why avoid the printk, sleep or macro tests?

Actually I don't remember anymore :) I should have documented that in
the script.

(Runs some tests)

PRINTK_WITHOUT_KERN_LEVEL: I think I can enable this again, IIRC earlier
it gave useless warnings in ath10k_warn() & co.

MSLEEP: I think this is just noise, we don't care if it's actually 20 ms
even if ask for 10 ms. That's the minimum time to wait, not the maximum
(from our/HW point of view).

drivers/net/wireless/ath/ath10k/ahb.c:422: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/ahb.c:427: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/ahb.c:432: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/ahb.c:437: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/ahb.c:442: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/pci.c:2244: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/pci.c:2251: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/pci.c:2275: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt

USLEEP_RANGE: I don't remember why I disabled this, most likely just
because of the msleep noise above

drivers/net/wireless/ath/ath10k/pci.c:2672: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt

COMPLEX_MACRO: Can't remember this one either, I guess I just didn't
find a good solution to shut up checkpatch. But that was a long time
ago, it might be fixable now.

drivers/net/wireless/ath/ath10k/wmi.h:315: Macros with complex values should be enclosed in parentheses
drivers/net/wireless/ath/ath10k/wmi.h:6344: Macros with complex values should be enclosed in parentheses

BIT_MACRO: there's a lot of warnings from this one, the benefit from
changing all of them is questionable so I just disabled it.

drivers/net/wireless/ath/ath10k/rx_desc.h:676: Prefer using the BIT macro

> And this for ath10k_core_register_work:
>
>     ('ath10k_core_register_work', 'RETURN_VOID'),
>
> and the code associated to it:
>
> err:
> 	/* TODO: It's probably a good idea to release device from the driver
> 	 * but calling device_release_driver() here will cause a deadlock.
> 	 */
> 	return;
> }
>
> ia avoided a few times in the kernel by using a bare ";"
> instead of "return;" before the function closing brace.

Heh, didn't know that.

> It's maybe unfortunate that gcc / c spec doesn't allow
> jumping to a label just before the function close brace.

Indeed.

I find checkpatch very useful to maintain certain coding style in ath10k
and I don't need to worry small details like whitespace. I just need to
disable some of the warnings so that they don't hide the real warnings
I'm interested about.

-- 
Kalle Valo