linux-kernel - Re: [PATCH] staging: pi433: rf69.c: Introduced define DEBUG_FUNC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 2 Dec 2017 19:00:17 +0200
From:   Marcus Wolf <marcus.wolf@...f-entwicklungen.de>
To:     Greg KH <gregkh@...uxfoundation.org>,
        Marcus Wolf <linux@...f-entwicklungen.de>
Cc:     dan.carpenter@...cle.com, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: pi433: rf69.c: Introduced define
 DEBUG_FUNC_ENTRY

Am 02.12.2017 um 17:00 schrieb Greg KH:
> On Sat, Dec 02, 2017 at 01:45:50PM +0200, Marcus Wolf wrote:
>> Since dev_dbg already depends on define DEBUG, there was no sense, to enclose
>> dev_dbg lines with #ifdef DEBUG.
>> Instead of removing #ifdef DEBUG, I introduced define DEBUG_FUNC_ENTRY. So now it is
>> possible to switch of the function entry debug lines even while debug is switched on.
> 
> Ick ick ick.
> 
> No, these lines should just all be deleted.  Use ftrace if you want to
> see what functions are being called, that's what it is there for.  Don't
> create driver-specific defines and functionality for when we already
> have that functionality for the whole of the kernel.  That's really
> redundant and messy.
> 
>> Signed-off-by: Marcus Wolf <linux@...f-entwicklungen.de>
>> ---
>>   drivers/staging/pi433/rf69.c |  118 +++++++++++++++++++++---------------------
>>   1 file changed, 58 insertions(+), 60 deletions(-)
>>
>> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
>> index 12c9df9..0df084e 100644
>> --- a/drivers/staging/pi433/rf69.c
>> +++ b/drivers/staging/pi433/rf69.c
>> @@ -15,8 +15,10 @@
>>    * GNU General Public License for more details.
>>    */
>>   
>> -/* enable prosa debug info */
>> +/* generic enable/disable dev_dbg */
>>   #undef DEBUG
>> +/* enable print function entry */
>> +#undef DEBUG_FUNC_ENTRY
>>   /* enable print of values on reg access */
>>   #undef DEBUG_VALUES
>>   /* enable print of values on fifo access */
>> @@ -40,7 +42,7 @@
>>   
>>   int rf69_set_mode(struct spi_device *spi, enum mode mode)
>>   {
>> -	#ifdef DEBUG
>> +	#ifdef DEBUG_FUNC_ENTRY
>>   		dev_dbg(&spi->dev, "set: mode");
>>   	#endif
> 
> So this whole #ifdef dev_dbg #endif should all be removed.
> 
> thanks,
> 
> greg k-h
> 

Hi all,

just for clarification, why I introduced these dev_dbg during 
development of the driver and didn't use ftrace:

Since I wanted the driver to use a single module as sender and receiver 
at (almost) the same time, the module constantly needs to be 
reconfigured (constant switching between rx configuration and tx 
configuration - see my documentation for details on the idea).

The routine, accessing the registers is able to print the register 
number and the value, it reads / writes from / to the register. It's dam 
hard to keep the survey over the use 30...40 register numbers, in 10th 
of rows of register setting and reading, if you see just the numbers in 
the log. Especially this is / was hard, if one register was written 
several times, because it contains different settings. Then decoding of 
the adress wasn't enough, I even need to decode the bits in the value.

Therefore I finally introduced this dev_dbg lines at the enty of the 
setter (and getter): After that in the log I coud see something like this:
Set gain: 0x43 0x81
Set threshold: 0x51 0x30
Get moulation: 0x22 0x7c
instead of just numbers.
That eased the debugging a lot.

When using ftrace I would be able to see, in which order the setter were 
called, but the link to the vlaues would be missing.

If those dev_dbg are unwanted, I am ok, if someone removes them.

Hope this helps understanding....

Marcus