[Top] [All Lists]

Re: [PATCH v2 1/2] MIPS: fix/enrich 34K APRP (APSP) functionalities

To: "Maciej W. Rozycki" <>
Subject: Re: [PATCH v2 1/2] MIPS: fix/enrich 34K APRP (APSP) functionalities
From: Deng-Cheng Zhu <>
Date: Wed, 23 May 2012 17:07:05 +0800
Cc: John Crispin <>, <>, <>
In-reply-to: <>
References: <> <> <> <> <> <> <> <> <> <>
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv: Gecko/20120216 Lightning/1.0b2 Thunderbird/3.1.19
Thanks for your detailed explanation! Please see my comments below.

On 05/23/2012 06:12 AM, Maciej W. Rozycki wrote:
On Tue, 22 May 2012, Deng-Cheng Zhu wrote:

   Hmm, there's nothing platform-specific there, the file is pretty generic,
it could be moved to arch/mips/kernel/ or thereabouts.  That applies to
<asm/mips-boards/launch.h>   too, before you ask

Yeah, agree with you. I didn't do it simply because I'm not sure :)

  I can see you've copied the whole contents over to arch/mips/kernel/vpe.c
now.  Please don't do that.  This code is modular for a reason.  Either
modify original code to suit your needs while preserving functionality or,
if infeasible, copy it over to a new module selected based on
configuration.  Common parts should be abstracted and extracted to a
common dependency, either a shared header or another module, as

OK. Good advice!

(you may want to use alloc_bootmem or suchlike instead of hardcoding the
trampoline page, though it's probably pretty safe to assume the end of
the exception handler page is available everywhere).

I'm not quite clear about this. Do you mean to bypass the arbitrary monitor
in vpe_run() (in other words, to directly bring up the vpe in vpe_run())?
Why do we need to worry about writing to the cpulaunch data?

  The location of RAM is platform-specific, CKSEG0ADDR mustn't be used to
"allocate" kernel memory.  But as I say the exception handlers' page is
generally pretty safe, although the addition of the CP0 EBase register to
the architecture stopped it being guaranteed at one point.

  Ultimately I think this memory should be properly allocated, but this is
preexisting code, so there is no requirement that you fix that on this
occasion (or at all), unless you'd really like to.

OK. I'll let it be for now.

   There's nothing platform-specific referred to from arch/mips/kernel/vpe.c
AFAICT (and I trust in Beth having got this piece right).  I reckon it
used to work with CONFIG_MIPS_SIM too, though I could imagine the
configuration got neglected a bit as it is somewhat unusual.

Oh, When I said IRQ related stuff I meant the interrupt specific changes in
rtlx.c (not vpe.c) which correspond to those in malta-int.c. They are
there to resolve some issues (Please refer to the code changes and added
comments in these 2 files in PATCH #1 and #2.).

  I still can't see anything platform-specific in rtlx.c (also written by
Beth, BTW) -- it's all software interrupts that are architectural.  What
pieces of code and comments are you specifically referring to?

I meant some interrupt specific changes in rtlx.c correspond to platform-
specific ones in malta-int.c. You may simply refer to the latter for the
issues I addressed. The changes to malta-int.c led to the platform
dependency and it seems the issues could not be tackled in generic layer.

  Also in some places you do stuff like:

int foo(void)
int foo(void)
[something else...]

Please don't.  Again these pieces should be separate modules selected by
configuration, e.g. rtlx.c, rtlx-mt.c and rtlx-cmp.c with the former
holding the common stuff, and the two latters non-CMP- and CMP-specific
pieces, respectively (assuming that they are mutually exclusive and can't
be enabled both at a time in a single kernel binary for some reason).

Thanks, good point.

  It may make sense to move this whole stuff to a subdirectory at one

Do you mean to move things like vpe.c, kspd.c and rtlx.c (and the proposed
-mt/-cmp variants) into a directory such as arch/mips/kernel/aprp/?


<Prev in Thread] Current Thread [Next in Thread>