Re: [PATCH 4/9] device property: provide fwnode_property_get_names()

[Andy Shevchenko]

On Wed, Nov 04, 2026 at 03:03:46PM +0100, Bartosz Golaszewski wrote:
> Provide fwnode_property_get_names() - a new fwnode function that allows
> callers to retrieve the list of all properties of given firmware node.

...

>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/export.h>
> +#include <linux/minmax.h>
>  #include <linux/of.h>
>  #include <linux/property.h>
>  #include <linux/phy.h>
>  #include <linux/slab.h>

>  #include <linux/string.h>
> +#include <linux/string_helpers.h>

You can replace string.h with string_helpers.h.

We already rely on the fact that the latter includes the former
elsewhere in the kernel.

>  #include <linux/types.h>

> +/**
> + * fwnode_property_get_names - get the list of all properties of this fwnode
> + * @fwnode: Firmware node to get the list of properties for
> + * @props: Pointer to an array of strings. It will contain the address of the
> + *	   string list containing the property names.
> + *
> + * Return: Number of property names written into *props. If > 0, the caller must
> + *	   free the array with kfree_strarray(). *props is only valid for
> + *	   a positive return value.
> + *	   %-EINVAL if arguments are not valid
> + *	   %-ENXIO if no suitable firmware interface is present
> + *	   %-ENOMEM if memory allocation failed

> + * The list also includes the properties of the secondary firmware node, if
> + * any. Properties present in both nodes are listed once.

I don't know if this paragraph is for Return section or not. If not, it should
go before as the Return section must be the last one in the kernel-doc (it's
documented). Otherwise I'm not sure it belongs to the Return section. I think
it would be better as a part of the description which should elaborate on the
returning values in the parameter.

> + */
> +int fwnode_property_get_names(const struct fwnode_handle *fwnode, char ***props)

Triple indirection is rare. Do we really need it?

> +{
> +	int ret1, ret2 = -ENXIO, n1, n2, i, j, count;
> +	char **prim = NULL, **sec = NULL, **merged;
> +
> +	if (IS_ERR_OR_NULL(fwnode))
> +		return -EINVAL;
> +
> +	ret1 = fwnode_call_int_op(fwnode, property_get_names, &prim);
> +	if (!IS_ERR_OR_NULL(fwnode->secondary))
> +		ret2 = fwnode_call_int_op(fwnode->secondary, property_get_names, &sec);
> +
> +	if (ret1 == -ENXIO && ret2 == -ENXIO)
> +		return -ENXIO;
> +
> +	n1 = max(ret1, 0);
> +	n2 = max(ret2, 0);
> +
> +	CLASS(strarray, prim_props)(prim, n1);
> +	CLASS(strarray, sec_props)(sec, n2);
> +
> +	if (ret1 < 0 && ret1 != -ENXIO)
> +		return ret1;
> +
> +	if (ret2 < 0 && ret2 != -ENXIO)
> +		return ret2;

> +	if (n1 == 0 && n2 == 0)
> +		return 0;

Shouldn't these checks be before any CLASS()? Otherwise seems strange to me.

> +	merged = kcalloc(n1 + n2, sizeof(*merged), GFP_KERNEL);

Shouldn't here be something from overflow.h?

> +	if (!merged)
> +		return -ENOMEM;
> +
> +	/* The strings are now owned by merged, only free the container. */
> +	for (i = 0; i < n1; i++) {
> +		merged[i] = prim_props.array[i];
> +		prim_props.array[i] = NULL;
> +	}
> +	count = n1;
> +
> +	/* Properties from the primary node take precedence. */
> +	for (i = 0; i < n2; i++) {

> +		for (j = 0; j < n1; j++) {
> +			if (!strcmp(merged[j], sec_props.array[i]))
> +				break;
> +		}

match_string() ?

> +		if (j >= n1) {
> +			merged[count++] = sec_props.array[i];
> +			sec_props.array[i] = NULL;
> +		}
> +	}
> +
> +	*props = merged;
> +	return count;
> +}

-- 
With Best Regards,
Andy Shevchenko