linux-kernel - Re: [RFC] i2c-algo-bit: Disable interrupts while SCL is high

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]

Message-ID: <20101218000924.546ad703@endymion.delvare>
Date:	Sat, 18 Dec 2010 00:09:24 +0100
From:	Jean Delvare <khali@...ux-fr.org>
To:	Michael Lawnick <ml.lawnick@....de>
Cc:	Ben Dooks <ben-i2c@...ff.org>,
	Linux I2C <linux-i2c@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Matthias Zacharias <Matthias.Zacharias@...-solutions.de>
Subject: Re: [RFC] i2c-algo-bit: Disable interrupts while SCL is high

Hi Michael,

On Fri, 17 Dec 2010 13:09:54 +0100, Michael Lawnick wrote:
> Jean Delvare said the following:
> > Hi Ben,
> > 
> > On Thu, 16 Dec 2010 16:00:46 +0000, Ben Dooks wrote:
> >> On Thu, Dec 16, 2010 at 03:06:38PM +0100, Jean Delvare wrote:
> >> > Add a spinlock to every user of i2c-algo-bit, which is taken before
> >> > raising SCL and released after lowering SCL. We don't really need
> >> > the exclusion functionality, but we have to disable local interrupts.
> >> > This is needed to comply with SMBus requirements that SCL shouldn't
> >> > be high for longer than 50 us.
> >> > 
> >> > SMBus slaves can consider SCL being high for 50 us as a timeout
> >> > condition. This has been observed to happen reproducibly with the
> >> > Melexis MLX90614.
> >> > 
> >> > The drawback of this approach is that spin_lock_irqsave() and
> >> > spin_unlock_irqrestore() will be called once for each bit going on the
> >> > I2C bus in either direction. This can mean up to 100 kHz for standard
> >> > I2C and SMBus and up to 250 kHz for fast I2C. The good thing is that
> >> > this limits the latency to reasonable values (2us at 250 kHz, 5 us at
> >> > 100 kHz and 50 us at 10 kHz).
> >> 
> >> Hmm, this is going to be a drain on interrupt latency... disabling
> >> interrupts in a system for that long could cause other things to
> >> jitter.
> > 
> > So you consider that even disabling interrupts for 5 us is too long? Or
> > are you only worried by the 50 us case?
> 
> Sorry to disturb, but
> <MANTRA>
> 	Disabling interrupts may be done only for a few instructions.</MANTRA>
> 
> Even 1 us is an eternity on modern systems.

Don't be sorry, this is exactly the kind of input I was asking for. I'm
a little surprised, I thought disabling interrupts for a couple
microseconds was happening all the time, but I'll trust your
experience. Given your point and Ben's, it seems clear that my patch is
not acceptable as is, and at the very least I should make the spinlock
usage optional.

High-resolution timers may be an option too, but I guess it will
require a rewrite of the driver, and also I don't think HR timers are
available everywhere, so we will have to keep the old code in place for
compatibility.

Matthias, can you please tell us whether your system supports
high-resolution timers? I need to know if that would be a viable
solution for you.

-- 
Jean Delvare
--
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/