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] [thread-next>] [day] [month] [year] [list]
Message-ID: <bbn3cbrxcagifpcjrzh5k7o5xvf6ajnf5y6zqnghex6sqwdt4t@mb3v6yfgehtv>
Date: Sun, 29 Dec 2024 03:23:30 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Raviteja Laggyshetty <quic_rlaggysh@...cinc.com>
Cc: Konrad Dybcio <konrad.dybcio@....qualcomm.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 Thu, Dec 26, 2024 at 09:43:20PM +0530, Raviteja Laggyshetty wrote:
> 
> 
> On 11/30/2024 9:02 PM, Dmitry Baryshkov wrote:
> > 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.
> > 
> 
> There are no two instances of l3 or NoC on any SoC except qcs9100 and
> qcs8300. I dont expect any new SoC as well.
> As second instance is needed only on qcs9100 and qcs8300, I am keeping
> the patch (patchset v6) as is and limit the dynamic id addition to l3
> provider only.

As you could have noticed, it was suggested to change ICC subsystem API
to allow the dynamic IDs. This isssue is not limited to just EPSS L3
driver. So we were discussing if you or your colleagues could sign up
for updating the interconnect subsystem to use node+arguments approach
instead of using a global static ID list.

> 
> >>
> >> 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