[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v4 1/2] xen/arm: extend fdt_property_interrupts



Hi,

I am only going to comment on process. I will look at the rest later on.

On 31/07/2019 13:28, Viktor Mitin wrote:
Hi Volodymyr,

On Wed, Jul 31, 2019 at 3:11 PM Volodymyr Babchuk
<Volodymyr_Babchuk@xxxxxxxx> wrote:


Hi Viktor,

It is recommended (and probably required, but I can't find exact place
in the rules) to include cover letter if you are sending more that one
patch in series. This will ease up review process, because reviewer will
know what to expect in the series.
 > There is no such requirement, only recommendation.

It is a strong recommendation: "If you need to send more than one patches (which normally means you're sending a patch series with cover letter),".

I did not put it since this is simple short patch series and both
patches in this series have been discussed previously, so it is known
what it is about.

For a first, if you don't have a cover letter then the threading in e-mail client would look weird:
   [PATCH v4 1/2] xen/arm: extend fdt_property_interrupts
      |-> [PATCH v4 2/2] xen/arm: merge make_timer_node and make_timer_domU_node

I tend to hid anything within the thread so I have only one title. For the title it is not clear to me what's the purpose of the e-mail.

The cover letter is also used to keep a summary of what was discussed and the overall goal. It does not matter if it is just a few lines. This is also a good place to have a discussion of the overall series (i.e not specific to a patch).

Lastly, you may have new reviewers that haven't followed the previous discussion. You have also reviewers like me which receive a few hundreds e-mails per week (just counting my inbox so e-mail I am CCed to). While I have a good memory, I can't possibly remember everything single e-mails.

So the cover letter is a good place to explain what changes have been done between series. You can also do that per-patch.

Speaking about changelog, I would highly recommend to keep all the changelog from v1. This gives us an idea what happen over the review.


Viktor Mitin writes:

Extend fdt_property_interrupts to deal with other domain than the hwdom.

The prototype of fdt_property_interrupts() has been modified
to support both hwdom and domU in one function.

This is a preparatory for the patch "xen/arm: merge make_timer_node and
make_timer_domU_node". Original goal is to merge make_timer_node and
make_timer_domU_node functions. See discussion in e-mail, the Message-ID is:
<20190620103805.927-1-viktor.mitin.19@xxxxxxxxx>

Note: there is no functional changes introduced by this patch.
This is not completely true, because you change the way how phandle is
retrieved. Also, earlier you said that "fdt_property_interrupts() has
been modified to support both hwdom and domU in one function". This is
the functional change.

Phandle is retreved the same way:
in case of  hwdom it is dt_interrupt_controller->phandle
in case of domU it is GUEST_PHANDLE_GIC.
Don't see any functional change here.

What is 'functional change' in your opinion?

The function is now extended so technically it has changed. "No functional change" is usually used when the code is consolidated/reworked.




Suggested-by: Julien Grall <julien.grall@xxxxxxx>
Signed-off-by: Viktor Mitin <viktor_mitin@xxxxxxxx>
---
  xen/arch/arm/domain_build.c | 22 +++++++++++++---------
  1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4c8404155a..d04a1c3aec 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -621,17 +621,19 @@ static void __init set_interrupt(gic_interrupt_t 
interrupt,
   *  "interrupts": contains the list of interrupts
   *  "interrupt-parent": link to the GIC
   */
-static int __init fdt_property_interrupts(void *fdt, gic_interrupt_t *intr,
-                                          unsigned num_irq)
+static int __init fdt_property_interrupts(const struct kernel_info *kinfo,
+                            gic_interrupt_t *intr, unsigned num_irq)
As I said earlier, this formatting contradicts with the coding style.

There is no such coding style requirement (not explicitly documented).
Even more, the original code formatted the same way.

What original code? This is formatted with all the parameters aligned. So can you please do it.


  {
      int res;
+    uint32_t phandle = is_hardware_domain(kinfo->d) ?
+                       dt_interrupt_controller->phandle : GUEST_PHANDLE_GIC;

-    res = fdt_property(fdt, "interrupts", intr, sizeof (intr[0]) * num_irq);
+    res = fdt_property(kinfo->fdt, "interrupts",
+                       intr, sizeof (intr[0]) * num_irq);
      if ( res )
          return res;

-    res = fdt_property_cell(fdt, "interrupt-parent",
-                            dt_interrupt_controller->phandle);
+    res = fdt_property_cell(kinfo->fdt, "interrupt-parent", phandle);

      return res;
  }
@@ -733,7 +735,7 @@ static int __init make_hypervisor_node(struct domain *d,
       *  TODO: Handle properly the cpumask;
       */
      set_interrupt(intr, d->arch.evtchn_irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
-    res = fdt_property_interrupts(fdt, &intr, 1);
+    res = fdt_property_interrupts(kinfo, &intr, 1);
      if ( res )
          return res;

@@ -960,8 +962,10 @@ static int __init make_gic_node(const struct domain *d, 
void *fdt,
      return res;
  }

-static int __init make_timer_node(const struct domain *d, void *fdt)
+static int __init make_timer_node(const struct kernel_info *kinfo)
  {
+    void *fdt = kinfo->fdt;
+
No need for empty line there.

Why not? Is there any reason or pointers to the documents?

I think what Volodymyr means is the newline is not necessary here. So this could possibly be removed. The counter-argument to this is why do you need this newline?

In generally, we tend to keep all variables declarations together. But I would not argue on this.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.