Re: [PATCH 4/10] Support for kernel+fs images

[Paul M. <le...@li...>]

On Thu, Jul 26, 2007 at 03:27:13PM +1000, David McCullough wrote:
> Add support for copying our filesystem out of the way when images are
> constructed by concatenating the root filesystem to the end of a linux.bin
> which has the BSS removed (to save space).
> 
That's an interesting abuse :-)

> --- a/arch/sh/kernel/setup.c
> +++ b/arch/sh/kernel/setup.c
> @@ -399,3 +399,66 @@ struct seq_operations cpuinfo_op = {
>  	.show	= show_cpuinfo,
>  };
>  #endif /* CONFIG_PROC_FS */
> +
> +#if defined(CONFIG_SH_CONCAT_FS)
> +/*
> + * do not call printk in here,  bad things will happen,  the kernel isn't
> + * actually up yet,  we are called from head.S before BSS is cleared.
> + */
> +
> +void __init __attribute__ ((weak))
> +arch_service_watchdog(void)
> +{
> +}
> +

I'm not sure if I like this watchdog thing..

> +extern void copy_romfs(void);
> +void __init copy_romfs()
> +{
> +	unsigned char	*sp, *dp;
> +	unsigned long	 len;
> +
> +	/*
> +	 * we used to use __bss_start,  but that is padded to 4K now and doesn't
> +	 * work, basically we need the last non-padded symbol before __bss_start
> +	 * and that is now __machvec_end
> +	 */
> +#define CURRENT_ROMFS_LOC	((unsigned long)(&__machvec_end))
> +
Please just add a non-padded symbol (ie, __davidms_strange_romfs_thing)
for this instead. Otherwise someone, most likely me, will forget that
there's some special bit of code that reads far too much in to what
__machvec_end really means.

> +#ifdef CONFIG_SH_ROMBOOT
> +	extern int _mem_start, _rom_store;
> +	sp = (unsigned char *) CURRENT_ROMFS_LOC - ((_mem_start - _rom_store) / 4);
> +#else
> +	sp = (unsigned char *) CURRENT_ROMFS_LOC;
> +#endif

CONFIG_SH_ROMBOOT is not added by any of your patches, likewise for
_mem_start and _rom_store. Presumably this should be sent as a follow up
patch if you want this functionality included.

> +	/* copy backwards to avoid writing over ourselves */
> +	arch_service_watchdog();
> +	while (dp >= ((unsigned char *) (&_end[0]))) {
> +		*dp-- = *sp--;
> +		if ((((unsigned long) dp) & 0x7ffff) == 0)
> +			arch_service_watchdog();
> +	}
> +	arch_service_watchdog();
> +}

Can you not simply disable the watchdog in your loader and start it up
from the board code? Having to do dummy reads from the thing in this
convoluted manner is rather undesirable.