linux-kernel - Re: [PATCH V6 1/4] interconnect: qcom: Add multidev EPSS L3 support

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] [day] [month] [year] [list]

Message-ID: <b2zicviv7nyl3izj2fzwzm2cp5phlxufaaoyi7e3g3iyxcyw56@iufgz33tsk33>
Date: Sat, 30 Nov 2024 17:32:44 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Cc: Raviteja Laggyshetty <quic_rlaggysh@...cinc.com>, 
	Georgi Djakov <djakov@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, 
	Odelu Kukatla <quic_okukatla@...cinc.com>, Mike Tipton <quic_mdtipton@...cinc.com>, 
	Sibi Sankar <quic_sibis@...cinc.com>, linux-arm-msm@...r.kernel.org, linux-pm@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V6 1/4] interconnect: qcom: Add multidev EPSS L3 support

On Sat, Nov 30, 2024 at 04:12:49PM +0100, Konrad Dybcio wrote:
> On 30.11.2024 4:09 PM, Dmitry Baryshkov wrote:
> > On Sat, Nov 30, 2024 at 01:49:56PM +0100, Konrad Dybcio wrote:
> >> On 25.11.2024 6:45 PM, Raviteja Laggyshetty wrote:
> >>> EPSS on SA8775P has two instances which requires creation of two device
> >>> nodes with different compatible and device data because of unique
> >>> icc node id and name limitation in interconnect framework.
> >>> Add multidevice support to osm-l3 code to get unique node id from IDA
> >>> and node name is made unique by appending node address.
> >>>
> >>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@...cinc.com>
> >>> ---
> >>
> >> [...]
> >>
> >>> +	ret = of_property_read_reg(pdev->dev.of_node, 0, &addr, NULL);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>>  	qp->base = devm_platform_ioremap_resource(pdev, 0);
> >>>  	if (IS_ERR(qp->base))
> >>>  		return PTR_ERR(qp->base);
> >>> @@ -242,8 +262,13 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
> >>>  
> >>>  	icc_provider_init(provider);
> >>>  
> >>> +	/* Allocate unique id for qnodes */
> >>> +	for (i = 0; i < num_nodes; i++)
> >>> +		qnodes[i]->id = ida_alloc_min(&osm_l3_id, OSM_L3_NODE_ID_START, GFP_KERNEL);
> >>
> >> As I've said in my previous emails, this is a framework-level problem.
> >>
> >> Up until now we've simply silently ignored the possibility of an
> >> interconnect provider having more than one instance, as conveniently
> >> most previous SoCs had a bunch of distinct bus masters.
> >>
> >> Currently, debugfs-client.c relies on the node names being unique.
> >> Keeping them as such is also useful for having a sane sysfs/debugfs
> >> interface. But it's not always feasible, and a hierarchical approach
> >> (like in pmdomain) may be a better fit.
> >>
> >> Then, node->id is used for creating links, and we unfortunately cannot
> >> assume that both src and dst are within the same provider.
> >> I'm not a fan of these IDs being hardcoded, but there are some drivers
> >> that rely on that, which itself is also a bit unfortunate..
> >>
> >>
> >> If Mike (who introduced debugfs-client and is probably the main user)
> >> doesn't object to a small ABI break (which is "fine" with a debugfs
> >> driver that requires editing the source code to be compiled), we could
> >> add a property within icc_provider like `bool dynamic_ids` and have an
> >> ICC-global IDA that would take care of any conflicts.
> > 
> > Frankly speaking, I think this just delays the inevitable. We have been
> > there with GPIOs and with some other suppliers. In my opinion the ICC
> > subsystem needs to be refactored in order to support linking based on
> > the supplier (fwnode?) + offset_id, but that's a huuuge rework.
> 
> I thought about this too, but ended up not including it in the email..
> 
> I think this will be more difficult with ICC, as tons of circular
> dependencies are inevitable by design and we'd essentially have to
> either provide placeholder nodes (like it's the case today) or probe
> only parts of a device, recursively, to make sure all links can be
> created

Or just allow probing, but then fail path creation. It will be a
redesign, but I think it is inevitable in the end.

> 
> Konrad
> 
> >> Provider drivers whose consumers don't already rely on programmatical
> >> use of hardcoded IDs *and* don't have cross-provider links could then
> >> enable that flag and have the node IDs and names set like you did in
> >> this patch. This also sounds very useful for icc-clk.
> > 

-- 
With best wishes
Dmitry