| Message ID | 20190731102856.23215-1-viktor.mitin.19@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/2] xen/arm: extend fdt_property_interrupts | expand |
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. 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@gmail.com> > > 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. > > Suggested-by: Julien Grall <julien.grall@arm.com> > Signed-off-by: Viktor Mitin <viktor_mitin@epam.com> > --- > 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. > { > 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. > static const struct dt_device_match timer_ids[] __initconst = > { > DT_MATCH_COMPATIBLE("arm,armv7-timer"), > @@ -1016,7 +1020,7 @@ static int __init make_timer_node(const struct domain *d, void *fdt) > dt_dprintk(" Virt interrupt %u\n", irq); > set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW); > > - res = fdt_property_interrupts(fdt, intrs, 3); > + res = fdt_property_interrupts(kinfo, intrs, 3); > if ( res ) > return res; > > @@ -1377,7 +1381,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, > if ( device_get_class(node) == DEVICE_GIC ) > return make_gic_node(d, kinfo->fdt, node); > if ( dt_match_node(timer_matches, node) ) > - return make_timer_node(d, kinfo->fdt); > + return make_timer_node(kinfo); > > /* Skip nodes used by Xen */ > if ( dt_device_used_by(node) == DOMID_XEN ) -- Volodymyr Babchuk at EPAM
Hi Volodymyr, On Wed, Jul 31, 2019 at 3:11 PM Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> 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@gmail.com> > > > > 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@arm.com> > > Signed-off-by: Viktor Mitin <viktor_mitin@epam.com> > > --- > > 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
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@epam.com> 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@gmail.com> >>> >>> 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@arm.com> >>> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com> >>> --- >>> 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,
Hi, You should have enough characters in the title to explain what you are extending. Something like: xen/arm: Extend fdt_property_interrupts to support domU On 31/07/2019 11:28, Viktor Mitin wrote: > 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@gmail.com> The commit message should not point to discussion but summarize it. If you want to point to the discussion, then please do it after ---. Also, this is a bit risky to write down the title of a patch that does not preceded it (or been committed). Image we decide to rename it... Instead, it is common to say "A follow-up patch will need to create the interrupts for either dom0 or domU". > > Note: there is no functional changes introduced by this patch. You are expanding the function to this is technically a functional changes. > > Suggested-by: Julien Grall <julien.grall@arm.com> > Signed-off-by: Viktor Mitin <viktor_mitin@epam.com> > --- > 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) Please align the section line with the first argument. > { > 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) This change is still not explained in the commit message. > { > + void *fdt = kinfo->fdt; > + You add the newline here but drop it in the next patch. In general, it is strongly recommended to not add code this is removed in the same series unless there are a reason to. > static const struct dt_device_match timer_ids[] __initconst = > { > DT_MATCH_COMPATIBLE("arm,armv7-timer"), > @@ -1016,7 +1020,7 @@ static int __init make_timer_node(const struct domain *d, void *fdt) > dt_dprintk(" Virt interrupt %u\n", irq); > set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW); > > - res = fdt_property_interrupts(fdt, intrs, 3); > + res = fdt_property_interrupts(kinfo, intrs, 3); > if ( res ) > return res; > > @@ -1377,7 +1381,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, > if ( device_get_class(node) == DEVICE_GIC ) > return make_gic_node(d, kinfo->fdt, node); > if ( dt_match_node(timer_matches, node) ) > - return make_timer_node(d, kinfo->fdt); > + return make_timer_node(kinfo); > > /* Skip nodes used by Xen */ > if ( dt_device_used_by(node) == DOMID_XEN ) > Cheers,
Hi Julien and Volodymyr, On Wed, Jul 31, 2019 at 3:52 PM Julien Grall <julien.grall@arm.com> wrote: > > Hi, > > >> 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. Thank you for this great and detailed argumentation provided. It makes sense, so probably Xen patches wiki should be updated with this information and cover letter should become not a recommendation, but a rule. Thanks
Hi Viktor, On 01/08/2019 13:26, Viktor Mitin wrote: > Hi Julien and Volodymyr, > > On Wed, Jul 31, 2019 at 3:52 PM Julien Grall <julien.grall@arm.com> wrote: >> >> Hi, >> >>>> 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. > > Thank you for this great and detailed argumentation provided. It makes > sense, so probably Xen patches wiki should be updated with this > information and cover letter should become not a recommendation, but a > rule. Update to the wiki are always welcomed. Cheers,
> On 1 Aug 2019, at 13:41, Julien Grall <julien.grall@arm.com> wrote: > > Hi Viktor, > > On 01/08/2019 13:26, Viktor Mitin wrote: >> Hi Julien and Volodymyr, >> On Wed, Jul 31, 2019 at 3:52 PM Julien Grall <julien.grall@arm.com> wrote: >>> >>> Hi, >>> >>>>> 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. >> Thank you for this great and detailed argumentation provided. It makes >> sense, so probably Xen patches wiki should be updated with this >> information and cover letter should become not a recommendation, but a >> rule. > > Update to the wiki are always welcomed. I still have an action to rework https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches <https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches> and <https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patchesand> migrate the content to the sphinx docs @Victor: can you quickly point out where we recommend to use cover letters (if you remember). I thought it was a requirement Lars
Hi Lars, On 01/08/2019 14:59, Lars Kurth wrote: > > >> On 1 Aug 2019, at 13:41, Julien Grall <julien.grall@arm.com >> <mailto:julien.grall@arm.com>> wrote: >> >> Hi Viktor, >> >> On 01/08/2019 13:26, Viktor Mitin wrote: >>> Hi Julien and Volodymyr, >>> On Wed, Jul 31, 2019 at 3:52 PM Julien Grall <julien.grall@arm.com >>> <mailto:julien.grall@arm.com>> wrote: >>>> >>>> Hi, >>>> >>>>>> 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. >>> Thank you for this great and detailed argumentation provided. It makes >>> sense, so probably Xen patches wiki should be updated with this >>> information and cover letter should become not a recommendation, but a >>> rule. >> >> Update to the wiki are always welcomed. > > I still have an action to rework > https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches and > <https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patchesand> migrate the > content to the sphinx docs > > @Victor: can you quickly point out where we recommend to use cover letters (if > you remember). I thought it was a requirement It is not explicitly written in the wiki page. But it is implied in a few places such as in the section "Providing a git branch", "Using git send-email alone". > > Lars >
> On 1 Aug 2019, at 15:02, Julien Grall <julien.grall@arm.com> wrote: > > Hi Lars, > > On 01/08/2019 14:59, Lars Kurth wrote: >>> On 1 Aug 2019, at 13:41, Julien Grall <julien.grall@arm.com <mailto:julien.grall@arm.com>> wrote: >>> >>> Hi Viktor, >>> >>> On 01/08/2019 13:26, Viktor Mitin wrote: >>>> Hi Julien and Volodymyr, >>>> On Wed, Jul 31, 2019 at 3:52 PM Julien Grall <julien.grall@arm.com <mailto:julien.grall@arm.com>> wrote: >>>>> >>>>> Hi, >>>>> >>>>>>> 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. >>>> Thank you for this great and detailed argumentation provided. It makes >>>> sense, so probably Xen patches wiki should be updated with this >>>> information and cover letter should become not a recommendation, but a >>>> rule. >>> >>> Update to the wiki are always welcomed. >> I still have an action to rework https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches and <https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patchesand> migrate the content to the sphinx docs >> @Victor: can you quickly point out where we recommend to use cover letters (if you remember). I thought it was a requirement > > It is not explicitly written in the wiki page. But it is implied in a few places such as in the section "Providing a git branch", "Using git send-email alone". You are right. That should be made explicit. I think you are the only person in years that sent a series without cover letter I will have a go over that page in the next few days reducing the options and making it more strict and clear It would be good if you could go over it, once I am done, and let me know whether it is clearer Lars
> > It is not explicitly written in the wiki page. But it is implied in a few places such as in the section "Providing a git branch", "Using git send-email alone". > > You are right. That should be made explicit. I think you are the only person in years that sent a series without cover letter > > I will have a go over that page in the next few days reducing the options and making it more strict and clear > > It would be good if you could go over it, once I am done, and let me know whether it is clearer Lars, sure, will go over it once it ready. BTW what is the policy for editing Xen wiki? Is there some actual pointer to read about it, for example, who is allowed to edit it, what are the rules, etc? Thanks
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) { 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; + static const struct dt_device_match timer_ids[] __initconst = { DT_MATCH_COMPATIBLE("arm,armv7-timer"), @@ -1016,7 +1020,7 @@ static int __init make_timer_node(const struct domain *d, void *fdt) dt_dprintk(" Virt interrupt %u\n", irq); set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW); - res = fdt_property_interrupts(fdt, intrs, 3); + res = fdt_property_interrupts(kinfo, intrs, 3); if ( res ) return res; @@ -1377,7 +1381,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, if ( device_get_class(node) == DEVICE_GIC ) return make_gic_node(d, kinfo->fdt, node); if ( dt_match_node(timer_matches, node) ) - return make_timer_node(d, kinfo->fdt); + return make_timer_node(kinfo); /* Skip nodes used by Xen */ if ( dt_device_used_by(node) == DOMID_XEN )
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@gmail.com> Note: there is no functional changes introduced by this patch. Suggested-by: Julien Grall <julien.grall@arm.com> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com> --- xen/arch/arm/domain_build.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)