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

[Viktor Mitin <viktor.mitin.19@xxxxxxxxx>]

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.
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.

> 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?

>
> >
> > 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.

> >  {
> >      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?

Thanks

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