Re: [PATCH V2 2/2] cpufreq: dt: Refactor initialization to handle probe deferral properly

[Geert Uytterhoeven]

Hi Viresh, Stephan,

On Mon, Aug 24, 2020 at 11:12 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> From: Stephan Gerhold <stephan@xxxxxxxxxxx>
>
> cpufreq-dt is currently unable to handle -EPROBE_DEFER properly
> because the error code is not propagated for the cpufreq_driver->init()
> callback. Instead, it attempts to avoid the situation by temporarily
> requesting all resources within resources_available() and releasing them
> again immediately after. This has several disadvantages:
>
>   - Whenever we add something like interconnect handling to the OPP core
>     we need to patch cpufreq-dt to request these resources early.
>
>   - resources_available() is only run for CPU0, but other clusters may
>     eventually depend on other resources that are not available yet.
>     (See FIXME comment removed by this commit...)
>
>   - All resources need to be looked up several times.
>
> Now that the OPP core can propagate -EPROBE_DEFER during initialization,
> it would be nice to avoid all that trouble and just propagate its error
> code when necessary.
>
> This commit refactors the cpufreq-dt driver to initialize private_data
> before registering the cpufreq driver. We do this by iterating over
> all possible CPUs and ensure that all resources are initialized:
>
>   1. dev_pm_opp_get_opp_table() ensures the OPP table is allocated
>      and initialized with clock and interconnects.
>
>   2. dev_pm_opp_set_regulators() requests the regulators and assigns
>      them to the OPP table.
>
>   3. We call dev_pm_opp_of_get_sharing_cpus() early so that we only
>      initialize the OPP table once for each shared policy.
>
> With these changes, we actually end up saving a few lines of code,
> the resources are no longer looked up multiple times and everything
> should be much more robust.
>
> Signed-off-by: Stephan Gerhold <stephan@xxxxxxxxxxx>
> [ Viresh: Use list_head structure for maintaining the list and minor
>           changes ]
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>

Thanks for your patch, which is now commit dc279ac6e5b4e06e ("cpufreq:
dt: Refactor initialization to handle probe deferral properly") in
pm/linux-next, and to which I bisected a regression.
Reverting this commit fixes the issue.

On r8a7791/koelsch, during resume from s2ram:

     PM: suspend entry (deep)
     Filesystems sync: 0.000 seconds
     Freezing user space processes ... (elapsed 0.003 seconds) done.
     OOM killer disabled.
     Freezing remaining freezable tasks ... (elapsed 0.009 seconds) done.
     Disabling non-boot CPUs ...
     Enabling non-boot CPUs ...
    +i2c-sh_mobile e60b0000.i2c: Transfer request timed out
    +i2c-sh_mobile e60b0000.i2c: Transfer request timed out
    +i2c-sh_mobile e60b0000.i2c: Transfer request timed out
    +i2c-sh_mobile e60b0000.i2c: Transfer request timed out
    +i2c-sh_mobile e60b0000.i2c: Transfer request timed out
    +i2c-sh_mobile e60b0000.i2c: Transfer request timed out
    +i2c-sh_mobile e60b0000.i2c: Transfer request timed out
    +i2c-sh_mobile e60b0000.i2c: Transfer request timed out
    +i2c-sh_mobile e60b0000.i2c: Transfer request timed out
    +i2c-sh_mobile e60b0000.i2c: Transfer request timed out
    +i2c-sh_mobile e60b0000.i2c: Transfer request timed out
    +i2c-sh_mobile e60b0000.i2c: Transfer request timed out
    +cpu cpu0: OPP table can't be empty
     CPU1 is up
     rcar-pcie fe000000.pcie: PCIe x1: link up

The cpufreq code tries to talk to the PMIC, while the I2C controller
that hosts the PMIC is suspended, and thus any communication attempt
times out.  __i2c_check_suspended() fails to notice that, as the
i2c_shmobile_i2c driver doesn't have a suspend callback calling
i2c_mark_adapter_suspended() yet.  After fixing that (will send a patch
soon), the I2C core rightfully complains with:

    WARNING: CPU: 1 PID: 13 at drivers/i2c/i2c-core.h:54
__i2c_transfer+0x4a4/0x4e4
    i2c i2c-6: Transfer while suspended
    CPU: 1 PID: 13 Comm: cpuhp/1 Not tainted
5.9.0-shmobile-09581-g05a3e5886c7615b1-dirty #718
    Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
    [<c010dcec>] (unwind_backtrace) from [<c0109b18>] (show_stack+0x10/0x14)
    [<c0109b18>] (show_stack) from [<c075e928>] (dump_stack+0x8c/0xac)
    [<c075e928>] (dump_stack) from [<c011c23c>] (__warn+0xd0/0xe8)
    [<c011c23c>] (__warn) from [<c011c2c4>] (warn_slowpath_fmt+0x70/0x9c)
    [<c011c2c4>] (warn_slowpath_fmt) from [<c0548be8>]
(__i2c_transfer+0x4a4/0x4e4)
    [<c0548be8>] (__i2c_transfer) from [<c0548cd8>] (i2c_transfer+0xb0/0xf8)
    [<c0548cd8>] (i2c_transfer) from [<c046f884>] (regmap_i2c_read+0x54/0x88)
    [<c046f884>] (regmap_i2c_read) from [<c046b69c>]
(_regmap_raw_read+0x118/0x1f0)
    [<c046b69c>] (_regmap_raw_read) from [<c046b7b8>]
(_regmap_bus_read+0x44/0x68)
    [<c046b7b8>] (_regmap_bus_read) from [<c04698a0>] (_regmap_read+0x84/0x110)
    [<c04698a0>] (_regmap_read) from [<c046c10c>] (regmap_read+0x40/0x58)
    [<c046c10c>] (regmap_read) from [<c03cf0a8>]
(regulator_get_voltage_sel_regmap+0x28/0x74)
    [<c03cf0a8>] (regulator_get_voltage_sel_regmap) from [<c03cb29c>]
(regulator_get_voltage_rdev+0xa4/0x14c)
    [<c03cb29c>] (regulator_get_voltage_rdev) from [<c03cc658>]
(regulator_get_voltage+0x2c/0x60)
    [<c03cc658>] (regulator_get_voltage) from [<c03cd994>]
(regulator_is_supported_voltage+0x30/0xd8)
    [<c03cd994>] (regulator_is_supported_voltage) from [<c05a32b4>]
(_opp_add+0x164/0x1b8)
    [<c05a32b4>] (_opp_add) from [<c05a3388>] (_opp_add_v1+0x80/0xb8)
    [<c05a3388>] (_opp_add_v1) from [<c05a5220>]
(dev_pm_opp_of_add_table+0x130/0x168)
    [<c05a5220>] (dev_pm_opp_of_add_table) from [<c05a5330>]
(dev_pm_opp_of_cpumask_add_table+0x60/0xac)
    [<c05a5330>] (dev_pm_opp_of_cpumask_add_table) from [<c05ab88c>]
(cpufreq_init+0x94/0x1c4)
    [<c05ab88c>] (cpufreq_init) from [<c05a8190>] (cpufreq_online+0x148/0x7ac)
    [<c05a8190>] (cpufreq_online) from [<c05a87fc>]
(cpuhp_cpufreq_online+0x8/0x10)
    [<c05a87fc>] (cpuhp_cpufreq_online) from [<c011d088>]
(cpuhp_invoke_callback+0xf8/0x2e4)
    [<c011d088>] (cpuhp_invoke_callback) from [<c011d418>]
(cpuhp_thread_fun+0xac/0x244)
    [<c011d418>] (cpuhp_thread_fun) from [<c013c090>]
(smpboot_thread_fn+0x19c/0x1a8)
    [<c013c090>] (smpboot_thread_fn) from [<c0138408>] (kthread+0x104/0x110)
    [<c0138408>] (kthread) from [<c0100148>] (ret_from_fork+0x14/0x2c)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds