linux-kernel - Re: [PATCH v2 03/28] interconnect: qcom: rpmh: make nodes a NULL

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <efvyk4ojddr3emsdavex4uqrl476sj5hz3ihd6kditdxd373jm@wu2av4fvqc4h>
Date: Fri, 18 Jul 2025 20:26:57 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Georgi Djakov <djakov@...nel.org>
Cc: Bjorn Andersson <andersson@...nel.org>, linux-arm-msm@...r.kernel.org,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 03/28] interconnect: qcom: rpmh: make nodes a
 NULL_terminated array

On Fri, Jul 18, 2025 at 04:38:02PM +0300, Georgi Djakov wrote:
> 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.

Exactly, that's the point. It is easy to miss num_nodes, while omitting
NULL will crash the driver. So the error will be noted during
development.

> 
> [..]
> >   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?

Well, two arguments:

- Two first patches in the series (which fix num_links / num_nodes).

- I think we should drop the old (static IDs) code path. Switching to
  dynamic IDs would require reshuffling of the drivers (and these
  arrays) anyway.

> 
> Thanks,
> Georgi
> 

-- 
With best wishes
Dmitry