Re: [PATCH v2 03/28] interconnect: qcom: rpmh: make nodes a NULL_terminated array

[Georgi Djakov <djakov@kernel.org>]

Hi Dmitry,

On 7/4/25 7:35 PM, Dmitry Baryshkov wrote:
> Having the .num_nodes as a separate struct field can provoke errors as
> it is easy to omit it or to put an incorrect value into that field. Turn
> .nodes into a NULL-terminated array, removing a need for a separate
> .num_nodes field. 

I am not entirely convinced that an error in the termination is more
unlikely than an error in the num_nodes. Aren't we trading one kind of
error for another? Also if we omit the num_nodes i expect that just the
QoS of a specific path will fail, but if we miss the NULL - worse things
might happen.

[..]
>   27 files changed, 541 insertions(+), 1119 deletions(-)

The negative diffstat is nice.

> 
> diff --git a/drivers/interconnect/qcom/bcm-voter.c b/drivers/interconnect/qcom/bcm-voter.c
> index a2d437a05a11fa7325f944865c81a3ac7dbb203e..4fa960630d28f338f484794d271a5b52f3e698d3 100644
> --- a/drivers/interconnect/qcom/bcm-voter.c
> +++ b/drivers/interconnect/qcom/bcm-voter.c
> @@ -68,7 +68,7 @@ static void bcm_aggregate_mask(struct qcom_icc_bcm *bcm)
>   		bcm->vote_x[bucket] = 0;
>   		bcm->vote_y[bucket] = 0;
>   
> -		for (i = 0; i < bcm->num_nodes; i++) {
> +		for (i = 0; bcm->nodes[i]; i++) {
>   			node = bcm->nodes[i];

I like better the single memory access and the two registers comparison
that we already have, but both are fine as we are not in some critical
section.

[..]
> diff --git a/drivers/interconnect/qcom/icc-rpmh.h b/drivers/interconnect/qcom/icc-rpmh.h
> index bd8d730249b1c9e5b37afbee485b9500a8028c2e..0018aa74187edcac9a0492c737771d957a133cc0 100644
> --- a/drivers/interconnect/qcom/icc-rpmh.h
> +++ b/drivers/interconnect/qcom/icc-rpmh.h
> @@ -126,7 +126,6 @@ struct qcom_icc_node {
>    * communicating with RPMh
>    * @list: used to link to other bcms when compiling lists for commit
>    * @ws_list: used to keep track of bcms that may transition between wake/sleep
> - * @num_nodes: total number of @num_nodes
>    * @nodes: list of qcom_icc_nodes that this BCM encapsulates
>    */
>   struct qcom_icc_bcm {
> @@ -142,7 +141,6 @@ struct qcom_icc_bcm {
>   	struct bcm_db aux_data;
>   	struct list_head list;
>   	struct list_head ws_list;
> -	size_t num_nodes;

So no change in memory footprint, as now instead of the num_nodes, there
will be an additional NULL pointer for termination.

[..]

Well, this approach is also good. The existing one just follows the pattern
used by other frameworks, that seemed more common and thus make the code
easier to read and review.

I don't see a strong argument to switch to a NULL terminated arrays (yet),
as it does not make things any better for performance/memory, so I'm not
sure if it's worth reshuffling the 10k+ LoC in drivers. Is there any other
argument that i miss?

Thanks,
Georgi