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]

Date:	Sat, 13 Feb 2016 13:21:25 +0000
From:	Jonathan Cameron <jic23@...nel.org>
To:	"Andrew F. Davis" <afd@...com>, Hartmut Knaack <knaack.h@....de>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Peter Meerwald <pmeerw@...erw.net>,
	Marc Titinger <mtitinger@...libre.com>
Cc:	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: ina2xx: Remove trace_printk debug statments

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.

What do others think about this change?

Andrew, what is driving your wish to change this?

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();
>  
>