Re: [PATCH] xen/device_tree: silence ambiguous integer casting warning error

[Stefano Stabellini <sstabellini@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6216 bytes --]

On Wed, 20 Apr 2022, Paran Lee wrote:
> GCC with "-g -Wall -Wextra" option throws warning message as below:
> 
> error: comparison of integer expressions of different signedness:
>  ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
> 
> Silence the warning by correcting the integer type.
> 
> Signed-off-by: Paran Lee <p4ranlee@gmail.com>

Thanks for the patch, this is a good improvement.



> ---
>  xen/common/device_tree.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 4aae281e89..402e465c7a 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -79,7 +79,7 @@ struct dt_bus
>      const char *addresses;
>      bool_t (*match)(const struct dt_device_node *node);
>      void (*count_cells)(const struct dt_device_node *child,
> -                        int *addrc, int *sizec);
> +                        unsigned int *addrc, unsigned int *sizec);
>      u64 (*map)(__be32 *addr, const __be32 *range, int na, int ns, int pna);
>      int (*translate)(__be32 *addr, u64 offset, int na);
>      unsigned int (*get_flags)(const __be32 *addr);
> @@ -569,7 +569,7 @@ static bool_t dt_bus_default_match(const struct dt_device_node *node)
>  }
>  
>  static void dt_bus_default_count_cells(const struct dt_device_node *dev,
> -                                int *addrc, int *sizec)
> +                                       unsigned int *addrc, unsigned int *sizec)
>  {
>      if ( addrc )
>          *addrc = dt_n_addr_cells(dev);
> @@ -649,7 +649,7 @@ static bool_t dt_bus_pci_match(const struct dt_device_node *np)
>  }
>  
>  static void dt_bus_pci_count_cells(const struct dt_device_node *np,
> -				   int *addrc, int *sizec)
> +				   unsigned int *addrc, unsigned int *sizec)
>  {
>      if (addrc)
>          *addrc = 3;

Although I don't necessarily mind these two changes to
dt_bus_default_count_cells and dt_bus_pci_count_cells as they are
improvements, I don't see how they are related to the "comparison of
integer expressions of different signedness" problem.

I definitely see the comparison between "i" and "imaplen" below for
instance but where is the problematic integers comparison in the case of
"na" and "ns"?

It would help to post the GCC output that you are getting with all the
warnings for this file.


> @@ -737,7 +737,7 @@ static const struct dt_bus dt_busses[] =
>  
>  static const struct dt_bus *dt_match_bus(const struct dt_device_node *np)
>  {
> -    int i;
> +    long unsigned int i;

Just write is as "unsigned long" for uniformity with the rest of the
code


>      for ( i = 0; i < ARRAY_SIZE(dt_busses); i++ )
>          if ( !dt_busses[i].match || dt_busses[i].match(np) )
> @@ -754,7 +754,8 @@ static const __be32 *dt_get_address(const struct dt_device_node *dev,
>      u32 psize;
>      const struct dt_device_node *parent;
>      const struct dt_bus *bus;
> -    int onesize, i, na, ns;
> +    unsigned int i;
> +    unsigned int onesize, na, ns;
>  
>      /* Get parent & match bus type */
>      parent = dt_get_parent(dev);
> @@ -797,8 +798,7 @@ static int dt_translate_one(const struct dt_device_node *parent,
>                              int pna, const char *rprop)
>  {
>      const __be32 *ranges;
> -    unsigned int rlen;
> -    int rone;
> +    unsigned int rlen, rone;
>      u64 offset = DT_BAD_ADDR;
>  
>      ranges = dt_get_property(parent, rprop, &rlen);
> @@ -857,7 +857,7 @@ static u64 __dt_translate_address(const struct dt_device_node *dev,
>      const struct dt_device_node *parent = NULL;
>      const struct dt_bus *bus, *pbus;
>      __be32 addr[DT_MAX_ADDR_CELLS];
> -    int na, ns, pna, pns;
> +    unsigned int na, ns, pna, pns;
>      u64 result = DT_BAD_ADDR;
>  
>      dt_dprintk("DT: ** translation for device %s **\n", dev->full_name);


In this function na and ns get printed a few times. We should convert
%d to %u.


Also, if we are going to change na and ns to unsigned int, then I think
we should also do it in dt_read_number and dt_translate_one. Not
necessarily in this patch though.


> @@ -966,8 +966,7 @@ int dt_for_each_range(const struct dt_device_node *dev,
>      const struct dt_bus *bus, *pbus;
>      const __be32 *ranges;
>      __be32 addr[DT_MAX_ADDR_CELLS];
> -    unsigned int rlen;
> -    int na, ns, pna, pns, rone;
> +    unsigned int rlen, na, ns, pna, pns, rone;
>  
>      bus = dt_match_bus(dev);
>      if ( !bus )
> @@ -1134,7 +1133,7 @@ unsigned int dt_number_of_address(const struct dt_device_node *dev)
>      u32 psize;
>      const struct dt_device_node *parent;
>      const struct dt_bus *bus;
> -    int onesize, na, ns;
> +    unsigned int onesize, na, ns;
>  
>      /* Get parent & match bus type */
>      parent = dt_get_parent(dev);
> @@ -1169,8 +1168,8 @@ int dt_for_each_irq_map(const struct dt_device_node *dev,
>      const struct dt_device_node *ipar, *tnode, *old = NULL;
>      const __be32 *tmp, *imap;
>      u32 intsize = 1, addrsize, pintsize = 0, paddrsize = 0;
> -    u32 imaplen;
> -    int i, ret;
> +    u32 i, imaplen;
> +    int ret;
>  
>      struct dt_raw_irq dt_raw_irq;
>      struct dt_irq dt_irq;
> @@ -1354,8 +1353,8 @@ static int dt_irq_map_raw(const struct dt_device_node *parent,
>      const struct dt_device_node *ipar, *tnode, *old = NULL, *newpar = NULL;
>      const __be32 *tmp, *imap, *imask;
>      u32 intsize = 1, addrsize, newintsize = 0, newaddrsize = 0;
> -    u32 imaplen;
> -    int match, i;
> +    u32 i, imaplen;
> +    int match;
>  
>      dt_dprintk("dt_irq_map_raw: par=%s,intspec=[0x%08x 0x%08x...],ointsize=%d\n",
>                 parent->full_name, be32_to_cpup(intspec),
> @@ -1737,7 +1736,7 @@ static int __dt_parse_phandle_with_args(const struct dt_device_node *np,
>  
>              if ( out_args )
>              {
> -                int i;
> +                u32 i;
>  
>                  WARN_ON(count > MAX_PHANDLE_ARGS);
>                  if (count > MAX_PHANDLE_ARGS)

These changes are great, thanks! I wouldn't mind a patch that only
changes "i" in __dt_parse_phandle_with_args, dt_irq_map_raw,
dt_for_each_irq_map, dt_match_bus. Those changes are straightforward.