Hi Andy,
> >> > -#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.
>
> Thanks for explanation why. (it would be good partially move this to
> the commit message).
We will improve the commit message in the next revision of the patch.
> >> > +#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.
>
> No way. It's a bad style when you have a macro like you proposing. It
> would give you a bottle of sparkling bugs.
>
> > What does "keel it's semantics" mean?
>
> Macro should be standalone piece of code which does something from A
> to Z, not from A-K when you need a complementary macro to do L-Z
> parts.
I agree, we will do it without a macro then.
> > 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))
>
> Yes, though, please, look at the examples in the existing code and
> make it slightly better
>
> timeout = ...
> do {
> ret = ...
> if (ret) // or if (!ret)
> ...
>
> usleep_range(...);
> } while(time_before(...));
When working on the problem we had an intermediate result were we
came to the same solution as your proposal showed:
tout = jiffies + msecs_to_jiffies(at24_write_timeout);
do {
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;
usleep_range(1000, 1500);
} while (!time_before(tout, read_time))
The advantage of this code is that the usleep_range() is unconditional.
(In my older proposal a "if (ret)" condition is required to make sure
that there is not sleep at the very first iteration but only at
follow-up iterations where regmap_bulk_read() failed.)
The disadvantage of the new proposal is that in case of a timeout one
more unnecessary sleep is made. Is that acceptable?
An alternative would be to duplicate the regmap_bulk_read() and the
debugging code outside the loop.
tout = jiffies + msecs_to_jiffies(at24_write_timeout);
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);
while (ret && !time_before(tout, read_time))
usleep_range(1000, 1500);
/*
* The timestamp shall be taken before sleep and the actual
* operation to avoid a premature timeout in case of high CPU load.
*/
read_time = jiffies;
ret = regmap_bulk_read(regmap, offset, buf, count);
dev_dbg(&client->dev, "read retry %zu@%d --> %d (%ld)\n",
count, offset, ret, jiffies);
}
if (!ret)
return count;
Is this preferable?
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
