linux-kernel - Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 31 Jul 2019 15:01:44 +0200
From:   Artur Świgoń <a.swigon@...tner.samsung.com>
To:     Krzysztof Kozlowski <krzk@...nel.org>
Cc:     devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        cw00.choi@...sung.com, myungjoo.ham@...sung.com,
        inki.dae@...sung.com, sw0312.kim@...sung.com,
        georgi.djakov@...aro.org, m.szyprowski@...sung.com,
        Bartłomiej Żołnierkiewicz 
        <b.zolnierkie@...sung.com>
Subject: Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect
 functionality to exynos-bus

On Wed, 2019-07-24 at 20:36 +0200, Krzysztof Kozlowski wrote:
> On Tue, Jul 23, 2019 at 02:20:14PM +0200, Artur Świgoń wrote:
> > This patch adds interconnect functionality to the exynos-bus devfreq
> > driver.
> > 
> > The SoC topology is a graph (or, more specifically, a tree) and most of its
> > edges are taken from the devfreq parent-child hierarchy (cf.
> > Documentation/devicetree/bindings/devfreq/exynos-bus.txt). The previous
> > patch adds missing edges to the DT (under the name 'parent'). Due to
> 
> Do not refer to DT patches. They will come through different tree so
> "previous" will not be correct anymore. You mentioned dependencies in
> cover letter so it is sufficient.

OK.
 
> >  /*
> > @@ -61,6 +69,13 @@ exynos_bus_ops_edev(enable_edev);
> >  exynos_bus_ops_edev(disable_edev);
> >  exynos_bus_ops_edev(set_event);
> >  
> > +static int exynos_bus_next_id(void)
> > +{
> > +	static int exynos_bus_node_id;
> > +
> > +	return exynos_bus_node_id++;
> 
> This does not look robust. Use IDR for IDs. 

OK.

> > +static int exynos_bus_icc_connect(struct exynos_bus *bus)
> > +{
> > +	struct device_node *np = bus->dev->of_node;
> > +	struct devfreq *parent_devfreq;
> > +	struct icc_node *parent_node = NULL;
> > +	struct of_phandle_args args;
> > +	int ret = 0;
> > +
> > +	parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
> > +	if (!IS_ERR(parent_devfreq)) {
> > +		struct exynos_bus *parent_bus;
> 
> What if someone unbinds this parent devfreq? I guess everything in
> devfreq starts exploding... however it's not the problem of this patch.
> 
> Do you also need suspend/resume order (device links)? I guess the other
> side, e.g.  mixer, should resume before the bus?

Actually, I think that the bus (devfreq) should resume before mixer. However,
suspend/resume order is another issue that applies to this driver regardless of
using the interconnect framework, although device links could probably also be
implemented in the interconnect framework itself.

> > +		parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
> > +		parent_node = parent_bus->node;
> > +	} else {
> > +		/* Look for parent in DT */
> > +		int num = of_count_phandle_with_args(np, "parent",
> > +						     "#interconnect-cells");
> > +		if (num != 1)
> 
> You will return here 0 but isn't it an error?

It is definitely not an error when 'parent' does not exist in DT (for buses that
are parents themselves). I can extend the comment in the code to explicitly
state that.

Best regards,
-- 
Artur Świgoń
Samsung R&D Institute Poland
Samsung Electronics