linux-kernel - Re: [PATCH] clk: bcm: rpi: Don't register as OF provider if !dev->np

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: <161678217156.3012082.17482478610662557764@swboyd.mtv.corp.google.com>
Date:   Fri, 26 Mar 2021 11:09:31 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Maxime Ripard <maxime@...no.tech>,
        Nicolas Saenz Julienne <nsaenz@...nel.org>,
        Nicolas Saenz Julienne <nsaenzjulienne@...e.de>
Cc:     linux-rpi-kernel@...ts.infradead.org, geert@...ux-m68k.org,
        Nicolas Saenz Julienne <nsaenz@...nel.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Michael Turquette <mturquette@...libre.com>,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] clk: bcm: rpi: Don't register as OF provider if !dev->np

Quoting Nicolas Saenz Julienne (2021-03-25 11:57:48)
> There are two ways clk-raspberrypi might be registered: through
> device-tree or through an explicit platform device registration. The
> latter happens after firmware/raspberrypi's probe, and it's limited to
> RPi3s, which solely use the ARM clock to scale CPU's frequency. That
> clock is matched with cpu0's device thanks to the ARM clock being
> registered as a clkdev.
> 
> In that scenario, don't register the device as an OF clock provider, as
> it makes no sense and will cause trouble.

What sort of trouble?

> 
> Fixes: d4b4f1b6b97e ("clk: bcm: rpi: Add DT provider for the clocks")
> Reported-by: Marek Szyprowski <m.szyprowski@...sung.com>
> Signed-off-by: Nicolas Saenz Julienne <nsaenz@...nel.org>
> ---
>  drivers/clk/bcm/clk-raspberrypi.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
> index f89b9cfc4309..27e85687326f 100644
> --- a/drivers/clk/bcm/clk-raspberrypi.c
> +++ b/drivers/clk/bcm/clk-raspberrypi.c
> @@ -337,10 +337,12 @@ static int raspberrypi_clk_probe(struct platform_device *pdev)
>         if (ret)
>                 return ret;
>  
> -       ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> -                                         clk_data);
> -       if (ret)
> -               return ret;
> +       if (dev->of_node) {

Can you add a comment to the code indicating the problem this is fixing?
I fear that we'll look back on this later and simply remove this if
condition because it's "redundant". Better to have some code comment so
we don't have to look up git history to figure out that we only call
this when the of node is populated. I'm not sure I understand what goes
wrong though. Won't the absence of dev->of_node mean the provider
doesn't do anything?

> +               ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +                                                 clk_data);
> +               if (ret)
> +                       return ret;
> +       }
>  
>         rpi->cpufreq = platform_device_register_data(dev, "raspberrypi-cpufreq",
>                                                      -1, NULL, 0);