linux-kernel - Re: [2/5] i2c: davinci: query STP always when NACK is received

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:	Mon, 24 Nov 2014 15:34:35 +0200
From:	Grygorii Strashko <grygorii.strashko@...com>
To:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
CC:	Wolfram Sang <wsa@...-dreams.de>, <linux-i2c@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, Sekhar Nori <nsekhar@...com>,
	Kevin Hilman <khilman@...prootsystems.com>,
	Santosh Shilimkar <ssantosh@...nel.org>,
	Murali Karicheri <m-karicheri2@...com>
Subject: Re: [2/5] i2c: davinci: query STP always when NACK is received

Hi Uwe,
On 11/23/2014 10:33 PM, Uwe Kleine-König wrote:
> On Fri, Nov 21, 2014 at 05:33:37PM +0200, Grygorii Strashko wrote:
>> On 11/21/2014 03:10 PM, Uwe Kleine-König wrote:
>>> On Fri, Nov 21, 2014 at 02:48:57PM +0200, Grygorii Strashko wrote:
>>>> On 11/21/2014 12:19 AM, Uwe Kleine-König wrote:
>>>>>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>>>>>> index 9bbfb8f..2cef115 100644
>>>>>> --- a/drivers/i2c/busses/i2c-davinci.c
>>>>>> +++ b/drivers/i2c/busses/i2c-davinci.c
>>>>>> @@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>>>>>>     	if (dev->cmd_err & DAVINCI_I2C_STR_NACK) {
>>>>>>     		if (msg->flags & I2C_M_IGNORE_NAK)
>>>>>>     			return msg->len;
>>>>>> -		if (stop) {
>>>>>> -			w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>>>>>> -			w |= DAVINCI_I2C_MDR_STP;
>>>>>> -			davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>>>>>> -		}
>>>>>> +		w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>>>>>> +		w |= DAVINCI_I2C_MDR_STP;
>>>>>> +		davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>>>>> I think this is a good change, but I wonder if the handling of
>>>>> I2C_M_IGNORE_NAK is correct here. If the controller reports a NACK say
>>>>> for the 2nd byte of a 5-byte-message, the transfer supposed to
>>>>> continue, right? (Hmm, maybe the framework handle this and restarts the
>>>>> transfer with I2C_M_NOSTART but the davinci driver doesn't seem to
>>>>> handle this flag?)
>>>>
>>>> Have nothing to say about handling of I2C_M_IGNORE_NAK. I'm not going to
>>>> change current behavior - davinci driver will interrupt transfer of i2c_msg always
>>>>    in case of NACK and start transfer of the next i2c_msg (if exist).
>>>> In my opinion, Above question is out of scope of this patch.
>>> Yeah right, that's exactly what I thought.
>>>
>>> Thinking again I wonder if with your change handling is correct when the
>>> sender wants to do a repeated start. That would need a more detailed
>>> look into the driver.
>>
>> Davinci driver will always abort transfer with error -EREMOTEIO in case if
>> NACK received from I2C slave device. And the next omap_i2c_xfer() call may
>> be *not* targeted to the same I2C slave device.
>> ^ if !I2C_M_IGNORE_NAK
> Does this resolve my concern? I think it doesn't. Also a Sr might well
> address another device, doesn't it?
> 
> A call to .master_xfer with a message sequence implicitly expects ACKs
> from the slave and doesn't tell anything about what should be done on a
> NAK. So IMHO you must not send a P when the slave responds with a NAK,
> but error out and let the sender decide if it wants to reply with P or
> Sr.

Sry, but what should be done is defined by I2C/SMbus specs? Does it?
For SMBus devices, the specification states (http://smbus.org/specs/)
"4.2.Acknowledge (ACK) and not acknowledge (NACK)":
- "The slave device detects an invalid command or invalid data. In this 
  case the slave device must not acknowledge the received byte. The master
  upon detection of this condition must generate a STOP condition and
  retry the transaction"
For I2C devices, the specification states [http://www.nxp.com/documents/user_manual/UM10204.pdf]:
"3.1.6 Acknowledge (ACK) and Not Acknowledge (NACK)"
"When SDA remains HIGH during this ninth clock pulse, this is defined as the Not
Acknowledge signal. The master can then generate either a STOP condition to
abort the transfer, or a repeated START condition to start a new transfer."

Let take a look on i2c/smbus xfer:
i2c_lock_adapter(adap)
 adap->algo->master_xfer/smbus_xfer()
i2c_unlock_adapter(adap);
 |- rt_mutex_unlock(&adapter->bus_lock);
    |- task switch
 
So, there is no guarantee that next xfer will address the same I2C client device,
which, in turn, may lead to BB detection (will lead to BB detection if previous
transfer has been not acknowledged by SMbus client device).

Small summary, I2C core + Davinci I2C driver provide ability to use repeated
start (Sr) only within one I2C transaction - which is a number of write/read
operations specified by i2c_msg array. NACK always interrupts transaction
with -EREMOTEIO.

Also, the I2C core doesn't provide ability to manually send P.

regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/