linux-kernel - Re: [PATCHv2 1/5] clk: samsung: exynos5410: Add NULL pointer checks in clock init

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: <CAEe5pB-FnkY8E9qPjy+6R5ausEEx-pO2wupWYZO8Lscb2BAwiw@mail.gmail.com>
Date:	Thu, 31 Jul 2014 15:13:37 +0200
From:	Humberto Naves <hsnaves@...il.com>
To:	Tomasz Figa <tomasz.figa@...il.com>
Cc:	linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org, devicetree@...r.kernel.org,
	Kukjin Kim <kgene.kim@...sung.com>,
	Tomasz Figa <t.figa@...sung.com>,
	Thomas Abraham <ta.omasab@...il.com>,
	Andreas Farber <afaerber@...e.de>,
	Randy Dunlap <rdunlap@...radead.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>
Subject: Re: [PATCHv2 1/5] clk: samsung: exynos5410: Add NULL pointer checks
 in clock init

Hi,

I am bit confused by your response: first you mentioned that I should
remove the NULL check for variable np, but later on you suggested that
I should rearrange the conditional statement to avoid adding more
indentation. My guess is that I should remove that if statement
altogether?

Regarding the ctx variable, should I still remove the NULL check? As
you said, in the near future samsung_clk_init() won't panic anymore,
and keeping the check in place won't hurt anybody.

Best,
Humberto

On Thu, Jul 31, 2014 at 2:34 PM, Tomasz Figa <tomasz.figa@...il.com> wrote:
> Hi Humberto,
>
> Please see my comments inline.
>
> On 31.07.2014 13:22, Humberto Silva Naves wrote:
>> Added NULL pointer checks for device_node input parameter and
>> for the samsung_clk_provider context returned by samsung_clk_init.
>> Even though the *current* samsung_clk_init function never returns
>> a NULL pointer, it is good to keep this check in place to avoid
>> possible problems in the future due to changes in implementation.
>> That way, we also improve the consistency of the code that performs
>> clock initialization across the different SoCs.
>>
>> Signed-off-by: Humberto Silva Naves <hsnaves@...il.com>
>> ---
>>  drivers/clk/samsung/clk-exynos5410.c |   12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos5410.c b/drivers/clk/samsung/clk-exynos5410.c
>> index 231475b..bf57c80 100644
>> --- a/drivers/clk/samsung/clk-exynos5410.c
>> +++ b/drivers/clk/samsung/clk-exynos5410.c
>> @@ -188,11 +188,17 @@ static void __init exynos5410_clk_init(struct device_node *np)
>>       struct samsung_clk_provider *ctx;
>>       void __iomem *reg_base;
>>
>> -     reg_base = of_iomap(np, 0);
>> -     if (!reg_base)
>> -             panic("%s: failed to map registers\n", __func__);
>> +     if (np) {
>
> Since all Exynos-based boards are always booted using DT, this function
> will never be called if there is no node for the clock controller and so
> there is no way this pointer can end up being NULL. I don't see a point
> in complicating this code with useless checks.
>
>> +             reg_base = of_iomap(np, 0);
>> +             if (!reg_base)
>> +                     panic("%s: failed to map registers\n", __func__);
>> +     } else {
>> +             panic("%s: unable to determine soc\n", __func__);
>> +     }
>
> As a side note, since panic() does not return, the code above could be
> changed to follow rest of checks in this function:
>
>         if (!np)
>                 panic("%s: unable to determine soc\n", __func__);
>
>         reg_base = of_iomap(np, 0);
>         ...
>
> leading to more readable code with less indentation and less changes to
> existing code.
>
>>
>>       ctx = samsung_clk_init(np, reg_base, CLK_NR_CLKS);
>> +     if (!ctx)
>> +             panic("%s: unable to allocate context.\n", __func__);
>
> samsung_clk_init() already panics on any error, although now as I think
> of it, it probably should be changed with a patch to just error out and
> let the caller handle the error. However callers don't need to be
> changed before this is done.
>
> Best regards,
> Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/