Re: [PATCH 2/2] i2c-hid: remove mostly useless parameter 'debug'

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

On 02/08/13 20:31, Andy Shevchenko wrote:
> On Fri, 2013-08-02 at 20:14 +0200, Benjamin Tissoires wrote: 
>> On 02/08/13 16:49, Andy Shevchenko wrote:
>>> On Fri, 2013-08-02 at 16:30 +0200, Benjamin Tissoires wrote: 
> 
> []
> 
>>>> Moreover, the dev_dbg calls are compiled out if DEBUG symbol is not
>>>> set.
>>>
>>> Yes, and what is the difference between previously used
>>> if (debug)
>>>   dev_dbg(...)
>>>
>>> ?
>>
>> That's because the previous code was directly using
>> dev_printk(KERN_DEBUG,...) and not dev_dbg() within the condition.
>>
>> This call does not use dynamic debugging or does not get compiled out if
>> DEBUG is not set.
> 
> Yeah, I've checked this just after I sent the message.
> 
>> Also, to my defence, I can add that I developed this driver on an ARM
>> board without dynamic debugging support enabled... :)
> 
> 
> So, the problem is DYNAMIC_DEBUG is not default option in kernel (and I
> could imagine why).
> 
> I checked few distros for that: Debian/Ubuntu - off,
> Fedora/OpenSuse/ArchLinux - on.
> 
> Just to understand what kind of user might run this with debug? Is it
> developer or person who knows the subject? If it's not a such person, it
> would be difficult  to him or her to enable dynamic debug in distros
> like Ubuntu, which sucks.

I'm afraid people reporting this kind of bugs are most of the time
regular users, who just bought a brand new laptop, and who send a bug
report through their distribution's bugzilla... :(
I'm not expecting that much of developers or person who knows the
subject to actually report the problem, because most of the time a
normal user reports "my touchpad is not working". At least, that's what
I have mostly seen in the input subsystem.

> 
>>>> So, if we ever have to change this debug variable, I would prefer
>>>> using the hid debug environment which would at least limit the number
>>>> of debug outputs to the HID subsystem.
>>>
>>> Usually when I see such code I understood it was written in
>>> pre-dynamic-debug epoch. So, my point is to switch to dynamic debug and
>>> use it efficiently.
>>
>> Ok, so if you can guarantee me that adding the proper kernel parameter
>> will allow me to retrieve all the i2c-hid logs from the boot, then I'd
>> be happy to ack this. However, I have no way to test this right now, so
>> I'll need to trust you (that's why I'm asking you to do proper testing).
> 
> With only one condition if dynamic debug is enabled in kernel.
> So, it would be nice to gather opinions, however, the decision is
> totally depends on type of user who wants to debug the module.
> 

Yep, I agree that Jiri's opinion would be helpful.

Meanwhile, we could also wait a little for more i2c-hid to hit the
market and to be widely tested, and then remove the debug flag. We
should also remove the debug events in get_i2c_hid_get_input() which
would pollute systems without dynamic debugging but with the DEBUG
config still enabled.

Cheers,
Benjamin