linux-kernel - Re: [PATCH V13 1/5] of: dynamic: Add interfaces for creating device node dynamically

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZP99DdtruXfI6L/L@smile.fi.intel.com>
Date:   Mon, 11 Sep 2023 23:48:13 +0300
From:   Andy Shevchenko <andriy.shevchenko@...el.com>
To:     Lizhi Hou <lizhi.hou@....com>
Cc:     linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, robh@...nel.org, max.zhen@....com,
        sonal.santan@....com, stefano.stabellini@...inx.com,
        Clément Léger <clement.leger@...tlin.com>
Subject: Re: [PATCH V13 1/5] of: dynamic: Add interfaces for creating device
 node dynamically

On Tue, Aug 15, 2023 at 10:19:56AM -0700, Lizhi Hou wrote:
> of_changeset_create_node() creates device node dynamically and attaches
> the newly created node to a changeset.
> 
> Expand of_changeset APIs to handle specific types of properties.
>     of_changeset_add_prop_string()
>     of_changeset_add_prop_string_array()
>     of_changeset_add_prop_u32_array()

...

> +/**
> + * of_changeset_add_prop_string - Add a string property to a changeset
> + *
> + * @ocs:	changeset pointer
> + * @np:		device node pointer
> + * @prop_name:	name of the property to be added
> + * @str:	pointer to null terminated string
> + *
> + * Create a string property and add it to a changeset.
> + *
> + * Return: 0 on success, a negative error value in case of an error.
> + */
> +int of_changeset_add_prop_string(struct of_changeset *ocs,
> +				 struct device_node *np,
> +				 const char *prop_name, const char *str)
> +{
> +	struct property prop;
> +
> +	prop.name = (char *)prop_name;

This looks not nice. You dropped const qualifier, which is a bit worrying.
Can  you fix underneath APIs/data types so we can avoid this?

> +	prop.length = strlen(str) + 1;

> +	prop.value = (void *)str;

Do you need this casting?

Okay, seems also related to the const qualifier. I would accept this in a form
of const void *, but it will probably look a bit weird. What about to have a value
to be a union?

> +	return of_changeset_add_prop_helper(ocs, np, &prop);
> +}

...

> +	prop.name = (char *)prop_name;

Same comment as per above.

...

> +	prop.length = 0;
> +	for (i = 0; i < sz; i++)
> +		prop.length += strlen(str_array[i]) + 1;
> +
> +	prop.value = kmalloc(prop.length, GFP_KERNEL);
> +	if (!prop.value)
> +		return -ENOMEM;
> +
> +	vp = prop.value;
> +	for (i = 0; i < sz; i++) {
> +		vp += snprintf(vp, (char *)prop.value + prop.length - vp, "%s",
> +			       str_array[i]) + 1;
> +	}

Is all this kinda of reinventing kasprintf()? Perhaps you can somehow utilize
kasprintf_strarray()? It might require to get a common denominator that takes
a formatting string as a parameter.

> +	ret = of_changeset_add_prop_helper(ocs, np, &prop);
> +	kfree(prop.value);

...

> +	for (i = 0; i < sz; i++)
> +		val[i] = cpu_to_be32(array[i]);

NIH cpu_to_be32_array()

...

> +	prop.name = (char *)prop_name;
> +	prop.length = sizeof(u32) * sz;
> +	prop.value = (void *)val;

Do you need this casting?

-- 
With Best Regards,
Andy Shevchenko