linux-kernel - Re: [PATCH v5] iio: add ad5761 DAC driver

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] [day] [month] [year] [list]

Date:	Mon, 11 Jan 2016 19:21:05 +0000
From:	Jonathan Cameron <jic23@...nel.org>
To:	Ricardo Ribalda Delgado <ricardo.ribalda@...il.com>
Cc:	Lars-Peter Clausen <lars@...afoo.de>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Hartmut Knaack <knaack.h@....de>,
	Peter Meerwald <pmeerw@...erw.net>, linux-iio@...r.kernel.org,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5] iio: add ad5761 DAC driver

On 11/01/16 16:23, Ricardo Ribalda Delgado wrote:
> Hello Jonathan
> 
> Thanks for your review
> 
> On Sat, Jan 9, 2016 at 5:15 PM, Jonathan Cameron <jic23@...nel.org> wrote:
>> I'm a little unsure that we shouldn't just fail the probe if the
>> range is supplied.  Even the default (the best option available)
>> could in theory do damage to a circuit with a maximum of 3V though
>> it's probably unlikely...
> 
> I personally hate when a driver needs a mandatory pdata. I expect a
> default behaviour on the absence of it.
That's fair enough, though in this particular case, in theory it could
cause damage.  Let's take the view that's unlikely.
> 
>>> +     st->vref_reg = devm_regulator_get_optional(&st->spi->dev, "vref");
>>> +     if (!st->vref_reg || PTR_ERR(st->vref_reg) == -ENODEV) {
>> This is a little unusual... Only one of those errors is returned by
>> devm_regulator_get_optional if the regulator is not specified.
>> I believe from a quick look at the regulator code that it returns the
>> -ENODEV part.
>>
>> So how can it be null?
>>
> 
> After a fast look:
> 
> devm_regulator_get_optional-> _devm_regulator_get -> regulator_get
> (regulator/consumer.h)
> 
> Probably consumer.h cannot be reached at the same time than
> _devm_regulator_get. But for safety I rather leave the check.
Hmm. In this particular case it's relatively clear that one of
the checks must be meaningless so chances of patches turning up
to 'fix' this in future makes me think it's better to save time
and drop it now.
> 
> 
> 
> I send v6 right away.
> 
> 
> Thanks!
> 
> 
>