linux-kernel - Re: [PATCH 04/11] staging: iio: adt7316: fix handling of dac high resolution option

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:   Fri, 14 Dec 2018 14:29:44 -0700
From:   Jeremy Fertic <jeremyfertic@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     devel@...verdev.osuosl.org, Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        linux-iio@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Hartmut Knaack <knaack.h@....de>,
        Jonathan Cameron <jic23@...nel.org>
Subject: Re: [PATCH 04/11] staging: iio: adt7316: fix handling of dac high
 resolution option

On Fri, Dec 14, 2018 at 09:26:18AM +0300, Dan Carpenter wrote:
> On Thu, Dec 13, 2018 at 03:01:46PM -0700, Jeremy Fertic wrote:
> > On Wed, Dec 12, 2018 at 11:23:16AM +0300, Dan Carpenter wrote:
> > > On Tue, Dec 11, 2018 at 05:54:56PM -0700, Jeremy Fertic wrote:
> > > > @@ -651,10 +649,12 @@ static ssize_t adt7316_store_da_high_resolution(struct device *dev,
> > > >  	u8 config3;
> > > >  	int ret;
> > > >  
> > > > +	if (chip->id == ID_ADT7318 || chip->id == ID_ADT7519)
> > > > +		return -EPERM;
> > > 
> > > return -EINVAL is more appropriate than -EPERM.
> > > 
> > > regards,
> > > dan carpenter
> > > 
> > 
> > I chose -EPERM because the driver uses it quite a few times in similar
> > circumstances.
> 
> Yeah.  I saw that when I reviewed the later patches in this series.
> 
> It's really not doing it right.  -EPERM means permission checks like
> access_ok() failed so it's not appropriate.  -EINVAL is sort of general
> purpose for invalid commands so it's probably the correct thing.
> 
> > At least with this driver, -EINVAL is used when the user
> > attempts to write data that would never be valid. -EPERM is used when
> > either the current device settings prevent some functionality from being
> > used, or the device never supports that functionality. This patch is the
> > latter, that these two chip ids never support this function.
> > 
> > I'll change to -EINVAL in a v2 series, but I wonder if I should hold off
> > on a separate patch for other instances in this driver since it will be
> > undergoing a substantial refactoring.
> 
> Generally, you should prefer kernel standards over driver standards and
> especially for staging.  But it doesn't matter.  When I reviewed this
> patch, I hadn't seen that the driver was doing it like this but now I
> know so it's fine.  We can clean it all at once later if you want.
> 
> regards,
> dan carpenter
> 

I'll wait to deal with these error values since some of them might go away
with all the changes necessary to get the driver out of staging. Thanks
for clarifying things for me.