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

[David M. <Dav...@se...>]

Jivin Paul Mundt lays it down ...
> 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 :-)

It's a very old thing,  our images from the beginning of time have been
like this.  If you don't like it,  no problems, you can get around it by making
images with the BSS included in the linux.bin.

> > --- 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..

What was there before was all ifdef'd,  I figured this might be more
acceptable,  I can go back to ifdef's if you like.

> > +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.

ok.

> > +#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.

My BAD,  the CONFIG_SH_ROMBOOT was contributed by someone else and since
we don't use it,  I didn't need all the other bits to make it go.

I'll fix this if you think the "strange romfs thing" is acceptable in
some shape or form.

> > +	/* 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.

The watchdog is on from poweron, it cannot be disabled in any possible
way.  You have at most 1 second (or less on some versions) to service it or
it resets the hardware.

Again, if it's no good we'll just carry the needed changes in our tree.

Cheers,
Davidm

-- 
David McCullough,  dav...@se...,   Ph:+61 734352815
Secure Computing - SnapGear  http://www.uCdot.org http://www.cyberguard.com