linux-kernel - Re: [PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0"

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, 08 Dec 2018 00:07:21 +0530
From:   Shreeya Patel <shreeya.patel23498@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>,
        Jeremy Fertic <jeremyfertic@...il.com>
Cc:     devel@...verdev.osuosl.org, lars@...afoo.de,
        Michael.Hennerich@...log.com, linux-iio@...r.kernel.org,
        gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        pmeerw@...erw.net, knaack.h@....de, jic23@...nel.org
Subject: Re: [PATCH] Revert "Staging: iio: adt7316: Add an extra check for
 'ret' equals to 0"

On Thu, 2018-12-06 at 15:40 +0300, Dan Carpenter wrote:
> On Wed, Dec 05, 2018 at 02:59:53PM -0700, Jeremy Fertic wrote:
> > On Thu, Dec 06, 2018 at 01:25:55AM +0530, Shreeya Patel wrote:
> > > On Tue, 2018-12-04 at 18:49 -0700, Jeremy Fertic wrote:
> > > > This reverts commit 00426e99789357dbff7e719a092ce36a3ce49d94.
> > > > 
> > > > i2c_smbus_read_byte() returns 0 when a byte with the value 0 is
> > > > read
> > > > from
> > > > the device. This is a valid read so revert the check for 0.
> > > > 
> > > > Signed-off-by: Jeremy Fertic <jeremyfertic@...il.com>
> > > > ---
> > > 
> > > Hi Jeremy,
> > > 
> > > As per my understanding, 0 value indicates no error but no data
> > > read.
> > > Then how can this be a valid case?
> > > 
> > > Can you please make me understand that how can we consider this
> > > as a
> > > valid case even when no data has been read?
> 
> It's not reading no data.  It's reading one byte of data and
> returning
> it.
> 
> > > 
> > > 
> > > Thanks
> > 
> > I'm not sure I understand why the value 0 would indicate no data
> > read.
> > Doesn't that just mean a byte was read with the value 0.
> 
> Yes.  It does mean that.  Please don't ask rhetorical
> questions...  :(
> This list is full of people who can't resist answering every
> question.
> 
> > For instance, if the input to the adc is 0V. Can you point me to
> > where
> > you're seeing that this would indicate no data read?
> 
> drivers/i2c/i2c-core-smbus.c
>     88  /**
>     89   * i2c_smbus_read_byte - SMBus "receive byte" protocol
>     90   * @client: Handle to slave device
>     91   *
>     92   * This executes the SMBus "receive byte" protocol, returning
> negative errno
>     93   * else the byte received from the device.
>     94   */
>     95  s32 i2c_smbus_read_byte(const struct i2c_client *client)
>     96  {
>     97          union i2c_smbus_data data;
>     98          int status;
>     99  
>    100          status = i2c_smbus_xfer(client->adapter, client-
> >addr, client->flags,
>    101                                  I2C_SMBUS_READ, 0,
>    102                                  I2C_SMBUS_BYTE, &data);
>    103          return (status < 0) ? status : data.byte;
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>    104  }
>    105  EXPORT_SYMBOL(i2c_smbus_read_byte);

Even I had sent the same code to Jonathan and we had a discussion on
this. 
I asked him that this code clearly shows that there is an error
condition only when status < 0 then why do we need a check for status =
0.

Then he explained me that 0 isn't an error. The issue is the silliness
of the i2c interface.

Pretty much every other bus returns an error (negative) if less data is
received than expected.  Most i2c
bus master's do as well but in theory it can return 0 to indicate no
error but no data read (which doesn't make any sense)

0 doesn't ever happen in reality but it should be handled for
correctness though.

So we should wait for what Jonathan has to say on this :)

Thanks

> You are right.  Commit 00426e997893 ("Staging: iio: adt7316: Add an
> extra check for 'ret' equals to 0") needs to be reverted...
> 
> regards,
> dan carpenter
> 
>