| Submitter | David Hendricks |
|---|---|
| Date | 2011-04-22 04:31:00 |
| Message ID | <BANLkTimkpRdnWJUJnhcfCyJa9Hk=9PZgyQ@mail.gmail.com> |
| Download | mbox | patch |
| Permalink | /patch/2908/ |
| State | Superseded |
| Headers | show |
Comments
Am 22.04.2011 06:31 schrieb David Hendricks: > Index: internal.c > =================================================================== > --- internal.c (revision 1288) > +++ internal.c (working copy) > @@ -164,6 +164,7 @@ > } > free(arg); > > + register_shutdown(internal_shutdown, NULL); > get_io_perms(); > Wrong order. We want to register the shutdown function only after we got IO permissions. That said, should get_io_perms automatically register release_io_perms? This would kill a few useless shutdown functions. > > /* Initialize PCI access for flash enables */ > @@ -264,11 +265,9 @@ > #endif > } > > -int internal_shutdown(void) > +void internal_shutdown(void *data) > { > release_io_perms(); > - > - return 0; > } > #endif > > Index: it85spi.c > =================================================================== > --- it85spi.c (revision 1288) > +++ it85spi.c (working copy) > @@ -80,6 +80,8 @@ > #define INDIRECT_WRITE(base, value) OUTB(value, (base) + 4) > #endif /* LPC_IO */ > > +void it85xx_shutdown(void *); > + > #ifdef LPC_IO > unsigned int shm_io_base; > #endif > @@ -308,6 +310,7 @@ > /* Set this as spi controller. */ > spi_controller = SPI_CONTROLLER_IT85XX; > > + register_shutdown(it85xx_shutdown, NULL); > return 0; > } > > @@ -349,11 +352,10 @@ > return ret; > } > > -int it85xx_shutdown(void) > +void it85xx_shutdown(void *data) > { > msg_pdbg("%s():%d\n", __func__, __LINE__); > it85xx_exit_scratch_rom(); > - return 0; > } > > /* According to ITE 8502 document, the procedure to follow mode is following: > It would be great if you could forward-port this patch to the current IT85* SPI code. > Index: flashrom.c > =================================================================== > --- flashrom.c (revision 1288) > +++ flashrom.c (working copy) > @@ -126,7 +126,8 @@ > { > .name = "internal", > .init = internal_init, > - .shutdown = internal_shutdown, > + /* called implicitly using shutdown callback */ > +// .shutdown = internal_shutdown, > .map_flash_region = physmap, > .unmap_flash_region = physunmap, > .chip_readb = internal_chip_readb, > The same for all other programmers, please. And kill the .shutdown struct member everywhere (even in the header file) while you're at it. > @@ -543,7 +544,7 @@ > return ret; > } > > -int programmer_shutdown(void) > +int flashrom_shutdown(void) > { > /* Registering shutdown functions is no longer allowed. */ > may_register_shutdown = 0; > @@ -551,7 +552,7 @@ > int i = --shutdown_fn_count; > shutdown_fn[i].func(shutdown_fn[i].data); > We should care about the result of the shutdown function, even if it is only for printing diagnostic messages. > } > - return programmer_table[programmer].shutdown(); > + return 0; > And return the error code of the failing shutdown function (if one failed), zero (if none failed) or some arbitrary value if more than one shutdown function failed. > } > > void *programmer_map_flash_region(const char *descr, unsigned long phys_addr, > @@ -1939,6 +1940,6 @@ > free(oldcontents); > free(newcontents); > out_nofree: > - programmer_shutdown(); > + flashrom_shutdown(); > return ret; > } > Index: programmer.h > =================================================================== > --- programmer.h (revision 1288) > +++ programmer.h (working copy) > @@ -111,7 +111,7 @@ > extern const struct programmer_entry programmer_table[]; > > int programmer_init(char *param); > -int programmer_shutdown(void); > +int flashrom_shutdown(void); > Given that this is still a programmer shutdown (as opposed to a flash chip shutdown which may be necessary in the future), I'd like to keep the name. Overall, I like this patch. If you send a variant which converts all programmers and addresses the comments, I'll ack. Regards, Carl-Daniel
On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote: > > Am 22.04.2011 06:31 schrieb David Hendricks: >> >> Index: internal.c >> =================================================================== >> --- internal.c (revision 1288) >> +++ internal.c (working copy) >> @@ -164,6 +164,7 @@ >> } >> free(arg); >> >> + register_shutdown(internal_shutdown, NULL); >> get_io_perms(); >> > > Wrong order. We want to register the shutdown function only after we got IO permissions. That said, should get_io_perms automatically register release_io_perms? This would kill a few useless shutdown functions. I like this idea of everybody cleaning up after themselfes. > > Index: it85spi.c > =================================================================== > --- it85spi.c (revision 1288) > +++ it85spi.c (working copy) > @@ -80,6 +80,8 @@ > #define INDIRECT_WRITE(base, value) OUTB(value, (base) + 4) > #endif /* LPC_IO */ > > +void it85xx_shutdown(void *); > + This should go away, rather make the function static instead. > > #ifdef LPC_IO > unsigned int shm_io_base; > #endif > @@ -308,6 +310,7 @@ > /* Set this as spi controller. */ > spi_controller = SPI_CONTROLLER_IT85XX; > > + register_shutdown(it85xx_shutdown, NULL); > return 0; > } > > @@ -349,11 +352,10 @@ > return ret; > } > > -int it85xx_shutdown(void) > +void it85xx_shutdown(void *data) > { > msg_pdbg("%s():%d\n", __func__, __LINE__); > it85xx_exit_scratch_rom(); > - return 0; > } And move it above the register_... function. -- Stefan Reinauer Google Inc.
Thanks for the comments! I tend to agree with them all. FWIW, we've been testing the basic ideas here in the Chromium OS version and it seems to work pretty well. So at least I think we're on the right path. A few details to address before making broader changes (and potentially making a huge mess of things).... On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote: > Am 22.04.2011 06:31 schrieb David Hendricks: > >> Index: internal.c >> =================================================================== >> --- internal.c (revision 1288) >> +++ internal.c (working copy) >> @@ -164,6 +164,7 @@ >> } >> free(arg); >> >> + register_shutdown(internal_shutdown, NULL); >> get_io_perms(); >> >> > > Wrong order. We want to register the shutdown function only after we got IO > permissions. That said, should get_io_perms automatically register > release_io_perms? This would kill a few useless shutdown functions. > Fixed the ordering, and I agree that get_io_perms should register release_io_perms. On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote: > It would be great if you could forward-port this patch to the current IT85* >> SPI code. > > Done. Also fixed the prototype thing Stefan pointed out. On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote: > Index: flashrom.c >> =================================================================== >> --- flashrom.c (revision 1288) >> +++ flashrom.c (working copy) >> @@ -126,7 +126,8 @@ >> { >> .name = "internal", >> .init = internal_init, >> - .shutdown = internal_shutdown, >> + /* called implicitly using shutdown callback */ >> +// .shutdown = internal_shutdown, >> .map_flash_region = physmap, >> .unmap_flash_region = physunmap, >> .chip_readb = internal_chip_readb, >> >> > > The same for all other programmers, please. > And kill the .shutdown struct member everywhere (even in the header file) > while you're at it. > After giving this some thought, I think getting rid of .shutdown struct members might be premature. Perhaps they can be useful for something like register_shutdown(programmer.shutdown, data). Also, the structure looks a bit awkward without a shutdown member, even if it is superfluous. If you still feel we should get rid of the .shutdown member, that's fine too. Otherwise we can just change the structure member so that it matches the function signature that register_shutdown() expects. On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote: > @@ -543,7 +544,7 @@ >> return ret; >> } >> >> -int programmer_shutdown(void) >> +int flashrom_shutdown(void) >> { >> /* Registering shutdown functions is no longer allowed. */ >> may_register_shutdown = 0; >> @@ -551,7 +552,7 @@ >> int i = --shutdown_fn_count; >> shutdown_fn[i].func(shutdown_fn[i].data); >> >> > > We should care about the result of the shutdown function, even if it is > only for printing diagnostic messages. > In that case, the shutdown callbacks should return an int. Right now they return void. Should we go ahead and update the return type for shutdown functions? On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote: > } >> - return programmer_table[programmer].shutdown(); >> + return 0; >> >> > > And return the error code of the failing shutdown function (if one failed), > zero (if none failed) or some arbitrary value if more than one shutdown > function failed. > That would be much more useful, but requires the above change. On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote: > Index: programmer.h >> =================================================================== >> --- programmer.h (revision 1288) >> +++ programmer.h (working copy) >> @@ -111,7 +111,7 @@ >> extern const struct programmer_entry programmer_table[]; >> >> int programmer_init(char *param); >> -int programmer_shutdown(void); >> +int flashrom_shutdown(void); >> >> > > > Given that this is still a programmer shutdown (as opposed to a flash chip > shutdown which may be necessary in the future), I'd like to keep the name. > Agreed.
Am 03.05.2011 02:08 schrieb David Hendricks: > Thanks for the comments! I tend to agree with them all. FWIW, we've been > testing the basic ideas here in the Chromium OS version and it seems to work > pretty well. So at least I think we're on the right path. > Agreed. > On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger wrote: > >> should get_io_perms automatically register >> release_io_perms? This would kill a few useless shutdown functions. >> > I agree that get_io_perms should register > release_io_perms. > The more I think about it, the more I wonder if we really should do that. If we decide to do it, a failed programmer_init() must be followed by a programmer_shutdown() call because there is no way to unregister a function (e.g. release_io_perms). Can you leave out the release_io_perms automatic registration for now until we have discussed this and found a model which makes everyone happy? > On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger wrote: >> Index: flashrom.c >> >>> =================================================================== >>> --- flashrom.c (revision 1288) >>> +++ flashrom.c (working copy) >>> @@ -126,7 +126,8 @@ >>> { >>> .name = "internal", >>> .init = internal_init, >>> - .shutdown = internal_shutdown, >>> + /* called implicitly using shutdown callback */ >>> +// .shutdown = internal_shutdown, >>> .map_flash_region = physmap, >>> .unmap_flash_region = physunmap, >>> .chip_readb = internal_chip_readb, >>> >>> >>> >> The same for all other programmers, please. >> And kill the .shutdown struct member everywhere (even in the header file) >> while you're at it. >> >> > After giving this some thought, I think getting rid of .shutdown struct > members might be premature. Perhaps they can be useful for something like > register_shutdown(programmer.shutdown, data). AFAICS that would lead to ordering problems for shutdown. > Also, the structure looks a > bit awkward without a shutdown member, even if it is superfluous. > Right, that part is a bit awkward. That said, if we register the shutdown functions properly, .shutdown should never need to be defined. > If you still feel we should get rid of the .shutdown member, that's fine > too. Otherwise we can just change the structure member so that it matches > the function signature that register_shutdown() expects. > Can you think of a technical reason where register_shutdown(somefunction) would not be sufficient? If not, I'd say we kill that structure member and reintroduce it only if we stumble over some case where it is needed. > On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger wrote: > >> @@ -543,7 +544,7 @@ >> >>> return ret; >>> } >>> >>> -int programmer_shutdown(void) >>> +int flashrom_shutdown(void) >>> { >>> /* Registering shutdown functions is no longer allowed. */ >>> may_register_shutdown = 0; >>> @@ -551,7 +552,7 @@ >>> int i = --shutdown_fn_count; >>> shutdown_fn[i].func(shutdown_fn[i].data); >>> >> We should care about the result of the shutdown function, even if it is >> only for printing diagnostic messages. >> > In that case, the shutdown callbacks should return an int. Right now they > return void. Should we go ahead and update the return type for shutdown > functions? > Please do. This is especially useful for libflashrom. My plan is to allow multiple sequences of programmer_init(); do_stuff(); programmer_shutdown(); for different (or identical) programmers without quitting flashrom in between. Assuming that init functions clean up after themselves, the only reason to refuse another init is a failed shutdown in the previous cycle. (Side note: Programmer parameter parsing will need minor changes to work as intended once we support such a style of operation.) > On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger wrote >>> - return programmer_table[programmer].shutdown(); >>> + return 0; >>> >> And return the error code of the failing shutdown function (if one failed), >> zero (if none failed) or some arbitrary value if more than one shutdown >> function failed. >> > That would be much more useful, but requires the above change. > Go ahead. Regards, Carl-Daniel
Patch
Index: cli_classic.c =================================================================== --- cli_classic.c (revision 1288) +++ cli_classic.c (working copy) @@ -373,7 +373,7 @@ for (i = 0; i < ARRAY_SIZE(flashes) && flashes[i]; i++) printf(" %s", flashes[i]->name); printf("\nPlease specify which chip to use with the -c <chipname> option.\n"); - programmer_shutdown(); + flashrom_shutdown(); exit(1); } else if (!flashes[0]) { printf("No EEPROM/flash device found.\n"); @@ -385,14 +385,14 @@ flashes[0] = probe_flash(flashchips, 1); if (!flashes[0]) { printf("Probing for flash chip '%s' failed.\n", chip_to_probe); - programmer_shutdown(); + flashrom_shutdown(); exit(1); } printf("Please note that forced reads most likely contain garbage.\n"); return read_flash_to_file(flashes[0], filename); } // FIXME: flash writes stay enabled! - programmer_shutdown(); + flashrom_shutdown(); exit(1); } @@ -405,21 +405,21 @@ (!force)) { fprintf(stderr, "Chip is too big for this programmer " "(-V gives details). Use --force to override.\n"); - programmer_shutdown(); + flashrom_shutdown(); return 1; } if (!(read_it | write_it | verify_it | erase_it)) { printf("No operations were specified.\n"); // FIXME: flash writes stay enabled! - programmer_shutdown(); + flashrom_shutdown(); exit(0); } if (!filename && !erase_it) { printf("Error: No filename specified.\n"); // FIXME: flash writes stay enabled! - programmer_shutdown(); + flashrom_shutdown(); exit(1); } Index: internal.c =================================================================== --- internal.c (revision 1288) +++ internal.c (working copy) @@ -164,6 +164,7 @@ } free(arg); + register_shutdown(internal_shutdown, NULL); get_io_perms(); /* Initialize PCI access for flash enables */ @@ -264,11 +265,9 @@ #endif } -int internal_shutdown(void) +void internal_shutdown(void *data) { release_io_perms(); - - return 0; } #endif Index: it85spi.c =================================================================== --- it85spi.c (revision 1288) +++ it85spi.c (working copy) @@ -80,6 +80,8 @@ #define INDIRECT_WRITE(base, value) OUTB(value, (base) + 4) #endif /* LPC_IO */ +void it85xx_shutdown(void *); + #ifdef LPC_IO unsigned int shm_io_base; #endif @@ -308,6 +310,7 @@ /* Set this as spi controller. */ spi_controller = SPI_CONTROLLER_IT85XX; + register_shutdown(it85xx_shutdown, NULL); return 0; } @@ -349,11 +352,10 @@ return ret; } -int it85xx_shutdown(void) +void it85xx_shutdown(void *data) { msg_pdbg("%s():%d\n", __func__, __LINE__); it85xx_exit_scratch_rom(); - return 0; } /* According to ITE 8502 document, the procedure to follow mode is following: Index: flashrom.c =================================================================== --- flashrom.c (revision 1288) +++ flashrom.c (working copy) @@ -126,7 +126,8 @@ { .name = "internal", .init = internal_init, - .shutdown = internal_shutdown, + /* called implicitly using shutdown callback */ +// .shutdown = internal_shutdown, .map_flash_region = physmap, .unmap_flash_region = physunmap, .chip_readb = internal_chip_readb, @@ -543,7 +544,7 @@ return ret; } -int programmer_shutdown(void) +int flashrom_shutdown(void) { /* Registering shutdown functions is no longer allowed. */ may_register_shutdown = 0; @@ -551,7 +552,7 @@ int i = --shutdown_fn_count; shutdown_fn[i].func(shutdown_fn[i].data); } - return programmer_table[programmer].shutdown(); + return 0; } void *programmer_map_flash_region(const char *descr, unsigned long phys_addr, @@ -1939,6 +1940,6 @@ free(oldcontents); free(newcontents); out_nofree: - programmer_shutdown(); + flashrom_shutdown(); return ret; } Index: programmer.h =================================================================== --- programmer.h (revision 1288) +++ programmer.h (working copy) @@ -111,7 +111,7 @@ extern const struct programmer_entry programmer_table[]; int programmer_init(char *param); -int programmer_shutdown(void); +int flashrom_shutdown(void); enum bitbang_spi_master_type { BITBANG_SPI_INVALID = 0, /* This must always be the first entry. */ @@ -290,7 +290,7 @@ extern int force_boardmismatch; void probe_superio(void); int internal_init(void); -int internal_shutdown(void); +void internal_shutdown(void *); void internal_chip_writeb(uint8_t val, chipaddr addr); void internal_chip_writew(uint16_t val, chipaddr addr); void internal_chip_writel(uint32_t val, chipaddr addr); @@ -584,7 +584,6 @@ /* it85spi.c */ struct superio probe_superio_ite85xx(void); int it85xx_spi_init(void); -int it85xx_shutdown(void); int it85xx_probe_spi_flash(void); int it85xx_spi_send_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr);