Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

clk: qcom: lpass-sc7280: Fix pm_runtime usage

The pm_runtime usage in lpass-sc7280 was broken in quite a few
ways. Specifically:

1. At the end of probe it called "put" twice. This is a no-no and will
end us up with a negative usage count. Even worse than calling
"put" twice, it never called "get" once. Thus after bootup it could
be seen that the runtime usage of the devices managed by this
driver was -2.
2. In some error cases it manually called pm_runtime_disable() even
though it had previously used devm_add_action_or_reset() to set
this up to be called automatically. This meant that in these error
cases we'd double-call pm_runtime_disable().
3. It forgot to call undo pm_runtime_use_autosuspend(), which can
sometimes have subtle problems (and the docs specifically mention
that you need to undo this function).

Overall the above seriously calls into question how this driver is
working. It seems like a combination of "it doesn't", "by luck", and
"because of the weirdness of runtime_pm". Specifically I put a
printout to the serial console every time the runtime suspend/resume
was called for the two devices created by this driver (I wrapped the
pm_clk calls). When I had serial console enabled, I found that the
calls got resumed at bootup (when the clk core probed and before our
double-put) and then never touched again. That's no good.
[ 0.829997] DOUG: my_pm_clk_resume, usage=1
[ 0.835487] DOUG: my_pm_clk_resume, usage=1

When I disabled serial console (speeding up boot), I got a different
pattern, which I guess (?) is better:
[ 0.089767] DOUG: my_pm_clk_resume, usage=1
[ 0.090507] DOUG: my_pm_clk_resume, usage=1
[ 0.151885] DOUG: my_pm_clk_suspend, usage=-2
[ 0.151914] DOUG: my_pm_clk_suspend, usage=-2
[ 1.825747] DOUG: my_pm_clk_resume, usage=-1
[ 1.825774] DOUG: my_pm_clk_resume, usage=-1
[ 1.888269] DOUG: my_pm_clk_suspend, usage=-2
[ 1.888282] DOUG: my_pm_clk_suspend, usage=-2

These different patterns have to do with the fact that the core PM
Runtime code really isn't designed to be robust to negative usage
counts and sometimes may happen to stumble upon a behavior that
happens to "work". For instance, you can see that
__pm_runtime_suspend() will treat any non-zero value (including
negative numbers) as if the device is in use.

In any case, let's fix the driver to be correct. We'll hold a
pm_runtime reference for the whole probe and then drop it (once!) at
the end. We'll get rid of manual pm_runtime_disable() calls in the
error handling. We'll also switch to devm_pm_runtime_enable(), which
magically handles undoing pm_runtime_use_autosuspend() as of commit
b4060db9251f ("PM: runtime: Have devm_pm_runtime_enable() handle
pm_runtime_dont_use_autosuspend()").

While we're at this, let's also use devm_pm_clk_create() instead of
rolling it ourselves.

Note that the above changes make it obvious that
lpassaudio_create_pm_clks() was doing more than just creating
clocks. It was also setting up pm_runtime parameters. Let's rename it.

All of these problems were found by code inspection. I started looking
at this driver because it was involved in a deadlock that I reported a
while ago [1]. Though I bisected the deadlock to commit 1b771839de05
("clk: qcom: gdsc: enable optional power domain support"), it was
never really clear why that patch affected it other than a luck of
timing changes. I'll also note that by fixing the timing (as done in
this change) we also seem to aboid the deadlock, which is a nice
benefit.

Also note that some of the fixes here are much the same type of stuff
that Dmitry did in commit 72cfc73f4663 ("clk: qcom: use
devm_pm_runtime_enable and devm_pm_clk_create"), but I guess
lpassaudiocc-sc7280.c didn't exist then.

[1] https://lore.kernel.org/r/20220922154354.2486595-1-dianders@chromium.org

Fixes: a9dd26639d05 ("clk: qcom: lpass: Add support for LPASS clock controller for SC7280")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Bjorn Andersson <andersson@kernel.org>
Link: https://lore.kernel.org/r/20221104064055.1.I00a0e4564a25489e85328ec41636497775627564@changeid

authored by

Douglas Anderson and committed by
Bjorn Andersson
d470be3c aec5f36c

+21 -34
+21 -34
drivers/clk/qcom/lpassaudiocc-sc7280.c
··· 722 722 }; 723 723 MODULE_DEVICE_TABLE(of, lpass_audio_cc_sc7280_match_table); 724 724 725 - static void lpassaudio_pm_runtime_disable(void *data) 726 - { 727 - pm_runtime_disable(data); 728 - } 729 - 730 - static void lpassaudio_pm_clk_destroy(void *data) 731 - { 732 - pm_clk_destroy(data); 733 - } 734 - 735 - static int lpassaudio_create_pm_clks(struct platform_device *pdev) 725 + static int lpass_audio_setup_runtime_pm(struct platform_device *pdev) 736 726 { 737 727 int ret; 738 728 739 729 pm_runtime_use_autosuspend(&pdev->dev); 740 730 pm_runtime_set_autosuspend_delay(&pdev->dev, 50); 741 - pm_runtime_enable(&pdev->dev); 742 - 743 - ret = devm_add_action_or_reset(&pdev->dev, lpassaudio_pm_runtime_disable, &pdev->dev); 731 + ret = devm_pm_runtime_enable(&pdev->dev); 744 732 if (ret) 745 733 return ret; 746 734 747 - ret = pm_clk_create(&pdev->dev); 748 - if (ret) 749 - return ret; 750 - 751 - ret = devm_add_action_or_reset(&pdev->dev, lpassaudio_pm_clk_destroy, &pdev->dev); 735 + ret = devm_pm_clk_create(&pdev->dev); 752 736 if (ret) 753 737 return ret; 754 738 ··· 740 756 if (ret < 0) 741 757 dev_err(&pdev->dev, "failed to acquire iface clock\n"); 742 758 743 - return ret; 759 + return pm_runtime_resume_and_get(&pdev->dev); 744 760 } 745 761 746 762 static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev) ··· 749 765 struct regmap *regmap; 750 766 int ret; 751 767 752 - ret = lpassaudio_create_pm_clks(pdev); 768 + ret = lpass_audio_setup_runtime_pm(pdev); 753 769 if (ret) 754 770 return ret; 755 771 ··· 759 775 760 776 regmap = qcom_cc_map(pdev, desc); 761 777 if (IS_ERR(regmap)) { 762 - pm_runtime_disable(&pdev->dev); 763 - return PTR_ERR(regmap); 778 + ret = PTR_ERR(regmap); 779 + goto exit; 764 780 } 765 781 766 782 clk_zonda_pll_configure(&lpass_audio_cc_pll, regmap, &lpass_audio_cc_pll_config); ··· 772 788 ret = qcom_cc_really_probe(pdev, &lpass_audio_cc_sc7280_desc, regmap); 773 789 if (ret) { 774 790 dev_err(&pdev->dev, "Failed to register LPASS AUDIO CC clocks\n"); 775 - pm_runtime_disable(&pdev->dev); 776 - return ret; 791 + goto exit; 777 792 } 778 793 779 794 ret = qcom_cc_probe_by_index(pdev, 1, &lpass_audio_cc_reset_sc7280_desc); 780 795 if (ret) { 781 796 dev_err(&pdev->dev, "Failed to register LPASS AUDIO CC Resets\n"); 782 - pm_runtime_disable(&pdev->dev); 783 - return ret; 797 + goto exit; 784 798 } 785 799 786 800 pm_runtime_mark_last_busy(&pdev->dev); 801 + exit: 787 802 pm_runtime_put_autosuspend(&pdev->dev); 788 - pm_runtime_put_sync(&pdev->dev); 789 803 790 804 return ret; 791 805 } ··· 821 839 struct regmap *regmap; 822 840 int ret; 823 841 824 - ret = lpassaudio_create_pm_clks(pdev); 842 + ret = lpass_audio_setup_runtime_pm(pdev); 825 843 if (ret) 826 844 return ret; 827 845 828 846 if (of_property_read_bool(pdev->dev.of_node, "qcom,adsp-pil-mode")) { 829 847 lpass_audio_cc_sc7280_regmap_config.name = "cc"; 830 848 desc = &lpass_cc_sc7280_desc; 831 - return qcom_cc_probe(pdev, desc); 849 + ret = qcom_cc_probe(pdev, desc); 850 + goto exit; 832 851 } 833 852 834 853 lpass_audio_cc_sc7280_regmap_config.name = "lpasscc_aon"; ··· 837 854 desc = &lpass_aon_cc_sc7280_desc; 838 855 839 856 regmap = qcom_cc_map(pdev, desc); 840 - if (IS_ERR(regmap)) 841 - return PTR_ERR(regmap); 857 + if (IS_ERR(regmap)) { 858 + ret = PTR_ERR(regmap); 859 + goto exit; 860 + } 842 861 843 862 clk_lucid_pll_configure(&lpass_aon_cc_pll, regmap, &lpass_aon_cc_pll_config); 844 863 845 864 ret = qcom_cc_really_probe(pdev, &lpass_aon_cc_sc7280_desc, regmap); 846 - if (ret) 865 + if (ret) { 847 866 dev_err(&pdev->dev, "Failed to register LPASS AON CC clocks\n"); 867 + goto exit; 868 + } 848 869 849 870 pm_runtime_mark_last_busy(&pdev->dev); 871 + exit: 850 872 pm_runtime_put_autosuspend(&pdev->dev); 851 - pm_runtime_put_sync(&pdev->dev); 852 873 853 874 return ret; 854 875 }