linux-kernel - Re: [PATCH] eeprom: at24: Fix unexpected timeout under high load

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-next>] [day] [month] [year] [list]

Message-ID: <cd8f37876a2e466ea066bcad10b0b8e4@de.bosch.com>
Date:   Sun, 5 Aug 2018 12:26:47 +0000
From:   "Jonas Mark (BT-FIR/ENG1)" <Mark.Jonas@...bosch.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Bartosz Golaszewski <brgl@...ev.pl>
CC:     Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-i2c <linux-i2c@...r.kernel.org>,
        "Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
        "WANG Xin (BT-FIR/ENG1-Zhu)" <Xin.Wang7@...bosch.com>,
        "Jonas Mark (BT-FIR/ENG1)" <Mark.Jonas@...bosch.com>
Subject: Re: [PATCH] eeprom: at24: Fix unexpected timeout under high load

Hi Andy,

Thank you for your feedback.
 
> > -#define at24_loop_until_timeout(tout, op_time)                         \
> > -       for (tout = jiffies + msecs_to_jiffies(at24_write_timeout),     \
> > -            op_time = 0;                                               \
> > -            op_time ? time_before(op_time, tout) : true;               \
> > -            usleep_range(1000, 1500), op_time = jiffies)
> 
> This one understandble and represents one operation.

It just has the downside that it will not retry if the timeout is
reached after the usleep_range().

If you have a system which combines high CPU load with repeated EEPROM
writes you will run into the following scenario:

- System makes a successful regmap_bulk_write() to EEPROM.
- System wants to perform another write to EEPROM but EEPROM is still
  busy with the last write.
- Because of high CPU load the usleep_range() will sleep more than
  25 ms (at24_write_timeout).
- Within the over-long sleeping the EEPROM finished the previous write
  operation and is ready again.
- at24_loop_until_timeout() will detect timeout and won't try to write.

The scenario above happens very often on our system and we need a fix.

> > +#define at24_loop_until_timeout_begin(tout, op_time)           \
> > +       tout = jiffies + msecs_to_jiffies(at24_write_timeout);  \
> > +       while (true) {                                          \
> > +               op_time = jiffies;
> > +
> > +#define at24_loop_until_timeout_end(tout, op_time)             \
> > +               if (time_before(tout, op_time))                 \
> > +                       break;                                  \
> > +               usleep_range(1000, 1500);                       \
> > +       }
> 
> Besides `while (true)`, which is a red flag for timeout loops,
> these are done in an hack way. Just open code them in both cases, or
> rewrite original one to keel it's semantics.

I have to admit that I am not sure what you mean.

We kept the macro-style of the loop because we assumed it was good
style in this context.

What does "keel it's semantics" mean?

With "open code them in both cases" do you mean to rid of the macro
and to directly write the loop into the code? Does the following
match your proposals?

ret = 0;
tout = jiffies + msecs_to_jiffies(at24_write_timeout);
do {
	if (ret)
		usleep_range(1000, 1500);

	read_time = jiffies;

	ret = regmap_bulk_read(regmap, offset, buf, count);
	dev_dbg(&client->dev, "read %zu@%d --> %d (%ld)\n",
		count, offset, ret, jiffies);
	if (!ret)
		return count;
} while (!time_before(tout, read_time))

Greetings,
Mark

Building Technologies, Panel Software Fire (BT-FIR/ENG1) 
Bosch Sicherheitssysteme GmbH | Postfach 11 11 | 85626 Grasbrunn | GERMANY | www.boschsecurity.com

Sitz: Stuttgart, Registergericht: Amtsgericht Stuttgart HRB 23118 
Aufsichtsratsvorsitzender: Stefan Hartung; Geschäftsführung: Tanja Rückert, Andreas Bartz, Thomas Quante, Bernhard Schuster