During the conversion of flash chip accesses to helper functions, I spotted assignments to volatile variables which were neither placed inside the mmapped ROM area nor were they counters. Due to the use of accessor functions, volatile usage can be reduced significantly because the accessor functions take care of actually performing the reads/writes correctly.
The following semantic patch spotted them: r exists@ expression b; typedef uint8_t; volatile uint8_t a; position p1; @@ a@p1 = readb(b);
@script:python@ p1 << r.p1; a << r.a; b << r.b; @@ print "* file: %s line %s has assignment to unnecessarily volatile variable: %s = readb(%s);" % (p1[0].file, p1[0].line, a, b)
Result was: HANDLING: sst28sf040.c * file: sst28sf040.c line 44 has assignment to unnecessarily volatile variable: tmp = readb(TODO: Binary); * file: sst28sf040.c line 43 has assignment to unnecessarily volatile variable: tmp = readb(TODO: Binary); * file: sst28sf040.c line 42 has assignment to unnecessarily volatile variable: tmp = readb(TODO: Binary); * file: sst28sf040.c line 41 has assignment to unnecessarily volatile variable: tmp = readb(TODO: Binary); * file: sst28sf040.c line 40 has assignment to unnecessarily volatile variable: tmp = readb(TODO: Binary); * file: sst28sf040.c line 39 has assignment to unnecessarily volatile variable: tmp = readb(TODO: Binary); * file: sst28sf040.c line 38 has assignment to unnecessarily volatile variable: tmp = readb(TODO: Binary); * file: sst28sf040.c line 58 has assignment to unnecessarily volatile variable: tmp = readb(TODO: Binary); * file: sst28sf040.c line 57 has assignment to unnecessarily volatile variable: tmp = readb(TODO: Binary); * file: sst28sf040.c line 56 has assignment to unnecessarily volatile variable: tmp = readb(TODO: Binary); * file: sst28sf040.c line 55 has assignment to unnecessarily volatile variable: tmp = readb(TODO: Binary); * file: sst28sf040.c line 54 has assignment to unnecessarily volatile variable: tmp = readb(TODO: Binary); * file: sst28sf040.c line 53 has assignment to unnecessarily volatile variable: tmp = readb(TODO: Binary); * file: sst28sf040.c line 52 has assignment to unnecessarily volatile variable: tmp = readb(TODO: Binary);
From that result, the fix was obvious:
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-unneeded_volatile/sst28sf040.c =================================================================== --- flashrom-unneeded_volatile/sst28sf040.c (Revision 3971) +++ flashrom-unneeded_volatile/sst28sf040.c (Arbeitskopie) @@ -32,8 +32,7 @@
static __inline__ void protect_28sf040(volatile uint8_t *bios) { - /* ask compiler not to optimize this */ - volatile uint8_t tmp; + uint8_t tmp;
tmp = readb(bios + 0x1823); tmp = readb(bios + 0x1820); @@ -46,8 +45,7 @@
static __inline__ void unprotect_28sf040(volatile uint8_t *bios) { - /* ask compiler not to optimize this */ - volatile uint8_t tmp; + uint8_t tmp;
tmp = readb(bios + 0x1823); tmp = readb(bios + 0x1820);
On Fri, 06 Mar 2009 00:23:45 +0100, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
During the conversion of flash chip accesses to helper functions, I spotted assignments to volatile variables which were neither placed inside the mmapped ROM area nor were they counters. Due to the use of accessor functions, volatile usage can be reduced significantly because the accessor functions take care of actually performing the reads/writes correctly.
The following semantic patch spotted them: r exists@ expression b; typedef uint8_t; volatile uint8_t a; position p1; @@ a@p1 = readb(b);
@script:python@ p1 << r.p1; a << r.a; b << r.b; @@ print "* file: %s line %s has assignment to unnecessarily volatile variable: %s = readb(%s);" % (p1[0].file, p1[0].line, a, b)
Result was: HANDLING: sst28sf040.c
- file: sst28sf040.c line 44 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 43 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 42 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 41 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 40 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 39 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 38 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 58 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 57 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 56 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 55 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 54 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 53 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 52 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
From that result, the fix was obvious:
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Acked-by: Joseph Smith joe@settoplinux.org
Good find. Looks like your having fun with the semantic patching :-)
Index: flashrom-unneeded_volatile/sst28sf040.c
--- flashrom-unneeded_volatile/sst28sf040.c (Revision 3971) +++ flashrom-unneeded_volatile/sst28sf040.c (Arbeitskopie) @@ -32,8 +32,7 @@
static __inline__ void protect_28sf040(volatile uint8_t *bios) {
- /* ask compiler not to optimize this */
- volatile uint8_t tmp;
uint8_t tmp;
tmp = readb(bios + 0x1823); tmp = readb(bios + 0x1820);
@@ -46,8 +45,7 @@
static __inline__ void unprotect_28sf040(volatile uint8_t *bios) {
- /* ask compiler not to optimize this */
- volatile uint8_t tmp;
uint8_t tmp;
tmp = readb(bios + 0x1823); tmp = readb(bios + 0x1820);
On 06.03.2009 00:59, Joseph Smith wrote:
On Fri, 06 Mar 2009 00:23:45 +0100, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
During the conversion of flash chip accesses to helper functions, I spotted assignments to volatile variables which were neither placed inside the mmapped ROM area nor were they counters. Due to the use of accessor functions, volatile usage can be reduced significantly because the accessor functions take care of actually performing the reads/writes correctly.
The following semantic patch spotted them: r exists@ expression b; typedef uint8_t; volatile uint8_t a; position p1; @@ a@p1 = readb(b);
@script:python@ p1 << r.p1; a << r.a; b << r.b; @@ print "* file: %s line %s has assignment to unnecessarily volatile variable: %s = readb(%s);" % (p1[0].file, p1[0].line, a, b)
Result was: HANDLING: sst28sf040.c
- file: sst28sf040.c line 44 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 43 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 42 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 41 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 40 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 39 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 38 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 58 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 57 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 56 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 55 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 54 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 53 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
- file: sst28sf040.c line 52 has assignment to unnecessarily volatile
variable: tmp = readb(TODO: Binary);
From that result, the fix was obvious:
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Acked-by: Joseph Smith joe@settoplinux.org
Good find. Looks like your having fun with the semantic patching :-)
Thanks.
By the way, the following semantic patch uses the builtin match printing functionality by prepending a "*" to the line with the pattern: @@ expression b; typedef uint8_t; volatile uint8_t a; @@ * a = readb(b);
Result is: HANDLING: sst28sf040.c diff = --- sst28sf040.c 2009-03-06 01:04:49.000000000 +0100 @@ -35,13 +35,6 @@ static __inline__ void protect_28sf040(v /* ask compiler not to optimize this */ volatile uint8_t tmp;
- tmp = readb(bios + 0x1823); - tmp = readb(bios + 0x1820); - tmp = readb(bios + 0x1822); - tmp = readb(bios + 0x0418); - tmp = readb(bios + 0x041B); - tmp = readb(bios + 0x0419); - tmp = readb(bios + 0x040A); }
static __inline__ void unprotect_28sf040(volatile uint8_t *bios) @@ -49,13 +42,6 @@ static __inline__ void unprotect_28sf040 /* ask compiler not to optimize this */ volatile uint8_t tmp;
- tmp = readb(bios + 0x1823); - tmp = readb(bios + 0x1820); - tmp = readb(bios + 0x1822); - tmp = readb(bios + 0x0418); - tmp = readb(bios + 0x041B); - tmp = readb(bios + 0x0419); - tmp = readb(bios + 0x041A); }
static __inline__ int erase_sector_28sf040(volatile uint8_t *bios,
It's arguably a bit easier to read if you get used to the leading "-" for matching lines.
This patch was enabled by Coccinelle: http://www.emn.fr/x-info/coccinelle/
Committed in r3973.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Joseph Smith