Adjust the NOR CFI flash timeouts to round better
PATCH 1/1 Adjust the NOR CFI flash timeouts to round better
Using jiffies for timeout could have significant rounding, ie if we start at jiffie 1.9 and timeout at 2 then we may only get .1 jiffies of timeout, which is not nearly enough. With a Hz of 250 I've seen random bogus timeouts from this code.
Add a +1 to the HZ calculations to ensure a guaranteed minimum timeout period.
This also increases the write_buffer timeout to 1.5us since I have flashes in use that have a datasheet max of over 1 us.
--- drivers/mtd/chips/cfi_cmdset_0002.c | 13 ++++++++----- 1 files changed, 8 insertions(+), 5 deletions(-)
drivers/mtd/chips/cfi_cmdset_0002.c +8 -5
@@ -1150,10 +1150,10 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,* use the maximum timeout value given by the chip at probe time * instead. Unfortunately, struct flchip does have a field for * maximum timeout, only for typical which can be far too short -* depending of the conditions. The ' + 1' is to avoid having a -* timeout of 0 jiffies if HZ is smaller than 1000. +* depending of the conditions. The ' + 2' is to ensure we get +* *at least* 1000us of timeout. */ -unsigned long uWriteTimeout = (HZ /1000 ) +1 ; +unsigned long uWriteTimeout = (HZ /1000 ) +2 ;int ret =0 ;map_word oldd;int retry_cnt =0 ;
@@ -1382,8 +1382,11 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, {struct cfi_private *cfi = map->fldrv_priv;unsigned long timeo = jiffies +HZ ; -/* see comments in do_write_oneword() regarding uWriteTimeo. */ -unsigned long uWriteTimeout = (HZ /1000 ) +1 ; +/* see comments in do_write_oneword() regarding uWriteTimeo. +Note: write_buffer commands take longer so we use a higher +time. The AMD 29LV256M for instance has a datasheet max +of 1.2ms for page and 600us for byte */ +unsigned long uWriteTimeout = (HZ /1500 ) +2 ;int ret = -EIO ;unsigned long cmd_adr;int z, words;
Signature · 1 line
1.7.5.4
drivers/mtd/chips/cfi_cmdset_0002.c static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, · line 1389
Quoted 7 lines · Jason Gunthorpe
drivers/mtd/chips/cfi_cmdset_0002.c +5
-2-/* see comments in do_write_oneword() regarding uWriteTimeo. */ -unsigned long uWriteTimeout = (HZ /1000 ) +1 ; +/* see comments in do_write_oneword() regarding uWriteTimeo. +Note: write_buffer commands take longer so we use a higher +time. The AMD 29LV256M for instance has a datasheet max +of 1.2ms for page and 600us for byte */ +unsigned long uWriteTimeout = (HZ /1500 ) +2 ;
What does HZ / 1500 mean? HZ is amount of timer interrupts per second, which may be 1/100, or 1/1000, or 1/300, depending on the system.
The CFI code seems to assume that HZ is 1/1000, which is wrong. I think the entire timeout strategy in this file should be cleaned up. You should use 'jiffies_to_msecs()' / 'msecs_to_jiffies()' helpers instead of using 'HZ'. And time-outs should be specified in msecs, not in jiffies. There should be no things like 'HZ/1500'.
Or do I misunderstand something?
Signature · 2 lines
Best Regards, Artem Bityutskiy
Quoted 11 lines · Artem Bityutskiy
drivers/mtd/chips/cfi_cmdset_0002.c static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, · line 1389
Quoted 7 lines · Jason Gunthorpe
drivers/mtd/chips/cfi_cmdset_0002.c +5
-2-/* see comments in do_write_oneword() regarding uWriteTimeo. */ -unsigned long uWriteTimeout = (HZ /1000 ) +1 ; +/* see comments in do_write_oneword() regarding uWriteTimeo. +Note: write_buffer commands take longer so we use a higher +time. The AMD 29LV256M for instance has a datasheet max +of 1.2ms for page and 600us for byte */ +unsigned long uWriteTimeout = (HZ /1500 ) +2 ;What does HZ / 1500 mean? HZ is amount of timer interrupts per second, which may be 1/100, or 1/1000, or 1/300, depending on the system.
If HZ is the amount of timer interrupts per second, then HZ/1500 is the amount of timer interrupts in 1/1500 of a second, aka 666.7µs.¹
So it's not *entirely* bogus, but it should certainly be cleaned up to use the msecs_to_jiffies() helpers... or preferably not use jiffies at all, perhaps.
Signature · 4 lines
dwmw2 ¹ That's 'µs', not 'us', for next time the patch is submitted. Welcome to the 21st century.
Quoted 3 lines · David Woodhouse
So it's not *entirely* bogus, but it should certainly be cleaned up to use the
msecs_to_jiffies()helpers... or preferably not use jiffies at all, perhaps.
No doubt this is part of why I've seen failures on my boards, the timeout is too short by half and rounded wrong :)
A quick grep shows many cases in drivers/mtd that use division of HZ, what would you like to see done here?
Jason
?? That's '??s', not 'us', for next time the patch is submitted. Welcome
to the 21st century.
Hmm.. I may need a new terminal and/or mail reader ;)
Quoted 8 lines · Jason Gunthorpe
Quoted 3 lines · David Woodhouse
So it's not *entirely* bogus, but it should certainly be cleaned up to use the
msecs_to_jiffies()helpers... or preferably not use jiffies at all, perhaps.No doubt this is part of why I've seen failures on my boards, the timeout is too short by half and rounded wrong :)
Yeah, the rounding by +2 is confusing. I'm sure it's wrong, but at least I can kind of see where it came from.
Quoted 2 lines
A quick grep shows many cases in
drivers/mtdthat use division of HZ, what would you like to see done here?
I'd like someone with infinite amounts of free time to go through and clean them all up :)
Quoted 4 lines
?? That's '??s', not 'us', for next time the patch is submitted. Welcome
to the 21st century.Hmm.. I may need a new terminal and/or mail reader ;)
As I said... welcome to the 21st century. Seriously though, what are you using that isn't capable of even *preserving* UTF-8 when you reply to it? That's really quite broken, and has been considered so for a decade or more already.
Signature · 1 line
dwmw2
Quoted 9 lines · David Woodhouse
Quoted 6 lines
Quoted 3 lines
So it's not *entirely* bogus, but it should certainly be cleaned up to use the
msecs_to_jiffies()helpers... or preferably not use jiffies at all, perhaps.No doubt this is part of why I've seen failures on my boards, the timeout is too short by half and rounded wrong :)
Yeah, the rounding by +2 is confusing. I'm sure it's wrong, but at least I can kind of see where it came from.
The rounding by +2? I think even if you used msecs_to_jiffies you'd need to add a +1 to the result to make the timeout algorithm work right..
Is there a better way to do these timeouts?
Quoted 5 lines
Quoted 2 lines
A quick grep shows many cases in
drivers/mtdthat use division of HZ, what would you like to see done here?I'd like someone with infinite amounts of free time to go through and clean them all up :)
lol! Would you accept msecs_to_jiffies conversion for the cfi files?
Quoted 9 lines
Quoted 4 lines
?? That's '??s', not 'us', for next time the patch is submitted. Welcome
to the 21st century.Hmm.. I may need a new terminal and/or mail reader ;)
As I said... welcome to the 21st century. Seriously though, what are you using that isn't capable of even *preserving* UTF-8 when you reply to it? That's really quite broken, and has been considered so for a decade or more already.
I deliberately made it not preserve UTF-8 for this posting, it is just rxvt, nobody ever put unicode into it.
Jason
Quoted 3 lines · Jason Gunthorpe
The rounding by +2? I think even if you used
msecs_to_jiffiesyou'd need to add a +1 to the result to make the timeout algorithm work right..
Yes, HZ/1500+1 I could understand. That's the number of jiffies that give you '666µs or more'. The +2 is odd though.
Quoted 4 lines
Quoted 2 lines
I'd like someone with infinite amounts of free time to go through and clean them all up :)
lol! Would you accept
msecs_to_jiffiesconversion for the cfi files?
Absolutely. Although I'd like to take a little step back and take a more thoughtful view of how we *should* be handing these timeouts, rather than a simple janitor-style conversion of the existing code.
Quoted 2 lines
I deliberately made it not preserve UTF-8 for this posting, it is just rxvt, nobody ever put unicode into it.
∃ rxvt-unicode
Signature · 1 line
dwmw2
Quoted 2 lines · David Woodhouse
Yes, HZ/1500+1 I could understand. That's the number of jiffies that give you '666µs or more'. The +2 is odd though.
Looking more closely I agree, it seems bogus - the use of time_after takes care of the short first jiffy problem, the real problem I see is that 1500 was supposed to be µs, that is close to the datasheet spec for the flash I have. Fixing that will almost double the timeout on my systems...
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
I'd like someone with infinite amounts of free time to go through and clean them all up :)
lol! Would you accept
msecs_to_jiffiesconversion for the cfi files?Absolutely. Although I'd like to take a little step back and take a more thoughtful view of how we *should* be handing these timeouts, rather than a simple janitor-style conversion of the existing code.
Okay, well, I'll send a patch in a few days, if there is a better way than:
unsigned long uWriteTimeout = usecs_to_jiffies(1500); timeo = jiffies + uWriteTimeout;
if (time_after(jiffies, timeo)) // TIMEOUT
Maybe someone will pipe up?
Jason
Quoted 13 lines · David Woodhouse
Quoted 11 lines · Artem Bityutskiy
drivers/mtd/chips/cfi_cmdset_0002.c static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, · line 1389
Quoted 7 lines · Jason Gunthorpe
drivers/mtd/chips/cfi_cmdset_0002.c +5
-2-/* see comments in do_write_oneword() regarding uWriteTimeo. */ -unsigned long uWriteTimeout = (HZ /1000 ) +1 ; +/* see comments in do_write_oneword() regarding uWriteTimeo. +Note: write_buffer commands take longer so we use a higher +time. The AMD 29LV256M for instance has a datasheet max +of 1.2ms for page and 600us for byte */ +unsigned long uWriteTimeout = (HZ /1500 ) +2 ;What does HZ / 1500 mean? HZ is amount of timer interrupts per second, which may be 1/100, or 1/1000, or 1/300, depending on the system.
Quoted 2 lines
If HZ is the amount of timer interrupts per second, then HZ/1500 is the amount of timer interrupts in 1/1500 of a second, aka 666.7µs.¹
First of all, yes, my e-mail was not very clear, I guess I just wanted to express that it is not very readable. Also, HZ / 1500 would be 0 for HZ < 1500, since we are using integer arithmetic.
Signature · 2 lines
Best Regards, Artem Bityutskiy
Quoted 2 lines · Artem Bityutskiy
Also, HZ / 1500 would be 0 for HZ < 1500, since we are using integer
arithmetic.
Hence the +1. One of them, at least.
Signature · 1 line
dwmw2