| Message ID | 10e7f409-b074-71d9-62c8-cfb7c225679e@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Artem, On 06/12/16 13:57, Artem Mygaiev wrote: > Remove unused tnode variables in device tree IRQ map parsing This description is slightly wrong, the variable are used but not the value. > Coverity-ID 1381866, 1381867 > > Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com> This code is from Linux (see drivers/of/irq.c) and I would prefer to keep it very similar as long as it does not lead to a bug: Nacked-by: Julien Grall <julien.grall@arm.com> > --- > xen/common/device_tree.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index 1be074b..edc48f6 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -1053,7 +1053,7 @@ int dt_for_each_irq_map(const struct dt_device_node *dev, > void *), > void *data) > { > - const struct dt_device_node *ipar, *tnode, *old = NULL; > + const struct dt_device_node *ipar, *old = NULL; > const __be32 *tmp, *imap; > u32 intsize = 1, addrsize, pintsize = 0, paddrsize = 0; > u32 imaplen; > @@ -1078,7 +1078,6 @@ int dt_for_each_irq_map(const struct dt_device_node *dev, > intsize = be32_to_cpu(*tmp); > break; > } > - tnode = ipar; > ipar = dt_irq_find_parent(ipar); > } while ( ipar ); > if ( ipar == NULL ) > @@ -1101,8 +1100,7 @@ int dt_for_each_irq_map(const struct dt_device_node *dev, > old = ipar; > do { > tmp = dt_get_property(old, "#address-cells", NULL); > - tnode = dt_get_parent(old); > - old = tnode; > + old = dt_get_parent(old); > } while ( old && tmp == NULL ); > > old = NULL; > @@ -1238,7 +1236,7 @@ static int dt_irq_map_raw(const struct dt_device_node *parent, > const __be32 *addr, > struct dt_raw_irq *oirq) > { > - const struct dt_device_node *ipar, *tnode, *old = NULL, *newpar = NULL; > + const struct dt_device_node *ipar, *old = NULL, *newpar = NULL; > const __be32 *tmp, *imap, *imask; > u32 intsize = 1, addrsize, newintsize = 0, newaddrsize = 0; > u32 imaplen; > @@ -1261,7 +1259,6 @@ static int dt_irq_map_raw(const struct dt_device_node *parent, > intsize = be32_to_cpu(*tmp); > break; > } > - tnode = ipar; > ipar = dt_irq_find_parent(ipar); > } while ( ipar ); > if ( ipar == NULL ) > @@ -1281,8 +1278,7 @@ static int dt_irq_map_raw(const struct dt_device_node *parent, > old = ipar; > do { > tmp = dt_get_property(old, "#address-cells", NULL); > - tnode = dt_get_parent(old); > - old = tnode; > + old = dt_get_parent(old); > } while ( old && tmp == NULL ); > > old = NULL; >
Hi Julien On 06.12.16 16:19, Julien Grall wrote: > Hi Artem, > > On 06/12/16 13:57, Artem Mygaiev wrote: >> Remove unused tnode variables in device tree IRQ map parsing > > This description is slightly wrong, the variable are used but not the value. True > >> Coverity-ID 1381866, 1381867 >> >> Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com> > > This code is from Linux (see drivers/of/irq.c) and I would prefer to keep it very similar as long as it does not lead to a bug: I understand, but in the kernel code tnode is used to safely decrement kobj refcount (e.g. in 4.8.12): 144 tnode = of_get_parent(old); 145 of_node_put(old); 146 old = tnode; AFAIU in Xen refcounts are dropped, so probably tnode can be dropped as a part of the infrastructure... no? > Nacked-by: Julien Grall <julien.grall@arm.com> > >> --- >> xen/common/device_tree.c | 12 ++++-------- >> 1 file changed, 4 insertions(+), 8 deletions(-) >> >> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c >> index 1be074b..edc48f6 100644 >> --- a/xen/common/device_tree.c >> +++ b/xen/common/device_tree.c >> @@ -1053,7 +1053,7 @@ int dt_for_each_irq_map(const struct dt_device_node *dev, >> void *), >> void *data) >> { >> - const struct dt_device_node *ipar, *tnode, *old = NULL; >> + const struct dt_device_node *ipar, *old = NULL; >> const __be32 *tmp, *imap; >> u32 intsize = 1, addrsize, pintsize = 0, paddrsize = 0; >> u32 imaplen; >> @@ -1078,7 +1078,6 @@ int dt_for_each_irq_map(const struct dt_device_node *dev, >> intsize = be32_to_cpu(*tmp); >> break; >> } >> - tnode = ipar; >> ipar = dt_irq_find_parent(ipar); >> } while ( ipar ); >> if ( ipar == NULL ) >> @@ -1101,8 +1100,7 @@ int dt_for_each_irq_map(const struct dt_device_node *dev, >> old = ipar; >> do { >> tmp = dt_get_property(old, "#address-cells", NULL); >> - tnode = dt_get_parent(old); >> - old = tnode; >> + old = dt_get_parent(old); >> } while ( old && tmp == NULL ); >> >> old = NULL; >> @@ -1238,7 +1236,7 @@ static int dt_irq_map_raw(const struct dt_device_node *parent, >> const __be32 *addr, >> struct dt_raw_irq *oirq) >> { >> - const struct dt_device_node *ipar, *tnode, *old = NULL, *newpar = NULL; >> + const struct dt_device_node *ipar, *old = NULL, *newpar = NULL; >> const __be32 *tmp, *imap, *imask; >> u32 intsize = 1, addrsize, newintsize = 0, newaddrsize = 0; >> u32 imaplen; >> @@ -1261,7 +1259,6 @@ static int dt_irq_map_raw(const struct dt_device_node *parent, >> intsize = be32_to_cpu(*tmp); >> break; >> } >> - tnode = ipar; >> ipar = dt_irq_find_parent(ipar); >> } while ( ipar ); >> if ( ipar == NULL ) >> @@ -1281,8 +1278,7 @@ static int dt_irq_map_raw(const struct dt_device_node *parent, >> old = ipar; >> do { >> tmp = dt_get_property(old, "#address-cells", NULL); >> - tnode = dt_get_parent(old); >> - old = tnode; >> + old = dt_get_parent(old); >> } while ( old && tmp == NULL ); >> >> old = NULL; >> >
On 06/12/16 14:33, Artem Mygaiev wrote: > Hi Julien > > > On 06.12.16 16:19, Julien Grall wrote: >> Hi Artem, >> >> On 06/12/16 13:57, Artem Mygaiev wrote: >>> Remove unused tnode variables in device tree IRQ map parsing >> >> This description is slightly wrong, the variable are used but not the value. > True >> >>> Coverity-ID 1381866, 1381867 >>> >>> Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com> >> >> This code is from Linux (see drivers/of/irq.c) and I would prefer to keep it very similar as long as it does not lead to a bug: > I understand, but in the kernel code tnode is used to safely decrement kobj refcount (e.g. in 4.8.12): > > 144 tnode = of_get_parent(old); > 145 of_node_put(old); > 146 old = tnode; > > AFAIU in Xen refcounts are dropped, so probably tnode can be dropped as a part of the infrastructure... no? I have an item in my todo list to re-sync the device-tree code with Linux. So we have a proper copy rather than a modified one. Meanwhile, I would strongly prefer to keep this code as close as possible from Linux. This tnode variable is harmless, so I stand on what I said. Regards,
On 06.12.16 16:37, Julien Grall wrote: > > > On 06/12/16 14:33, Artem Mygaiev wrote: >> Hi Julien >> >> >> On 06.12.16 16:19, Julien Grall wrote: >>> Hi Artem, >>> >>> On 06/12/16 13:57, Artem Mygaiev wrote: >>>> Remove unused tnode variables in device tree IRQ map parsing >>> >>> This description is slightly wrong, the variable are used but not the value. >> True >>> >>>> Coverity-ID 1381866, 1381867 >>>> >>>> Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com> >>> >>> This code is from Linux (see drivers/of/irq.c) and I would prefer to keep it very similar as long as it does not lead to a bug: >> I understand, but in the kernel code tnode is used to safely decrement kobj refcount (e.g. in 4.8.12): >> >> 144 tnode = of_get_parent(old); >> 145 of_node_put(old); >> 146 old = tnode; >> >> AFAIU in Xen refcounts are dropped, so probably tnode can be dropped as a part of the infrastructure... no? > > I have an item in my todo list to re-sync the device-tree code with Linux. So we have a proper copy rather than a modified one. Oh, I see, thanks. > > Meanwhile, I would strongly prefer to keep this code as close as possible from Linux. This tnode variable is harmless, so I stand on what I said. > > Regards, >
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index 1be074b..edc48f6 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -1053,7 +1053,7 @@ int dt_for_each_irq_map(const struct dt_device_node *dev, void *), void *data) { - const struct dt_device_node *ipar, *tnode, *old = NULL; + const struct dt_device_node *ipar, *old = NULL; const __be32 *tmp, *imap; u32 intsize = 1, addrsize, pintsize = 0, paddrsize = 0; u32 imaplen; @@ -1078,7 +1078,6 @@ int dt_for_each_irq_map(const struct dt_device_node *dev, intsize = be32_to_cpu(*tmp); break; } - tnode = ipar; ipar = dt_irq_find_parent(ipar); } while ( ipar ); if ( ipar == NULL ) @@ -1101,8 +1100,7 @@ int dt_for_each_irq_map(const struct dt_device_node *dev, old = ipar; do { tmp = dt_get_property(old, "#address-cells", NULL); - tnode = dt_get_parent(old); - old = tnode; + old = dt_get_parent(old); } while ( old && tmp == NULL ); old = NULL; @@ -1238,7 +1236,7 @@ static int dt_irq_map_raw(const struct dt_device_node *parent, const __be32 *addr, struct dt_raw_irq *oirq) { - const struct dt_device_node *ipar, *tnode, *old = NULL, *newpar = NULL; + const struct dt_device_node *ipar, *old = NULL, *newpar = NULL; const __be32 *tmp, *imap, *imask; u32 intsize = 1, addrsize, newintsize = 0, newaddrsize = 0; u32 imaplen; @@ -1261,7 +1259,6 @@ static int dt_irq_map_raw(const struct dt_device_node *parent, intsize = be32_to_cpu(*tmp); break; } - tnode = ipar; ipar = dt_irq_find_parent(ipar); } while ( ipar ); if ( ipar == NULL ) @@ -1281,8 +1278,7 @@ static int dt_irq_map_raw(const struct dt_device_node *parent, old = ipar; do { tmp = dt_get_property(old, "#address-cells", NULL); - tnode = dt_get_parent(old); - old = tnode; + old = dt_get_parent(old); } while ( old && tmp == NULL ); old = NULL;
Remove unused tnode variables in device tree IRQ map parsing Coverity-ID 1381866, 1381867 Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com> --- xen/common/device_tree.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)