Patchwork [v1] clk: Probe defer clk_get() on orphans

login
register
mail settings
Submitter Jeffrey Hugo
Date Feb. 11, 2019, 6:57 p.m.
Message ID <1549911467-2465-1-git-send-email-jhugo@codeaurora.org>
Download mbox | patch
Permalink /patch/723253/
State New
Headers show

Comments

Jeffrey Hugo - Feb. 11, 2019, 6:57 p.m.
If a parent to a clock comes from outside that clock's provider, the parent
may not be present at the time the clock is registered (ie the parent comes
from another driver that has not yet probed).  The clock can still be
registered, and a reference to it obtained, however that clock may not be
fully functional - ie get_rate might return an invalid value.

This has been a problem that has resulted in the UART console breaking on
some Qualcomm SoCs, as the UART baud rate is based on a clock that is the
child of XO.  Due to the large chain of dependencies, its possible that the
RPM has not provided XO by the time that the UART driver probes, gets the
baud rate clock, and calls get_rate - which returns 0 and results in a bad
configuration.

An orphan clock is a clock that is missing a parent or some other ancestor.
Since the parent is defined, we can assume that it is expected to appear at
some point in a properly configured system (all bets are off if a required
driver is not compiled, etc), and it is unlikely that the clock can be
properly consumed during the time the clock is an orphan.  Therefore,
return EPROBE_DEFER for orphan clocks so that consumers wait until the
parent chain is established, and proper clock operation can occur.

Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
---

This is based upon the "Rewrite clk parent handling" series at [1], and assumes
that the suspected missing line commented on at [2] is added.

The idea for this solution came from [3] and [4].

[1] https://lore.kernel.org/lkml/20190129061021.94775-1-sboyd@kernel.org/T/#u
[2] https://lkml.org/lkml/2019/2/11/1634
[3] https://lkml.org/lkml/2019/2/6/382
[4] https://lkml.org/lkml/2015/12/27/209

 drivers/clk/clk.c | 4 ++++
 1 file changed, 4 insertions(+)
Jeffrey Hugo - March 6, 2019, 9:48 p.m.
Ping?

Stephen, I know as this depends on your clock parent handling series 
(happens to apply just fine to v2), its not going to be accepted until 
that gets sorted out, but do you have any thoughts on if this seems like 
an appropriate thing to do, or if you'd like to see a different solution?

On 2/11/2019 11:57 AM, Jeffrey Hugo wrote:
> If a parent to a clock comes from outside that clock's provider, the parent
> may not be present at the time the clock is registered (ie the parent comes
> from another driver that has not yet probed).  The clock can still be
> registered, and a reference to it obtained, however that clock may not be
> fully functional - ie get_rate might return an invalid value.
> 
> This has been a problem that has resulted in the UART console breaking on
> some Qualcomm SoCs, as the UART baud rate is based on a clock that is the
> child of XO.  Due to the large chain of dependencies, its possible that the
> RPM has not provided XO by the time that the UART driver probes, gets the
> baud rate clock, and calls get_rate - which returns 0 and results in a bad
> configuration.
> 
> An orphan clock is a clock that is missing a parent or some other ancestor.
> Since the parent is defined, we can assume that it is expected to appear at
> some point in a properly configured system (all bets are off if a required
> driver is not compiled, etc), and it is unlikely that the clock can be
> properly consumed during the time the clock is an orphan.  Therefore,
> return EPROBE_DEFER for orphan clocks so that consumers wait until the
> parent chain is established, and proper clock operation can occur.
> 
> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
> ---
> 
> This is based upon the "Rewrite clk parent handling" series at [1], and assumes
> that the suspected missing line commented on at [2] is added.
> 
> The idea for this solution came from [3] and [4].
> 
> [1] https://lore.kernel.org/lkml/20190129061021.94775-1-sboyd@kernel.org/T/#u
> [2] https://lkml.org/lkml/2019/2/11/1634
> [3] https://lkml.org/lkml/2019/2/6/382
> [4] https://lkml.org/lkml/2015/12/27/209
> 
>   drivers/clk/clk.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 34e9524ea25d..cf71e0614282 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3405,6 +3405,10 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
>   		return ERR_CAST(hw);
>   
>   	core = hw->core;
> +
> +	if (core->orphan)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
>   	clk = alloc_clk(core, dev_id, con_id);
>   	if (IS_ERR(clk))
>   		return clk;
>
Stephen Boyd - March 6, 2019, 11:19 p.m.
Quoting Jeffrey Hugo (2019-03-06 13:48:13)
> Ping?
> 
> Stephen, I know as this depends on your clock parent handling series 
> (happens to apply just fine to v2), its not going to be accepted until 
> that gets sorted out, but do you have any thoughts on if this seems like 
> an appropriate thing to do, or if you'd like to see a different solution?

Please don't ping during the merge window. I'll probably look at this
patch next week.
Jeffrey Hugo - March 21, 2019, 2:50 p.m.
On 3/6/2019 4:19 PM, Stephen Boyd wrote:
> Quoting Jeffrey Hugo (2019-03-06 13:48:13)
>> Ping?
>>
>> Stephen, I know as this depends on your clock parent handling series
>> (happens to apply just fine to v2), its not going to be accepted until
>> that gets sorted out, but do you have any thoughts on if this seems like
>> an appropriate thing to do, or if you'd like to see a different solution?
> 
> Please don't ping during the merge window. I'll probably look at this
> patch next week.
> 

Sorry about pinging during the merge window.  At the time, I just 
realized the patch has been on list for several weeks with no response, 
but hadn't connected the dots that it happened to be the merge window at 
that particular moment.

Any update on when you think you might get around to having a look at this?
Jeffrey Hugo - April 5, 2019, 2:34 p.m.
On 3/21/2019 8:50 AM, Jeffrey Hugo wrote:
> On 3/6/2019 4:19 PM, Stephen Boyd wrote:
>> Quoting Jeffrey Hugo (2019-03-06 13:48:13)
>>> Ping?
>>>
>>> Stephen, I know as this depends on your clock parent handling series
>>> (happens to apply just fine to v2), its not going to be accepted until
>>> that gets sorted out, but do you have any thoughts on if this seems like
>>> an appropriate thing to do, or if you'd like to see a different 
>>> solution?
>>
>> Please don't ping during the merge window. I'll probably look at this
>> patch next week.
>>
> 
> Sorry about pinging during the merge window.  At the time, I just 
> realized the patch has been on list for several weeks with no response, 
> but hadn't connected the dots that it happened to be the merge window at 
> that particular moment.
> 
> Any update on when you think you might get around to having a look at this?
> 

Ping? (again)

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 34e9524ea25d..cf71e0614282 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3405,6 +3405,10 @@  struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
 		return ERR_CAST(hw);
 
 	core = hw->core;
+
+	if (core->orphan)
+		return ERR_PTR(-EPROBE_DEFER);
+
 	clk = alloc_clk(core, dev_id, con_id);
 	if (IS_ERR(clk))
 		return clk;