linux-kernel - Re: [PATCH] clk: return proper ERR_PTR for clk_get when !HAVE

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: <20150209142954.GA8024@katana>
Date:	Mon, 9 Feb 2015 15:29:54 +0100
From:	Wolfram Sang <wsa@...-dreams.de>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] clk: return proper ERR_PTR for clk_get when !HAVE_CLK

Russell,

thanks for the details again!

>  * Returns a struct clk corresponding to the clock producer, or
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  * valid IS_ERR() condition containing errno.  The implementation
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Yes, I read that...

> The NULL value is basically undefined for drivers, and according to the
> definition above, drivers should treat it as "success".

... and this is what I stumbled over. For me, the NULL pointer meant
failure. But given your background information, especially about the
intentional and defined usage of the pointers returned by clk_get, I
get a better picture of the NULL return value.

> The problem comes is that you seem to want something different to that.
> This is for opencores, right?  It's in much the same position as
> everything else - you don't want the driver to fail when we have
> !CONFIG_HAVE_CLK, but you need the clock frequency internally.

Yes, but it is a bit more complicated...

> I'm not sure what you do with clock_frequency_present and bus_clock_khz
> so that bit of the above is a guess (I'm not sure why you have it in the
> if (!IS_ERR()) { } block.)

... which has to do with this. Historically, "clock-frequency" in DT
means the i2c bus speed. This driver used the binding wrong to specify
the IP core clock speed instead and also used a fixed I2C bus speed
then. Now, when wanting to support a) flexible I2C bus speeds and b)
clock information from standard DT clock bindings, we also need to
ensure that we support the now deprecated and wrong binding as a
fallback. And even after having learnt something about the clk API now,
I still think the best fix is to replace all instances of 'if
(!IS_ERR(dev->clk))' with (!IS_ERR_OR_NULL()). Because in our case, NULL
is not success. We should skip any clk API interaction then and use the
fallback mechanisms.

   Wolfram

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)