Skip to contentSkip to conversation

Patches for 34K APRP

DISCUSSION Kevin D. Kissell 2 participants 9 messages Apr 16, 2008 – Apr 18, 2008 MIPS

lore
Share & tools
lore
#0 lore raw

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.

#1 lore raw

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.

Quoted 15 lines · Kevin D. Kissell

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.

#2 lore raw
Quoted 4 lines · Kevin D. Kissell

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

#3 lore raw
In reply to Kevin D. Kissell
Quoted 10 lines · Kevin D. Kissell
Quoted 1 line

From d164aa1b9dba720ee08a6a773e1a81b62aea7d10 Mon Sep 17 00:00:00 2001

From: Kevin D. Kissell <[email protected]>
Date: Thu, 10 Apr 2008 02:07:38 +0200
Subject: [PATCH] Fixes necessary for non-SMP kernels and non-relocatable binaries

(quoted diff) +0 -2
 		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;

Review

Quoted 10 lines
(quoted diff) +6 -4
-
-			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

#4 lore raw
In reply to Kevin D. Kissell
Quoted 8 lines · Kevin D. Kissell
 arch/mips/kernel/setup.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
arch/mips/kernel/setup.c +0 -0
index f8a535a..a6a0d62 100644

arch/mips/kernel/setup.c static void __init bootmem_init(void) · line 342

Quoted 7 lines
arch/mips/kernel/setup.c +4 -0
@@ -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

#5 lore raw
Quoted 6 lines · Ralf Baechle
Quoted 3 lines · Kevin D. Kissell
 arch/mips/kernel/setup.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Quoted 17 lines

arch/mips/kernel/setup.c static void __init bootmem_init(void) · line 343

Quoted 13 lines
arch/mips/kernel/setup.c +4 -0
index f8a535a..a6a0d62 100644
@@ -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.

#6 lore raw
Quoted 11 lines · Kevin D. Kissell
Quoted 3 lines

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

#7 lore raw
In reply to Kevin D. Kissell
Quoted 12 lines · Kevin D. Kissell
arch/mips/kernel/kspd.c +1 -1
@@ -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);

arch/mips/kernel/kspd.c static void sp_cleanup(void) · line 328

Quoted 6 lines
arch/mips/kernel/kspd.c +3 -0
@@ -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 ...

Quoted 106 lines
arch/mips/kernel/kspd.c +0 -0
 }
 
 static int channel_open = 0;
arch/mips/kernel/rtlx.c +41 -31
index 1ba00c1..c0bb347 100644
@@ -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;
+		    }
 		}

arch/mips/kernel/rtlx.c int rtlx_open(int index, int can_sleep) · line 220

Quoted 10 lines
arch/mips/kernel/rtlx.c +4 -4
 
 		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.

arch/mips/kernel/rtlx.c out_ret: · line 247

Quoted 9 lines
arch/mips/kernel/rtlx.c +2 -0
 		}
 
 		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().

arch/mips/kernel/rtlx.c unsigned int rtlx_read_poll(int index, int can_sleep) · line 270

Quoted 13 lines
arch/mips/kernel/rtlx.c +4 -2
+ 		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 ;-)

Quoted 26 lines
arch/mips/kernel/rtlx.c +5 -3
 			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);

arch/mips/kernel/rtlx.c static int __init rtlx_module_init(void) · line 535

Quoted 5 lines
arch/mips/kernel/rtlx.c +2 -0
@@ -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().

Quoted 16 lines
arch/mips/kernel/rtlx.c +3 -0
+		err = -ENODEV;
+		goto out_chrdev;
+	}
 
 	rtlx_irq.dev_id = rtlx;
 	setup_irq(rtlx_irq_num, &rtlx_irq);
include/asm-mips/rtlx.h +1 -1
index 65778c8..20b6660 100644
@@ -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,

include/asm-mips/rtlx.h extern unsigned int rtlx_read_poll(int index, int can_sleep); · line 41

Quoted 10 lines
include/asm-mips/rtlx.h +1 -1
 	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

#8 lore raw
Quoted 9 lines · Ralf Baechle
Quoted 6 lines · Kevin D. Kissell
arch/mips/kernel/kspd.c +0 -0
@@ -257,7 +257,7 @@ void sp_work_handle_request(void)
 
 		vcwd = vpe_getcwd(tclimit);
 
Quoted 16 lines

arch/mips/kernel/kspd.c static void sp_cleanup(void) · line 329

Quoted 13 lines
arch/mips/kernel/kspd.c +4 -1
- 		/* 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.