Re: [flashrom] [Patch] + RFC: Use shutdown callback mechanism to shutdown programmers

[Carl-Daniel Hailfinger]

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