Date:	Thu, 26 Apr 2012 08:53:30 -0700
From:	"Luis R. Rodriguez" <mcgrof@...not-panic.com>
To:	David Miller <davem@...emloft.net>
Cc:	zambrano@...adcom.com, netdev@...r.kernel.org,
	linux-wireless <linux-wireless@...r.kernel.org>,
	linux-bluetooth <linux-bluetooth@...r.kernel.org>,
	Arend Van Spriel <arend@...adcom.com>,
	Joe Perches <joe@...ches.com>
Subject: Re: [PATCH] b44: properly use pr_fmt()

On Thu, Apr 26, 2012 at 6:02 AM, Luis R. Rodriguez
<mcgrof@...not-panic.com> wrote:
> On Thu, Apr 26, 2012 at 5:23 AM, Luis R. Rodriguez
> <mcgrof@...not-panic.com> wrote:
>> On Mon, Apr 23, 2012 at 9:47 PM, David Miller <davem@...emloft.net> wrote:
>>> From: "Luis R. Rodriguez" <mcgrof@...not-panic.com>
>>> Date: Mon, 23 Apr 2012 19:46:43 -0700
>>>
>>>> From: "Luis R. Rodriguez" <mcgrof@...not-panic.com>
>>>>
>>>> pr_fmt() is either defined or we redefine it. Typically
>>>> drivers define it prior to including printk.h but this
>>>> is done under the assumption that no other subsystem
>>>> it uses has already defined pr_fmt(). In such cases
>>>> pr_fmt() should be undefined and redefined.
>>>>
>>>> Doing this properly shaves down compilation time quite
>>>> considerably.
>>>>
>>>> Signed-off-by: Luis R. Rodriguez <mcgrof@...not-panic.com>
>>>
>>> Every driver defines pr_fmt then includes the headers.  I do not
>>> see any other place which performs the initial undef of pr_fmt
>>> like you want to start doing here.
>>
>> One example is, and that is where I got the hint:
>>
>> drivers/net/wireless/mwifiex/decl.h
>>
>>> If there is a reason we should start doing this, it would appear
>>> to be a universal reason, not one specific to this driver.
>>
>> Sure.
>>
>>> If that's the case, we should formally agree to this and then work
>>> on gradually converting all the drivers in reasonably sized chunks.
>>
>> Understood.
>>
>>> Either way I can't apply this patch as-is, sorry Luis.
>>
>> So the issue can occur if any parent header ends up defining this,
>> this could happen if a subsystem wants to define the pr_fmt in their
>> own header and a driver for example wants to override this. In theory
>> this does not occur but in practice this may vary and if we want to
>> ensure this does not happen perhaps the thing to do is to document
>> this as such.
>>
>> I just tested allmodconfig though and checked compilation for V=1
>> against drivers/net/wireless/ and found no issues against linux-next
>> though so in practice at least for wireless networking it seems we are
>> squeaky clean in its usage. I'll test across the kernel though next to
>> see if there are any real violators of the assumption. Unfortunately
>> I've hit an issue with compiling allmodconfig already with linux-next
>> that is already reported so my homework will be to find a shiny
>> linux-next to compile test against allmodconfig.
>>
>> An area that this does clearly happen but that likely does not merit
>> and upstream change for is in the backport work compat that defines
>> this for older kernels and later we have to also backport users of
>> pr_fmt such as pr_emerg_once. Ultimately drivers will want to actually
>> then override this and in that case the undef is necessary as
>> otherwise you get the redefinition warning which incurs in practice a
>> considerable CPU performance hit at compilation time.
>
> Additionally Arend has pointed pointed out that previously printk.h
> was removed as a direct include as well given that kernel.h includes
> it already:
>
> https://lkml.org/lkml/2011/5/25/331
>
> So this would be a second issue with this patch.

OK I found no pr_fmt warnings at all for make allmodconfig ; make V=1
; on v3.4-rc4. For this and others reason stated then yes, this patch
is simply bogus.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists