<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p><br>
</p>
<div class="moz-cite-prefix">On 03/07/19 9:34 AM, Oliver O'Halloran
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:8a779e216bb088b33b022cd026bdb647e05aa338.camel@gmail.com">
<pre class="moz-quote-pre" wrap="">On Wed, 2019-06-26 at 02:16 +0530, Hari Bathini wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Introduce callbacks for platform specific operations like register,
unregister, invalidate & such, and move pseries specific code into
platform code.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Please don't move around large blocks of code *and* change the code in
a single patch. It makes reviewing the changes extremely tedious since
the changes are mixed in with hundreds of lines of nothing.
</pre>
</blockquote>
<pre>
Right, Oliver.
I am already working on splitting up few other patches for ease of reviewing.
Thought of keeping this patch this way though as it doesn't add any new logic
but moves the code around. Will ensure I split this one up too for the sake
of sanity.
</pre>
<blockquote type="cite"
cite="mid:8a779e216bb088b33b022cd026bdb647e05aa338.camel@gmail.com">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Signed-off-by: Hari Bathini <a class="moz-txt-link-rfc2396E" href="mailto:hbathini@linux.ibm.com"><hbathini@linux.ibm.com></a>
---
arch/powerpc/include/asm/fadump.h | 75 ----
arch/powerpc/kernel/fadump-common.h | 38 ++
arch/powerpc/kernel/fadump.c | 500 ++-----------------------
arch/powerpc/platforms/pseries/Makefile | 1
arch/powerpc/platforms/pseries/rtas-fadump.c | 529 ++++++++++++++++++++++++++
arch/powerpc/platforms/pseries/rtas-fadump.h | 96 +++++
6 files changed, 700 insertions(+), 539 deletions(-)
create mode 100644 arch/powerpc/platforms/pseries/rtas-fadump.c
create mode 100644 arch/powerpc/platforms/pseries/rtas-fadump.h
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+static struct fadump_ops pseries_fadump_ops = {
+ .init_fadump_mem_struct = pseries_init_fadump_mem_struct,
+ .register_fadump = pseries_register_fadump,
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I realise you are just translating the existing interface, but why is
init_fadump_mem_struct() done as a seperate step and not as a part of
the registration function? The struct doesn't seem to be necessary
until the actual registration happens.</pre>
</blockquote>
<pre>
Yeah. But registration is a user choice and can happen multiple times within
a single boot (for example, due to hotplug operations) but the structure
contents remain the same. So, it is initialized once early on...
</pre>
<blockquote type="cite"
cite="mid:8a779e216bb088b33b022cd026bdb647e05aa338.camel@gmail.com">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ .unregister_fadump = pseries_unregister_fadump,
+ .invalidate_fadump = pseries_invalidate_fadump,
+ .process_fadump = pseries_process_fadump,
+ .fadump_region_show = pseries_fadump_region_show,
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ .crash_fadump = pseries_crash_fadump,
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Rename this to fadump_trigger or something, it's not clear what it
does.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Sure.
Thanks for the review!
- Hari
</pre>
</body>
</html>