Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Em 30-04-2012 04:47, Borislav Petkov escreveu:
> On Sun, Apr 29, 2012 at 02:39:04PM -0300, Mauro Carvalho Chehab wrote:
>> Em 29-04-2012 13:43, Joe Perches escreveu:
>>> On Sun, 2012-04-29 at 13:20 -0300, Mauro Carvalho Chehab wrote:
>>>> The script below is even better. After that, only 113 occurrences of __func__
>>>> is now found at drivers/edac, and some of them are not related to debugf[1-9],
>>>> so they shouldn't be cover on a patch like that.
>>>> I'll do some manual cleanup on it.
>>>
>>> Hi Mauro.
>>>
>>> Another thing you could do would be to
>>> separate the level from the multiple macros,
>>> use a single macro, and convert the uses.
>>>
>>> #define debugf(level, fmt, ...)
>>> and change the uses to
>>> debugf([0-n], "some format", args...)
>>>
>>> I believe that's the more predominate
>>> kernel style for debugging macros with
>>> a tested level or mask.
>>
>> Agreed.
>>
>>> Perhaps also add !CONFIG_EDAC_DEBUG
>>> format/args checking to the debug statements.
>>
>> Most/all debug-only stuff are already checking for CONFIG_EDAC_DEBUG.
>> There are a few static debug-only data/functions that aren't testing for
>> it, but the compiler should remove the dead code anyway, so this shouldn't
>> cause any harm.
>>
>>> Lastly, indenting the messages 2 tabs isn't
>>> really useful, one or two spaces is probably
>>> enough.
>>
>> agreed.
>>
>>>
>>> I did this a bit ago so it may not apply
>>> after your changes:
>>
>> Believe or not, it applied without troubles ;)
>>
>> I've added at the end of my experimental series, at:
>>
>>
>> git://git.infradead.org/users/mchehab/edac.git experimental
>>
>> be careful if you use this branch, as I'm rebasing it every time I need
>> to change something on this series.
>>
>> I'm keeping a non-rebased version, with one branch per review, at:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-edac.git
>>
>> The current review is at hw_events_v17. Patches were already pushed there.
>> they should be there after the usual kernel.org master/mirror replication
>> delay.
> 
> Now wait a minute,
> 
> you guys are so trigger-happy to apply humongous, cleanup patches but
> let me ask this: can anyone of you really test those changes with each
> driver? Do you have all the hardware that those patches touch?

Well, then why you've touched that in the first place, without even looking
what would be affected at the EDAC core and at the drivers? Didn't you tested
it on all hardware that your patch affected?

Now that your patch got applied, reverting it is not a solution, as newer
stuff now assumes that __func__ will be at the debug messages. So, reverting
it will cause regressions.

> I know, I know, it builds fine and it looks correct but subtle bugs tend
> to sneak in in exactly such situations.

This patch touches only on debug code that aren't even enabled on production
kernels. Assuming that a sneaky bug were introduced, this won't cause much
hurt, and any developer inspecting those debug messages will be able to discover
and fix what happened there.

Regards,
Mauro