linux-kernel - Re: [PATCH 1/2] Add a common struct clk

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]

Date:	Thu, 6 Jan 2011 21:11:38 +0100
From:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
To:	Richard Cochran <richardcochran@...il.com>
Cc:	Jeremy Kerr <jeremy.kerr@...onical.com>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Ben Herrenchmidt <benh@...nel.crashing.org>
Subject: Re: [PATCH 1/2] Add a common struct clk

On Thu, Jan 06, 2011 at 05:07:52PM +0100, Richard Cochran wrote:
> On Wed, Jan 05, 2011 at 11:51:02AM +0800, Jeremy Kerr wrote:
> 
> > + * The @lock member provides either a spinlock or a mutex to protect (at least)
> > + * @enable_count. The type of lock used will depend on @flags; if CLK_ATOMIC is
> > + * set, then the core clock code will use a spinlock, otherwise a mutex. This
> > + * lock will be acquired during clk_enable and clk_disable, so for atomic
> > + * clocks, these ops callbacks must not sleep.
> > + *
> > + * The choice of atomic or non-atomic clock depends on how the clock is enabled.
> > + * Typically, you'll want to use a non-atomic clock. For clocks that need to be
> > + * enabled/disabled in interrupt context, use CLK_ATOMIC. Note that atomic
> > + * clocks with parents will typically cascade enable/disable operations to
> > + * their parent, so the parent of an atomic clock *must* be atomic too.
> 
> ...
> 
> > +struct clk {
> > +	const struct clk_ops	*ops;
> > +	unsigned int		enable_count;
> > +	int			flags;
> > +	union {
> > +		struct mutex	mutex;
> > +		spinlock_t	spinlock;
> > +	} lock;
> > +};
> 
> Here you have a "polymorphic" lock, where the clock instance knows
> which type it is supposed to be.  I got flak from David Miller and
> others trying to do the same thing with the mdio_bus:
> 
>    http://kerneltrap.org/mailarchive/linux-netdev/2010/7/6/6280618
> 
> The criticism, applied to your case, is that the clk_enable() caller
> cannot know whether it is safe to make the call or not. I was told,
> "there has got to be a better way."
Note that this is not "new".  Currently there is no convention available
if clk_enable sleeps or not.  See e.g.
http://thread.gmane.org/gmane.linux.ports.arm.kernel/100744

So if there is no consent and you want to introduce common code there is
no choice.

I don't like it, too.  IMHO clk_enable should be allowed to sleep, but I
see the concerns of the other camp, too.  If you know the better way
that has to exists, please tell us.

(Hmm, the way the gpio api does it comes to mind:

	clk_enable_atomic
	clk_enable_cansleep

(where one of these can be named clk_enable).)

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/