Re: [PATCH] memory: samsung: exynos5422-dmc: propagate error from exynos5_counters_get()

From: Lukasz Luba
Date: Tue Aug 04 2020 - 08:38:19 EST




On 8/4/20 1:19 PM, Marek Szyprowski wrote:
Hi Lukasz,

On 04.08.2020 11:11, Lukasz Luba wrote:
Hi Marek,

On 8/4/20 7:12 AM, Marek Szyprowski wrote:
exynos5_counters_get() might fail with -EPROBE_DEFER if the driver for
devfreq event counter is not yet probed. Propagate that error value to
the caller to ensure that the exynos5422-dmc driver will be probed again
when devfreq event contuner is available.

This fixes boot hang if both exynos5422-dmc and exynos-ppmu drivers are
compiled as modules.

Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
---
  drivers/memory/samsung/exynos5422-dmc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/memory/samsung/exynos5422-dmc.c
b/drivers/memory/samsung/exynos5422-dmc.c
index b9c7956e5031..639811a3eecb 100644
--- a/drivers/memory/samsung/exynos5422-dmc.c
+++ b/drivers/memory/samsung/exynos5422-dmc.c
@@ -914,7 +914,7 @@ static int exynos5_dmc_get_status(struct device
*dev,
      } else {
          ret = exynos5_counters_get(dmc, &load, &total);
          if (ret < 0)
-            return -EINVAL;
+            return ret;
            /* To protect from overflow, divide by 1024 */
          stat->busy_time = load >> 10;


Thank you for the patch, LGTM.
Some questions are still there, though. The function
exynos5_performance_counters_init() should capture that the counters
couldn't be enabled or set. So the functions:
exynos5_counters_enable_edev() and exynos5_counters_set_event()
must pass gently because devfreq device is registered.
Then devfreq checks device status, and reaches the state when
counters 'get' function returns that they are not ready...

If that is a kind of 2-stage initialization, maybe we should add
another 'check' in the exynos5_performance_counters_init() and call
the devfreq_event_get_event() to make sure that we are ready to go,
otherwise return ret from that function (which is probably EPROBE_DEFER)
and not register the devfreq device.

I've finally investigated this further and it turned out that the issue
is elsewhere. The $subject patch can be discarded, as it doesn't fix
anything. The -EPROBE_DEFER is properly returned by
exynos5_performance_counters_init(), which redirects exynos5_dmc_probe()
to remove_clocks label. This causes disabling mout_bpll/fout_bpll clocks
what in turn *sometimes* causes boot hang. This random behavior mislead
me that the $subject patch fixes the issue, but then longer tests
revealed that it didn't change anything.

Really good investigation, great work Marek!


It looks that the proper fix would be to keep fout_bpll enabled all the
time.

Yes, I agree. I am looking for your next patch to test it then.



Marek do want to submit such patch or I should bake it and submit on top
of this patch?

Could you tell me how I can reproduce this? Do you simply load one
module after another (exynos-ppmu than exynos5422-dmc) or in parallel?

I've just boot zImage built from multi_v7_defconfig with modules
installed. Modules are automatically loaded by udev during boot.

Thank you sharing this test procedure.

Regards,
Lukasz