linux-kernel - Re: [PATCH 1/1] PM / Domains: Avoid a potential deadlock

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: <CAPDyKFodmgkpLkJDiMqh3vm+Ynuo852biZkY7uB5sm9UeAJ63A@mail.gmail.com>
Date:   Wed, 13 Mar 2019 08:35:02 +0100
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Jiada Wang <jiada_wang@...tor.com>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Kevin Hilman <khilman@...nel.org>,
        Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Morimoto <kuninori.morimoto.gx@...esas.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>
Subject: Re: [PATCH 1/1] PM / Domains: Avoid a potential deadlock

On Tue, 12 Mar 2019 at 07:51, Jiada Wang <jiada_wang@...tor.com> wrote:
>
> Lockdep warns that prepare_lock and genpd->mlock can cause a deadlock
> the deadlock scenario is like following:
> First thread is probing cs2000
> cs2000_probe()
>   clk_register()
>     __clk_core_init()
>       clk_prepare_lock()                            ----> acquires prepare_lock
>         cs2000_recalc_rate()
>           i2c_smbus_read_byte_data()
>             rcar_i2c_master_xfer()
>               dma_request_chan()
>                 rcar_dmac_of_xlate()
>                   rcar_dmac_alloc_chan_resources()
>                     pm_runtime_get_sync()
>                       __pm_runtime_resume()
>                         rpm_resume()
>                           rpm_callback()
>                             genpd_runtime_resume()   ----> acquires genpd->mlock
>
> Second thread is attaching any device to the same PM domain
> genpd_add_device()
>   genpd_lock()                                       ----> acquires genpd->mlock
>     cpg_mssr_attach_dev()
>       of_clk_get_from_provider()
>         __of_clk_get_from_provider()
>           __clk_create_clk()
>             clk_prepare_lock()                       ----> acquires prepare_lock
>
> Since currently no PM provider access genpd's critical section
> in .attach_dev, and .detach_dev callbacks, so there is no need to protect
> these two callbacks with genpd->mlock.
> This patch avoids a potential deadlock by moving out .attach_dev and .detach_dev
> from genpd->mlock, so that genpd->mlock won't be held when prepare_lock is acquired
> in .attach_dev and .detach_dev

Thanks for the detailed description, this seems like a reasonable change to me!

>
> Signed-off-by: Jiada Wang <jiada_wang@...tor.com>

Reviewed-by: Ulf Hansson <ulf.hansson@...aro.org>

Kind regards
Uffe


> ---
>  drivers/base/power/domain.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 500de1dee967..a00ca6b8117b 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1467,12 +1467,12 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>         if (IS_ERR(gpd_data))
>                 return PTR_ERR(gpd_data);
>
> -       genpd_lock(genpd);
> -
>         ret = genpd->attach_dev ? genpd->attach_dev(genpd, dev) : 0;
>         if (ret)
>                 goto out;
>
> +       genpd_lock(genpd);
> +
>         dev_pm_domain_set(dev, &genpd->domain);
>
>         genpd->device_count++;
> @@ -1480,9 +1480,8 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>
>         list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
>
> - out:
>         genpd_unlock(genpd);
> -
> + out:
>         if (ret)
>                 genpd_free_dev_data(dev, gpd_data);
>         else
> @@ -1531,15 +1530,15 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
>         genpd->device_count--;
>         genpd->max_off_time_changed = true;
>
> -       if (genpd->detach_dev)
> -               genpd->detach_dev(genpd, dev);
> -
>         dev_pm_domain_set(dev, NULL);
>
>         list_del_init(&pdd->list_node);
>
>         genpd_unlock(genpd);
>
> +       if (genpd->detach_dev)
> +               genpd->detach_dev(genpd, dev);
> +
>         genpd_free_dev_data(dev, gpd_data);
>
>         return 0;
> --
> 2.19.2
>