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

[Julien Grall <julien.grall@xxxxxxx>]

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