linux-kernel - Re: [PATCH v2] spmi: Fix controller->node != parent->node breakage

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: <5b139af8d639e20a14c8bd00e8e03a4a.sboyd@kernel.org>
Date: Mon, 13 Jan 2025 13:52:48 -0800
From: Stephen Boyd <sboyd@...nel.org>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Joe Hattori <joe@...is.s.u-tokyo.ac.jp>, Konrad Dybcio <konradybcio@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>, Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc: Marijn Suijten <marijn.suijten@...ainline.org>, linux-kernel@...r.kernel.org, Bjorn Andersson <bjorn.andersson@....qualcomm.com>, Abel Vesa <abel.vesa@...aro.org>, Johan Hovold <johan+linaro@...nel.org>, linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org, Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Subject: Re: [PATCH v2] spmi: Fix controller->node != parent->node breakage

Quoting Konrad Dybcio (2025-01-13 05:02:58)
> From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
> 
> On some platforms, like recent Qualcomm SoCs with multi-bus SPMI
> arbiters, controller->node must be assigned to the individual buses'
> subnodes, as the slave devices are children of these, like so:
> 
> arbiter@...0000
>         spmi@...d000
>                 pmic@0
> 
>         spmi@...2000
>                 pmic@0
> 
> The commit referenced in Fixes changed that assignment, such that
> spmi_controller_alloc() always assumes the PMICs come directly under
> the arbiter node (which is true when there's only a single bus per
> controller).
> 
> Make controller->node specifiable to both benefit from Joe's refcount
> improvements and un-break the aforementioned platforms.

How is it broken? I see spmi_pmic_arb_bus_init() calls
devm_spmi_controller_alloc() which sets the of_node to the parent device
and then spmi_pmic_arb_bus_init() overwrites that with
'ctrl->dev.of_node = node' later on in the same function. That will
cause one more of_node_put() than is expected. I don't see that removed
in this patch though, so the leak is still there?

> 
> Fixes: 821b07853e32 ("spmi: hisi-spmi-controller: manage the OF node reference in device initialization and cleanup")

I've dropped this patch from my queue. I don't know if we're really
doing anything better by managing the of_node lifetime in that function
vs. letting the callers assign the node they want and manage the
lifetime themselves. Maybe we don't need to do anything? Presumably the
parent device driver will unregister the controller anyway, so the
lifetime of the of_node will be ensured regardless. For subnodes like
qcom SPMI, the subnodes are child nodes of the parent device so they
won't be removed. If they are dynamic nodes, then the caller can manage
the lifetime.