linux-kernel - Re: [PATCH 2/2] i2c: designware: abort the transfer if receiving byte count of 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:   Thu, 17 Aug 2023 16:42:05 +0200
From:   Yann Sionneau <yann@...nneau.net>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
        Yann Sionneau <ysionneau@...ray.eu>,
        Jonathan Borne <jborne@...ray.eu>
Subject: Re: [PATCH 2/2] i2c: designware: abort the transfer if receiving byte
 count of 0

Hi,

Le 11/08/2023 à 16:01, Andy Shevchenko a écrit :
> On Fri, Aug 11, 2023 at 02:46:24PM +0200, Yann Sionneau wrote:
>> From: Yann Sionneau <ysionneau@...ray.eu>
>>
>> Context:
>> It's not clear whether Linux SMBus stack supports receiving 0
>> as byte count for SMBus read data block.
>>
>> Linux supports SMBus v2.0 spec, which says "The byte count may not be 0."
>> Which does not seem very clear to me, as a non-native speaker.
>> (Note that v3.0 of the spec says "The byte count may be 0.")
>>
>> Some drivers explicitly return -EPROTO in case of byte count 0.
>>
>> The issue:
>> Regardless of whether Linux supports byte count of 0, if this happens
>> the i2c-designware driver goes into an unrecoverable state holding
>> SCL low if the IP is synthesized with the IC_EMPTYFIFO_HOLD_MASTER_EN
>> parameter.
>>
>> The fix proposed by this patch:
>> Abort the transfer by sending a STOP condition on the bus.
>>
>> Another approach would be to ignore the issue and let the driver
>> timeout and disable the IP. The IP disabling is fixed by the previous
>> patch in this patch series.
>> The current patch just makes the recovery faster since Abort is sent
>> directly without waiting for the timeout to happen. With this patch,
>> disabling the IP is not necessary anymore.
> ...
>
>> +				if ((tmp <= I2C_SMBUS_BLOCK_MAX) && (tmp != 0))
> 				if (tmp && tmp <= I2C_SMBUS_BLOCK_MAX)

I find the tmp != 0 "more obvious", I am more used to "just tmp" when 
it's a pointer or a boolean, but maybe it's just me!

I'll fix that in the V2 :)

>
>> +					len = i2c_dw_recv_len(dev, tmp);
>> +				else
>> +					i2c_dw_abort(dev);
Thanks!