linux-kernel - Re: [PATCH 2/2] ti-adc081c: Initial triggered buffer support

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:	Fri, 1 Apr 2016 14:13:50 +0300
From:	Leonard Crestez <leonard.crestez@...el.com>
To:	Peter Meerwald-Stadler <pmeerw@...erw.net>
Cc:	Jonathan Cameron <jic23@...nel.org>, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org, Hartmut Knaack <knaack.h@....de>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Daniel Baluta <daniel.baluta@...el.com>
Subject: Re: [PATCH 2/2] ti-adc081c: Initial triggered buffer support

On 04/01/2016 11:34 AM, Peter Meerwald-Stadler wrote:
>> -static const struct iio_chan_spec adc081c_channel = {
>> -	.type = IIO_VOLTAGE,
>> -	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> -};
>
> the patch would look cleaner/shorter if adc081c_channel won't get moved
> around

It was not moved around, it is now defined by a macro. Buffer support 
requires defining scan_type which contains a different number of bits.

The macros are now after iio_info adc081c_info instead of before, I can 
move that around. I also noticed that I declared both a struct 
adcxx1c_model and an unused struct adcxx1c_info. I will remove that.

The first patch in the series doesn't use any per-model data and just 
stores the number of bits in driver_data. I can change the series to 
introduce adcxx1c_model in the first patch.

>> +static irqreturn_t adc081c_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct adc081c *data = iio_priv(indio_dev);
>> +	s64 ts;
>> +	u16 buf[8];
>
> comment: 2 bytes data + 6 bytes padding + 8 bytes timestamp
>
>> +	int ret;
>> +
>> +	/* Otherwise iio_push_to_buffers will corrupt the stack. */
>> +	if (indio_dev->scan_bytes > sizeof(buf)) {
>> +		dev_crit_once(&indio_dev->dev, "Bad iio_scan_bytes=%d > %d\n",
>> +				indio_dev->scan_bytes, (int)sizeof(buf));
>
> rather than casting sizeof(buf), use the correct printf length modifier,
> i.e. %z
>
> not sure if this check is needed

I guess it's not needed. My first version defined the buffer incorrectly 
and caused a messy crash. Calculating manual buffer alignments seems 
very fragile.

It seems that C99 variable-length-arrays work fine, something like:
	u16 buf[indio_dev->scan_bytes / 2];

Would that be acceptable? It compiles without warnings and there some 
other places in the kernel where VLAs are used.

>> +	ret = i2c_smbus_read_word_swapped(data->i2c, REG_CONV_RES);
>
> REG_CONV_RES should be called ADC081C_REG_CONV_RES, but that's a separate
> issue

Yes, but that would be an entirely unrelated renaming.

>
>> +	ts = iio_get_time_ns();
>
> why is the timestamp taken here?, seems strange
> often this is done together with iio_push_to_buffers_with_timestamp()

I wanted to keep it as close to the read as possible. In this case it 
doesn't matter.

>> -	iio->channels = &adc081c_channel;
>> -	iio->num_channels = 1;
>> +	iio->channels = model->channels;
>> +	iio->num_channels = 2;
>
> the number of channels could go into the adcxx1c_info struct

But it's a constant, it does not vary between devices. I could make an 
ADC081C_NUM_CHANNELS define.

--
Regards,
Leonard