Re: [coreboot] [PATCH] flashrom: Remove superfluous volatile

[Carl-Daniel Hailfinger]

On 06.03.2009 00:59, Joseph Smith wrote:
> On Fri, 06 Mar 2009 00:23:45 +0100, Carl-Daniel Hailfinger
>  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
>
> Acked-by: Joseph Smith
>
> 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