linux-kernel - Re: of_clk_add_(hw_)providers multipule times for one node?

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: <CAK7LNAQfwh3Z_=VWHvDi6EXFtVLuRMFVYzKMq8qCaZ30jfCq8g@mail.gmail.com>
Date:	Mon, 8 Aug 2016 01:54:23 +0900
From:	Masahiro Yamada <yamada.masahiro@...ionext.com>
To:	Stephen Boyd <sboyd@...eaurora.org>, Rob Herring <robh@...nel.org>
Cc:	linux-clk <linux-clk@...r.kernel.org>,
	Michael Turquette <mturquette@...libre.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: of_clk_add_(hw_)providers multipule times for one node?

Hi Stephen,

2016-08-05 6:25 GMT+09:00 Stephen Boyd <sboyd@...eaurora.org>:
> +Rob in case he has any insight
>
> On 07/09, Masahiro Yamada wrote:
>> Hi.
>>
>> I think the current code allows to add
>> clk_providers multiple times against one DT node.
>>
>> Are there cases that really need to do so?
>
> If we have clk drivers that have a device driver structure and
> also use CLK_OF_DECLARE then we could get into a situation where
> they register two providers for the same device node. I can't
> think of any other situation where this would happen though.

What is the benefit for splitting one clock device
into CLK_OF_DECLARE() and a platform_driver?

If we go this way, I think we need to fix the current code.

of_clk_add_provider() calls of_clk_del_provider()
in its failure path.

Notice of_clk_del_provider() unregister
all the providers associated with the device node.

So, if of_clk_add_provider() fails to register a platform driver,
it may unregister another provider added by OF_CLK_DECLARE().

Some platform drivers call of_clk_del_provider() in a .remove callback,
so the same problem could happen.

Why does of_clk_del_provider() take (struct device_node *np) ?
Shouldn't it take (struct of_clk_provider *cp)?

> It used to return the last provider's error, but I accidentally
> changed that behavior when adding clk_hw providers in commit
> 0861e5b8cf80 (clk: Add clk_hw OF clk providers, 2016-02-05).
> Nobody seems to have complained though, so you're the first to
> have reported this.

If we allow multiple OF-providers for one device node,
I think any error should be treated as EPROBE_DEFER,
i.e. the current code is good.

The scenario is:

 - Clocks with ID 0 thru 3 are provided by CLK_OF_DECLARE()
 - Clocks with ID 4 thru 9 are provided by a platform driver.

What if a clock consumer requests the clk ID 5
after CLK_OF_DECLARE(), but before the clk platform driver is registered?

If the clock consumer gets the last provider's error
(-EINVAL returned from CLK_OR_DECLARE one in this case)
it will lose a chance to retry it after clocks from a platform driver
are registered.

A bit nasty...

-- 
Best Regards
Masahiro Yamada