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

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Tue Jun 14 02:13:54 CEST 2011


Am 14.06.2011 00:45 schrieb David Hendricks:
> Thanks again for the additional comments. I made the changes (comments
> below) and re-compiled with the extra programmers enabled.
>
> New version of the patch attached.
> Signed-off-by: David Hendricks <dhendrix at google.com>
>   

Thanks, looks really good.


> On Sat, Jun 11, 2011 at 5:09 AM, Carl-Daniel Hailfinger wrote:
>   
>> nicintel_spi has the register_shutdown too early.
>> nicintel will not unmap the nicintel_bar if mapping nicintel_control_bar
>> fails (error case handling needs a second label which unmaps nicintel_bar).
>>     
> Good catch -- I moved the register_shutdown call toward the bottom of the
> function, but before the bitbang_spi_init call so that failure in
> bitbang_spi_init will not prevent nicintel_spi_shutdown from being called.
>   

Not exactly what I meant. I'll try to explain it better with a diff on
top of your current diff. Apply that, and we're good to go:

> --- nicintel.c~	2011-06-14 01:58:17.000000000 +0200
> +++ nicintel.c	2011-06-14 01:59:57.000000000 +0200
> @@ -77,7 +77,7 @@
>  	nicintel_control_bar = physmap("Intel NIC control/status reg",
>  	                               addr, NICINTEL_CONTROL_MEMMAP_SIZE);
>  	if (nicintel_control_bar == ERROR_PTR)
> -		goto error_out;
> +		goto error_out_unmap;
>  
>  	if (register_shutdown(nicintel_shutdown, NULL))
>  		return 1;
> @@ -99,6 +99,8 @@
>  
>  	return 0;
>  
> +error_out_unmap:
> +	physunmap(nicintel_bar, NICINTEL_MEMMAP_SIZE);
>  error_out:
>  	pci_cleanup(pacc);
>  	release_io_perms();
>   


One comment about satasii.c:
> Index: satasii.c
> ===================================================================
> --- satasii.c	(revision 1327)
> +++ satasii.c	(working copy)
> @@ -59,7 +69,8 @@
>  		reg_offset = 0x50;
>  	}
>  
> -	sii_bar = physmap("SATA SIL registers", addr, 0x100) + reg_offset;
> +	sii_bar = reg_offset +
> +	          physmap("SATA SIL registers", addr, SATASII_MEMMAP_SIZE);
>   

Could you keep the old ordering? AFAIK the usual way to specify a
location is base+offset, not the other way round.


> Index: flashrom.c
> ===================================================================
> --- flashrom.c	(revision 1327)
> +++ flashrom.c	(working copy)
> @@ -543,13 +525,15 @@
>  
>  int programmer_shutdown(void)
>  {
> +	int rc = 0;
>   

int ret please.


> +
>  	/* Registering shutdown functions is no longer allowed. */
>  	may_register_shutdown = 0;
>  	while (shutdown_fn_count > 0) {
>  		int i = --shutdown_fn_count;
> -		shutdown_fn[i].func(shutdown_fn[i].data);
> +		rc |= shutdown_fn[i].func(shutdown_fn[i].data);
>  	}
> -	return programmer_table[programmer].shutdown();
> +	return rc;
>  }
>  
>  void *programmer_map_flash_region(const char *descr, unsigned long phys_addr,
>   


With those minor changes, the patch is
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

Please go ahead and commit.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/





More information about the flashrom mailing list