linux-kernel - Re: Locking in the clk API

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: <20110112074009.GX24920@pengutronix.de>
Date:	Wed, 12 Jan 2011 08:40:09 +0100
From:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
To:	Saravana Kannan <skannan@...eaurora.org>
Cc:	Jeremy Kerr <jeremy.kerr@...onical.com>,
	Paul Mundt <lethal@...ux-sh.org>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	linux-sh@...r.kernel.org,
	Ben Herrenschmidt <benh@...nel.crashing.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	linux-kernel@...r.kernel.org,
	Vincent Guittot <vincent.guittot@...aro.org>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: Locking in the clk API

Hi Saravana,

On Tue, Jan 11, 2011 at 07:25:01PM -0800, Saravana Kannan wrote:
> On 01/11/2011 06:35 AM, Jeremy Kerr wrote:
> >Hi Paul,
> >
> >>Again, you are approaching it from the angle that an atomic clock is a
> >>special requirement rather than the default behaviour.
> >
> >I'm not considering it a special requirement, but it's still a requirement
> >(that the called function does not sleep).
> >
> >The problem with the inverse logic (clk_enable/clk_enable_sleepable) is that
> >now you've made the caller need to know what kind of clock it has, or might
> >have one day.
> 
> I think it's just a matter of how you interpret the name of the API
> in English. It doesn't make the decision making of the developer any
> easier.
> 
> Just having a _atomic suffix doesn't mean the driver developer
> doesn't need to know what type of clock it is. They are still making
> the assumption that the enable/disable for that clock can be done
> atomically -- namely an "atomic clock".
But there is a difference to 'one function to rule both sleepable and
atomic clocks'.  When calling _atomic on a sleepable clock you get
-ESOMETHING back (and the clock stays off).  With a generic clk_enable
you get an oops and so cannot handle the error.

> Similarly, when a driver developer calls the _sleepable APIs in
> their code, for all practical purposes, they are making an
> assumption that the enable/disable for that clock *needs to* (not
> may) sleep.
IMHO this is not right.  If the driver developer doesn't care if the
clock sleeps or not (which is the norm I think) he calls the _sleepable
function and if the clock happen to be an atomic one it doesn't hurt
him.

And looking at the usage of the sleeping functions in the gpio API, I'd
bet that at least 50% of the calls to gpio_set_value can/should be
gpio_set_value_cansleep.  That's because driver developers don't care or
are not aware of the issue.  If it would be gpio_set_value
vs.gpio_set_value_atomic most developers would use the sleeping variant
and the few that should use the _atomic function would notice that when
seeing the corresponding oops.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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/