linux-kernel - Re: [PATCH 2/2] iio: ina2xx: Remove trace

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <56C19599.8010003@baylibre.com>
Date:	Mon, 15 Feb 2016 10:08:41 +0100
From:	Marc Titinger <mtitinger@...libre.com>
To:	"Andrew F. Davis" <afd@...com>,
	Jonathan Cameron <jic23@...nel.org>,
	Hartmut Knaack <knaack.h@....de>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Peter Meerwald <pmeerw@...erw.net>
Cc:	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: ina2xx: Remove trace_printk debug statments



On 14/02/2016 21:02, Andrew F. Davis wrote:
> On 02/13/2016 07:21 AM, Jonathan Cameron wrote:
>> On 12/02/16 18:34, Andrew F. Davis wrote:
>>> These are generally for devlopment use only, remove these
>>> from performance-critical code, convert to dev_dbg elswhere.
>>>
>>> Signed-off-by: Andrew F. Davis <afd@...com>
>> Hm... Tracepoints are also somewhat considered to be ABI and
>> hence it is possible some tooling relies on them.  Also they
>> are very nearly free when not enabled.
>>
>> The fundamental reason they are here it to allow checking of whether
>> the thread is ticking along fast enough to keep up with the incoming 
>> data.
>> Can see this being useful on live platforms to debug sampling issues.
>>
>
> This looks more like development testing statements to see if the 
> delay timer
> adjustment algorithm is working well. If one is debugging sampling 
> issues,
> then they are always free to add any extra debugging lines they need.
Hi Andrew,

I've left it in in case I would get feedback with different i2c 
controllers than the one of Omap
I've tested against, and would need to rework the mechanics of the data 
capture (for instance if the i2c
message completion code did not use a threaded irq or stuff like that).
I guess the trace stuff this can be remove for now.

BR,
Marc.

>
>> What do others think about this change?
>>
>> Andrew, what is driving your wish to change this?
>>
>
> v4.5-rc3 include/linux/kernel.h +609:
>
> "This is intended as a debugging tool for the developer only.
> Please refrain from leaving trace_printks scattered around in
> your code."
>
>> Jonathan
>>> ---
>>>   drivers/iio/adc/ina2xx-adc.c | 21 +++++++--------------
>>>   1 file changed, 7 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/ina2xx-adc.c 
>>> b/drivers/iio/adc/ina2xx-adc.c
>>> index 61e8ae9..ba11b2e 100644
>>> --- a/drivers/iio/adc/ina2xx-adc.c
>>> +++ b/drivers/iio/adc/ina2xx-adc.c
>>> @@ -440,7 +440,6 @@ static int ina2xx_work_buffer(struct iio_dev 
>>> *indio_dev)
>>>       struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>>       unsigned short data[8];
>>>       int bit, ret, i = 0;
>>> -    unsigned long buffer_us, elapsed_us;
>>>       s64 time_a, time_b;
>>>       unsigned int alert;
>>>
>>> @@ -464,8 +463,6 @@ static int ina2xx_work_buffer(struct iio_dev 
>>> *indio_dev)
>>>                   return ret;
>>>
>>>               alert &= INA266_CVRF;
>>> -            trace_printk("Conversion ready: %d\n", !!alert);
>>> -
>>>           } while (!alert);
>>>
>>>       /*
>>> @@ -490,14 +487,9 @@ static int ina2xx_work_buffer(struct iio_dev 
>>> *indio_dev)
>>>       iio_push_to_buffers_with_timestamp(indio_dev,
>>>                          (unsigned int *)data, time_a);
>>>
>>> -    buffer_us = (unsigned long)(time_b - time_a) / 1000;
>>> -    elapsed_us = (unsigned long)(time_a - chip->prev_ns) / 1000;
>>> -
>>> -    trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, 
>>> buffer_us);
>>> -
>>>       chip->prev_ns = time_a;
>>>
>>> -    return buffer_us;
>>> +    return (unsigned long)(time_b - time_a) / 1000;
>>>   };
>>>
>>>   static int ina2xx_capture_thread(void *data)
>>> @@ -532,12 +524,13 @@ static int ina2xx_buffer_enable(struct iio_dev 
>>> *indio_dev)
>>>       struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>>       unsigned int sampling_us = SAMPLING_PERIOD(chip);
>>>
>>> -    trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg 
>>> =%u\n",
>>> -             (unsigned int)(*indio_dev->active_scan_mask),
>>> -             1000000/sampling_us, chip->avg);
>>> +    dev_dbg(&indio_dev->dev, "Enabling buffer w/ scan_mask %02x, 
>>> freq = %d, avg =%u\n",
>>> +        (unsigned int)(*indio_dev->active_scan_mask),
>>> +        1000000 / sampling_us, chip->avg);
>>>
>>> -    trace_printk("Expected work period: %u us\n", sampling_us);
>>> -    trace_printk("Async readout mode: %d\n", 
>>> chip->allow_async_readout);
>>> +    dev_dbg(&indio_dev->dev, "Expected work period: %u us\n", 
>>> sampling_us);
>>> +    dev_dbg(&indio_dev->dev, "Async readout mode: %d\n",
>>> +        chip->allow_async_readout);
>>>
>>>       chip->prev_ns = iio_get_time_ns();
>>>
>>>
>>