Patches for 34K APRP

Patches for 34K APRP

[Kevin D. Kissell]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

APRP operation of the 34K has been multiply broken
in 2.6.24.4.  The following patch set gets things
working seemingly reliably. The first two were previously
submitted to Ralf as essential to get RP programs to launch
safely. The larger third patch is new, and gets the Linux
I/O  services for the RP working again, and fixes formatting
in a few places.

Note that SMTC has scheduling problems in 2.6.24,
so testing under SMTC has been limited to boots with
only one TC acting as a Linux CPU.

	Regards,

	Kevin K.

Re: Patches for 34K APRP

[Kevin D. Kissell]

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]

Sigh.  Some mailers (including Thunderbird for Windows)
seem to be unable to handle the attachment of the patch
files by Thunderbird.  So I attach them as a .tgz, which
even Outlook should be able to deal with correctly.

	Regards,

	Kevin K.

Kevin D. Kissell wrote:
> APRP operation of the 34K has been multiply broken
> in 2.6.24.4.  The following patch set gets things
> working seemingly reliably. The first two were previously
> submitted to Ralf as essential to get RP programs to launch
> safely. The larger third patch is new, and gets the Linux
> I/O  services for the RP working again, and fixes formatting
> in a few places.
> 
> Note that SMTC has scheduling problems in 2.6.24,
> so testing under SMTC has been limited to boots with
> only one TC acting as a Linux CPU.
> 
>     Regards,
> 
>     Kevin K.


[-- Attachment #2: aprprollup_160408.tgz --]
[-- Type: application/x-compressed-tar, Size: 3775 bytes --]

Re: Patches for 34K APRP

[Ralf Baechle]

On Wed, Apr 16, 2008 at 03:42:48PM +0200, Kevin D. Kissell wrote:

> Sigh.  Some mailers (including Thunderbird for Windows)
> seem to be unable to handle the attachment of the patch
> files by Thunderbird.  So I attach them as a .tgz, which
> even Outlook should be able to deal with correctly.

With the kernel maintainers head hidden for a moment - the attachments
certainly were ok, I was able to feed them to patch and had no problems
applying them.

With the kernel maintainers hat firmly back on the head:

 o git will use the Subject: line followed by the body followed by the
   attachment's leading comment as the commit message for the patch.
   Anything after a "---" tearoff line such a signature will be dropped.
   For the submitter that means to ensure only things that are meant to
   go into the log should be before the tearoff line.
 o As the result of this method git will only be able to handle a single
   patch per email.  But that's not a problem anyway.  Patches are meant
   to be easily discussable by email and that's best done with a single
   patch per mail.

I'm going to send comments on the actual patch by separate email.

  Ralf

Re: Patches for 34K APRP

[Ralf Baechle]

On Wed, Apr 16, 2008 at 03:32:22PM +0200, Kevin D. Kissell wrote:

> >From d164aa1b9dba720ee08a6a773e1a81b62aea7d10 Mon Sep 17 00:00:00 2001
> From: Kevin D. Kissell <kevink@mips.com>
> Date: Thu, 10 Apr 2008 02:07:38 +0200
> Subject: [PATCH] Fixes necessary for non-SMP kernels and non-relocatable binaries

>  		struct elf_phdr *phdr = (struct elf_phdr *) ((char *)hdr + hdr->e_phoff);
>  
>  		for (i = 0; i < hdr->e_phnum; i++) {
> -			if (phdr->p_type != PT_LOAD)
> -				continue;
> -
> -			memcpy((void *)phdr->p_paddr, (char *)hdr + phdr->p_offset, phdr->p_filesz);
> -			memset((void *)phdr->p_paddr + phdr->p_filesz, 0, phdr->p_memsz - phdr->p_filesz);
> -			phdr++;
> +		    if (phdr->p_type == PT_LOAD) {
> +			memcpy((void *)phdr->p_paddr, 
> +				(char *)hdr + phdr->p_offset, phdr->p_filesz);
> +			memset((void *)phdr->p_paddr + phdr->p_filesz, 
> +				0, phdr->p_memsz - phdr->p_filesz);
> +		    }

Patch applied with some reformatting to stick to the one tab per nesting
level rule.  Also git was bitching a little about whitespace:

.dotest/patch:13:       /* 
Space in indent is followed by a tab.
.dotest/patch:14:        * SMTC/SMVP kernels manage VPE enable independently,
Space in indent is followed by a tab.
.dotest/patch:15:        * but uniprocessor kernels need to turn it on, even
Space in indent is followed by a tab.
.dotest/patch:16:        * if that wasn't the pre-dvpe() state.
Space in indent is followed by a tab.
.dotest/patch:17:        */
warning: squelched 2 whitespace errors
warning: 7 lines applied after fixing whitespace errors.

Thanks,

  Ralf

Re: Patches for 34K APRP

[Ralf Baechle]

On Wed, Apr 16, 2008 at 03:32:22PM +0200, Kevin D. Kissell wrote:

>  arch/mips/kernel/setup.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index f8a535a..a6a0d62 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -336,6 +336,10 @@ static void __init bootmem_init(void)
>  #endif
>  		max_low_pfn = PFN_DOWN(HIGHMEM_START);
>  	}
> +	/*
> +	 * Propagate final value of max_low_pfn to max_pfn
> +	 */
> +	max_pfn = max_low_pfn;

That will be incorrect for systems with highmem.  So I think the right
fix is to replace all references to max_pfn in vpe.c with max_low_pfn.

It still won't play nicely with esotheric configurations such as
discontig memory ...

  Ralf

Re: Patches for 34K APRP

[Kevin D. Kissell]

Ralf Baechle wrote:
> On Wed, Apr 16, 2008 at 03:32:22PM +0200, Kevin D. Kissell wrote:
>
>   
>>  arch/mips/kernel/setup.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
>> index f8a535a..a6a0d62 100644
>> --- a/arch/mips/kernel/setup.c
>> +++ b/arch/mips/kernel/setup.c
>> @@ -336,6 +336,10 @@ static void __init bootmem_init(void)
>>  #endif
>>  		max_low_pfn = PFN_DOWN(HIGHMEM_START);
>>  	}
>> +	/*
>> +	 * Propagate final value of max_low_pfn to max_pfn
>> +	 */
>> +	max_pfn = max_low_pfn;
>>     
>
> That will be incorrect for systems with highmem.  So I think the right
> fix is to replace all references to max_pfn in vpe.c with max_low_pfn.
>   
Which will still be incorrect for systems with highmem, right?  The reason
I propose fixing max_pfn instead of just hacking vpe.c is that, as per my
email of a couple of weeks ago, there are other, architecture independent,
bits of code in the 2.6.24 kernel where max_pfn is assumed to be sane,
and the value of zero may result in otherwise inexplicable Bad Things
happening.  So even if you want to change vpe.c to use max_low_pfn
instead of max_pfn, I believe that we really want that patch to setup.c
until such time as we've verified that the kernel is max_pfn-free.

          Regards,

          Kevin K.

Re: Patches for 34K APRP

[Ralf Baechle]

On Thu, Apr 17, 2008 at 03:38:00PM +0200, Kevin D. Kissell wrote:

>> That will be incorrect for systems with highmem.  So I think the right
>> fix is to replace all references to max_pfn in vpe.c with max_low_pfn.
>>   
> Which will still be incorrect for systems with highmem, right?  The reason
> I propose fixing max_pfn instead of just hacking vpe.c is that, as per my
> email of a couple of weeks ago, there are other, architecture independent,
> bits of code in the 2.6.24 kernel where max_pfn is assumed to be sane,
> and the value of zero may result in otherwise inexplicable Bad Things
> happening.  So even if you want to change vpe.c to use max_low_pfn
> instead of max_pfn, I believe that we really want that patch to setup.c
> until such time as we've verified that the kernel is max_pfn-free.

Hmm..  yes.  But your 0002-Propagate-max_low_pfn-as-max_pfn-fo patch
uses max_low_pfn after it has already been cut down to exclude highmem.
Moving the assignment a few lines up just before the if statement should
fix the issue.

  Ralf

Re: Patches for 34K APRP

[Ralf Baechle]

On Wed, Apr 16, 2008 at 03:32:22PM +0200, Kevin D. Kissell wrote:

> --- a/arch/mips/kernel/kspd.c
> +++ b/arch/mips/kernel/kspd.c
> @@ -257,7 +257,7 @@ void sp_work_handle_request(void)
>  
>  		vcwd = vpe_getcwd(tclimit);
>  
> - 		/* change to the cwd of the process that loaded the SP program */
> + 		/* change to cwd of the process that loaded the SP program */
>  		old_fs = get_fs();
>  		set_fs(KERNEL_DS);
>  		sys_chdir(vcwd);
> @@ -323,6 +323,9 @@ static void sp_cleanup(void)
>  			set >>= 1;
>  		}
>  	}
> +
> +	/* Put daemon cwd back to root to avoid umount problems */
> +	sys_chdir("/");

Still not kosher; there might be multiple roots on a system ...

>  }
>  
>  static int channel_open = 0;
> diff --git a/arch/mips/kernel/rtlx.c b/arch/mips/kernel/rtlx.c
> index 1ba00c1..c0bb347 100644
> --- a/arch/mips/kernel/rtlx.c
> +++ b/arch/mips/kernel/rtlx.c
> @@ -73,6 +73,15 @@ static void rtlx_dispatch(void)
>  static irqreturn_t rtlx_interrupt(int irq, void *dev_id)
>  {
>  	int i;
> +	unsigned int flags, vpeflags;
> +
> +	/* Ought not to be strictly necessary for SMTC builds */
> +	local_irq_save(flags);
> +	vpeflags = dvpe();
> +	set_c0_status(0x100 << MIPS_CPU_RTLX_IRQ);
> +	irq_enable_hazard();
> +	evpe(vpeflags);
> +	local_irq_restore(flags);
>  
>  	for (i = 0; i < RTLX_CHANNELS; i++) {
>  			wake_up(&channel_wqs[i].lx_queue);
> @@ -109,7 +118,8 @@ static void __used dump_rtlx(void)
>  static int rtlx_init(struct rtlx_info *rtlxi)
>  {
>  	if (rtlxi->id != RTLX_ID) {
> -		printk(KERN_ERR "no valid RTLX id at 0x%p 0x%lx\n", rtlxi, rtlxi->id);
> +		printk(KERN_ERR "no valid RTLX id at 0x%p 0x%lx\n", 
> +			rtlxi, rtlxi->id);
>  		return -ENOEXEC;
>  	}
>  
> @@ -163,51 +173,51 @@ int rtlx_open(int index, int can_sleep)
>  
>  	if (rtlx == NULL) {
>  		if( (p = vpe_get_shared(tclimit)) == NULL) {
> -			if (can_sleep) {
> -				__wait_event_interruptible(channel_wqs[index].lx_queue,
> -				                           (p = vpe_get_shared(tclimit)),
> -				                           ret);
> -				if (ret)
> -					goto out_fail;
> -			} else {
> -				printk(KERN_DEBUG "No SP program loaded, and device "
> -					"opened with O_NONBLOCK\n");
> -				ret = -ENOSYS;
> +		    if (can_sleep) {
> +			__wait_event_interruptible(channel_wqs[index].lx_queue,
> +				(p = vpe_get_shared(tclimit)), ret);
> +			if (ret)
>  				goto out_fail;
> -			}
> +		    } else {
> +			printk(KERN_DEBUG "No SP program loaded, and device "
> +					"opened with O_NONBLOCK\n");
> +			ret = -ENOSYS;
> +			goto out_fail;
> +		    }
>  		}
>  
>  		smp_rmb();
>  		if (*p == NULL) {
> -			if (can_sleep) {
> -				DEFINE_WAIT(wait);
> -
> -				for (;;) {
> -					prepare_to_wait(&channel_wqs[index].lx_queue, &wait, TASK_INTERRUPTIBLE);
> -					smp_rmb();
> -					if (*p != NULL)
> -						break;
> -					if (!signal_pending(current)) {
> -						schedule();
> -						continue;
> -					}
> -					ret = -ERESTARTSYS;
> -					goto out_fail;
> +		    if (can_sleep) {
> +			DEFINE_WAIT(wait);
> +
> +			for (;;) {
> +				prepare_to_wait(&channel_wqs[index].lx_queue, 
> +						&wait, TASK_INTERRUPTIBLE);
> +				smp_rmb();
> +				if (*p != NULL)
> +					break;
> +				if (!signal_pending(current)) {
> +					schedule();
> +					continue;
>  				}
> -				finish_wait(&channel_wqs[index].lx_queue, &wait);
> -			} else {
> -				printk(" *vpe_get_shared is NULL. "
> -				       "Has an SP program been loaded?\n");
> -				ret = -ENOSYS;
> +				ret = -ERESTARTSYS;
>  				goto out_fail;
>  			}
> +			finish_wait(&channel_wqs[index].lx_queue, &wait);
> +		    } else {
> +			printk(" *vpe_get_shared is NULL. "
> +			       "Has an SP program been loaded?\n");
> +				ret = -ENOSYS;
> +				goto out_fail;
> +		    }
>  		}
>  
>  		if ((unsigned int)*p < KSEG0) {
> -			printk(KERN_WARNING "vpe_get_shared returned an invalid pointer "
> -			       "maybe an error code %d\n", (int)*p);
> -			ret = -ENOSYS;
> -			goto out_fail;
> +		    printk(KERN_WARNING "vpe_get_shared returned an invalid "
> +			"pointer maybe an error code %d\n", (int)*p);
> +		    ret = -ENOSYS;
> +		    goto out_fail;

Indention one tab per nested block.

>  		}
>  
>  		if ((ret = rtlx_init(*p)) < 0)
> @@ -233,6 +243,10 @@ out_ret:
>  
>  int rtlx_release(int index)
>  {
> + 	if (rtlx == NULL) {
> +		printk("rtlx_release() with null rtlx\n");

Missing KERN_ facility level.  I turned this printk into a pr_err().

> + 		return 0;
> +	}
>  	rtlx->channel[index].lx_state = RTLX_STATE_UNUSED;
>  	return 0;
>  }
> @@ -252,8 +266,8 @@ unsigned int rtlx_read_poll(int index, int can_sleep)
>  			int ret = 0;
>  
>  			__wait_event_interruptible(channel_wqs[index].lx_queue,
> -			                           chan->lx_read != chan->lx_write || sp_stopping,
> -			                           ret);
> +				(chan->lx_read != chan->lx_write) 
> +				|| sp_stopping, ret);

Linux conding style:

	if (expr1 ||
	    expr2)

GNU coding style:

	if (expr1
	    || expr2)

Gno GNUs ;-)

>  			if (ret)
>  				return ret;
>  
> @@ -283,7 +297,9 @@ static inline int write_spacefree(int read, int write, int size)
>  unsigned int rtlx_write_poll(int index)
>  {
>  	struct rtlx_channel *chan = &rtlx->channel[index];
> -	return write_spacefree(chan->rt_read, chan->rt_write, chan->buffer_size);
> +
> +	return write_spacefree(chan->rt_read, chan->rt_write, 
> +				chan->buffer_size);
>  }
>  
>  ssize_t rtlx_read(int index, void __user *buff, size_t count)
> @@ -345,8 +361,8 @@ ssize_t rtlx_write(int index, const void __user *buffer, size_t count)
>  	rt_read = rt->rt_read;
>  
>  	/* total number of bytes to copy */
> -	count = min(count,
> -		    (size_t)write_spacefree(rt_read, rt->rt_write, rt->buffer_size));
> +	count = min(count, (size_t)write_spacefree(rt_read, rt->rt_write, 
> +							rt->buffer_size));
>  
>  	/* first bit from write pointer to the end of the buffer, or count */
>  	fl = min(count, (size_t) rt->buffer_size - rt->rt_write);
> @@ -515,6 +531,11 @@ static int __init rtlx_module_init(void)
>  
>  	if (cpu_has_vint)
>  		set_vi_handler(MIPS_CPU_RTLX_IRQ, rtlx_dispatch);
> +	else {
> +		printk("APRP RTLX init on non-vectored-interrupt processor\n");

Missing KERN_ facility level.  I turned this printk into a pr_err().

> +		err = -ENODEV;
> +		goto out_chrdev;
> +	}
>  
>  	rtlx_irq.dev_id = rtlx;
>  	setup_irq(rtlx_irq_num, &rtlx_irq);
> diff --git a/include/asm-mips/rtlx.h b/include/asm-mips/rtlx.h
> index 65778c8..20b6660 100644
> --- a/include/asm-mips/rtlx.h
> +++ b/include/asm-mips/rtlx.h
> @@ -29,13 +29,13 @@ extern unsigned int rtlx_read_poll(int index, int can_sleep);
>  extern unsigned int rtlx_write_poll(int index);
>  
>  enum rtlx_state {
> -	RTLX_STATE_UNUSED,
> +	RTLX_STATE_UNUSED = 0,
>  	RTLX_STATE_INITIALISED,
>  	RTLX_STATE_REMOTE_READY,
>  	RTLX_STATE_OPENED
>  };
>  
> -#define RTLX_BUFFER_SIZE 1024
> +#define RTLX_BUFFER_SIZE 2048
>  
>  /* each channel supports read and write.
>     linux (vpe0) reads lx_buffer  and writes rt_buffer

Applie with a few fixes but not the chdir issue.

Thanks,

  Ralf

Re: Patches for 34K APRP

[Kevin D. Kissell]

Ralf Baechle wrote:
> On Wed, Apr 16, 2008 at 03:32:22PM +0200, Kevin D. Kissell wrote:
>
>   
>> --- a/arch/mips/kernel/kspd.c
>> +++ b/arch/mips/kernel/kspd.c
>> @@ -257,7 +257,7 @@ void sp_work_handle_request(void)
>>  
>>  		vcwd = vpe_getcwd(tclimit);
>>  
>> - 		/* change to the cwd of the process that loaded the SP program */
>> + 		/* change to cwd of the process that loaded the SP program */
>>  		old_fs = get_fs();
>>  		set_fs(KERNEL_DS);
>>  		sys_chdir(vcwd);
>> @@ -323,6 +323,9 @@ static void sp_cleanup(void)
>>  			set >>= 1;
>>  		}
>>  	}
>> +
>> +	/* Put daemon cwd back to root to avoid umount problems */
>> +	sys_chdir("/");
>>     
>
> Still not kosher; there might be multiple roots on a system ...
>   
What do you suggest, then?  Leaving things as they are makes it impossible
to do a clean shutdown of an APRP system if the kspd was last used to 
support
an RP binary that was read from a mounted file system.  I respectfully 
submit
that it's at least an order of magnitude more likely that a mounted file 
system
will have been used than a chrooted, mounted file system.

          Regards,

          Kevin K.