Re: [PATCH 05/11] dtc: Wrap phandle validity check

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

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

On Mon, Oct 12, 2020 at 05:19:42PM +0100, Andre Przywara wrote:
> In several places we check for a returned phandle value to be valid,
> for that it must not be 0 or "-1".
> 
> Wrap this check in a macro in dtc.h, and use ~0U instead of -1 on the
> way, to keep everything in the unsigned realm.

We can probably make this a bit safer still, if we made it a typed
inline, instead of a macro.

> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> ---
>  checks.c   | 10 +++++-----
>  dtc.h      |  2 ++
>  livetree.c |  4 ++--
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/checks.c b/checks.c
> index 17cb689..7cb9011 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -497,7 +497,7 @@ static cell_t check_phandle_prop(struct check *c, struct dt_info *dti,
>  
>  	phandle = propval_cell(prop);
>  
> -	if ((phandle == 0) || (phandle == -1)) {
> +	if (!FDT_VALID_PHANDLE(phandle)) {
>  		FAIL_PROP(c, dti, node, prop, "bad value (0x%x) in %s property",
>  		     phandle, prop->name);
>  		return 0;
> @@ -1379,14 +1379,14 @@ static void check_property_phandle_args(struct check *c,
>  	for (cell = 0; cell < prop->val.len / sizeof(cell_t); cell += cellsize + 1) {
>  		struct node *provider_node;
>  		struct property *cellprop;
> -		int phandle;
> +		cell_t phandle;
>  
>  		phandle = propval_cell_n(prop, cell);
>  		/*
>  		 * Some bindings use a cell value 0 or -1 to skip over optional
>  		 * entries when each index position has a specific definition.
>  		 */
> -		if (phandle == 0 || phandle == -1) {
> +		if (!FDT_VALID_PHANDLE(phandle)) {
>  			/* Give up if this is an overlay with external references */
>  			if (dti->dtsflags & DTSF_PLUGIN)
>  				break;
> @@ -1603,7 +1603,7 @@ static void check_interrupts_property(struct check *c,
>  		prop = get_property(parent, "interrupt-parent");
>  		if (prop) {
>  			phandle = propval_cell(prop);
> -			if ((phandle == 0) || (phandle == -1)) {
> +			if (!FDT_VALID_PHANDLE(phandle)) {
>  				/* Give up if this is an overlay with
>  				 * external references */
>  				if (dti->dtsflags & DTSF_PLUGIN)
> @@ -1760,7 +1760,7 @@ static struct node *get_remote_endpoint(struct check *c, struct dt_info *dti,
>  
>  	phandle = propval_cell(prop);
>  	/* Give up if this is an overlay with external references */
> -	if (phandle == 0 || phandle == -1)
> +	if (!FDT_VALID_PHANDLE(phandle))
>  		return NULL;
>  
>  	node = get_node_by_phandle(dti->dt, phandle);
> diff --git a/dtc.h b/dtc.h
> index a08f415..f2f96ec 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -49,6 +49,8 @@ extern int annotate;		/* annotate .dts with input source location */
>  #define PHANDLE_EPAPR	0x2
>  #define PHANDLE_BOTH	0x3
>  
> +#define FDT_VALID_PHANDLE(x)	((x) != 0 && (x) != ~0U)
> +
>  typedef uint32_t cell_t;
>  
>  static inline uint16_t dtb_ld16(const void *p)
> diff --git a/livetree.c b/livetree.c
> index 032df58..aea6095 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -559,7 +559,7 @@ struct node *get_node_by_phandle(struct node *tree, cell_t phandle)
>  {
>  	struct node *child, *node;
>  
> -	if ((phandle == 0) || (phandle == -1)) {
> +	if (!FDT_VALID_PHANDLE(phandle)) {
>  		assert(generate_fixups);
>  		return NULL;
>  	}
> @@ -594,7 +594,7 @@ cell_t get_node_phandle(struct node *root, struct node *node)
>  	static cell_t phandle = 1; /* FIXME: ick, static local */
>  	struct data d = empty_data;
>  
> -	if ((node->phandle != 0) && (node->phandle != -1))
> +	if (FDT_VALID_PHANDLE(node->phandle))
>  		return node->phandle;
>  
>  	while (get_node_by_phandle(root, phandle))

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]