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

[Julien Grall <julien@xen.org>]

Hi,

On 19/04/2022 16:46, Paran Lee wrote:
> GCC with "-g -Wall -Wextra" option throws warning message as below:

s/warning/error/

> 
> error: comparison of integer expressions of different signedness:
>   ‘int’ and ‘unsigned int’ [-Werror=sign-compare]

Can you post the full log?

> 
> Silence the warning by correcting the integer type.

IIRC most of the code you touch below is from Linux and had very limited 
changes afterwards. So while I agree the changes are good, AFAIK they 
are not (latent?) bugs and we don't build by default with -Wextra.

Therefore, I would prefer if they are upstreamed to Linux first and then 
backported to Xen.

> 
> Signed-off-by: Paran Lee <p4ranlee@gmail.com>
> ---
>   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);

Technically, the prototype of dt_n_addr_cells() is "int ... (". So now 
you are casting an "int" to an "unsigned int". AFAICT, dt_n_addr_cells() 
will never return a negative value. So we should propage the "unsigned int".

> @@ -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;
> @@ -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;

This should be size_t.

>   
>       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);
> @@ -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)

Cheers,

-- 
Julien Grall