| 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
| ||
|
Message-ID: <1276239018.1962.118.camel@pasglop>
Date: Fri, 11 Jun 2010 16:50:18 +1000
From: Benjamin Herrenschmidt <benh@...nel.crashing.org>
To: Ben Dooks <ben-linux@...ff.org>
Cc: Jeremy Kerr <jeremy.kerr@...onical.com>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC,PATCH 1/2] Add a common struct clk
On Fri, 2010-06-11 at 05:20 +0100, Ben Dooks wrote:
> On Fri, Jun 04, 2010 at 03:30:08PM +0800, Jeremy Kerr wrote:
> > We currently have 21 definitions of struct clk in the ARM architecture,
> > each defined on a per-platform basis. This makes it difficult to define
> > platform- (or architecture-) independent clock sources without making
> > assumptions about struct clk, and impossible to compile two
> > platforms with different struct clks into a single image.
>
> Whilst I agree that this is a useful thing to do, I'd like to ensure
> we get a good base for our work before we get another reason for Linus
> to complain about churn.
Well, in this case the goal is to unify things, both within ARM and
between architectures, so I fail to see Linus complaining about that :-)
> What do people think of just changing everyone who is currently using
> clk support to using this new implementation?
It's risky I suppose... there isn't many users of struct clk in powerpc
land today (I think one SoC platform only uses it upstream at the
moment) so I won't mind getting moved over at once but on ARM, you have
to deal with a lot more cruft that might warrant a more progressive
migration approach... but I'll let you guys judge.
> > struct clk {
> > const struct clk_ops *ops;
> > unsigned int enable_count;
> > struct mutex mutex;
>
> I'm a little worried about the size of struct clk if all of them
> have a mutex in them. If i'm right, this is 40 bytes of data each
> clock has to take on board.
>
> Doing the following:
>
> find arch/arm -type f -name "*.c" | xargs grep -c -E "struct.*clk.*=.*{" | grep -v ":0" | awk 'BEGIN { count=0; FS=":" }
> count += $2; END { print count }'
>
> indicates that there's approximately 2241 clocks under arch/arm at the
> moment. That's about 87KiB of mutexes if all are being compiled at
> once.
I'm not convince this is relevant. You assume that all 2241 of those
clocks are statically allocated -and- compiled at the same time in the
same kernel binary.
I think both assumptions are terribly unlikely, especially in ARM
land :-) And in the event where you would actually manage to achieve
such a thing, I believe 87K is going to be the very last of your
worries :-)
In case it is of interest (and I know not everybody will want to do like
that) the way I intend to use this on powerpc is to have static clk_ops,
but instanciate the struct clk (or rather subclasses of struct clk)
on demand at clk_get time.
Basically, the device-tree (or platform code as a fallback) will bind
a device clock input to a clock provider / output pair. The clock
provider is something that produces struct clk * on demand for its
outputs.
Cheers,
Ben.
> ~> };
> >
> > And a set of clock operations (defined per type of clock):
> >
> > struct clk_operations {
> > int (*enable)(struct clk *);
> still think that not passing an bool as a second argument is silly.
>
> > void (*disable)(struct clk *);
> ~> unsigned long (*get_rate)(struct clk *);
> > [...]
> > };
> >
> > To define a hardware-specific clock, machine code can "subclass" the
> > struct clock into a new struct (adding any device-specific data), and
> > provide a set of operations:
> >
> > struct clk_foo {
> > struct clk clk;
> > void __iomem *some_register;
> > };
> >
> > struct clk_operations clk_foo_ops = {
> > .get_rate = clk_foo_get_rate,
> > };
> >
> > The common clock definitions are based on a development patch from Ben
> > Herrenschmidt <benh@...nel.crashing.org>.
> >
> > Signed-off-by: Jeremy Kerr <jeremy.kerr@...onical.com>
> >
> > ---
> > arch/Kconfig | 3
> > include/linux/clk.h | 159 ++++++++++++++++++++++++++++++++++++--------
> > 2 files changed, 135 insertions(+), 27 deletions(-)
> ~~~~>
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index acda512..2458b5e 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -151,4 +151,7 @@ config HAVE_MIXED_BREAKPOINTS_REGS
> > config HAVE_USER_RETURN_NOTIFIER
> > bool
> >
> > +config USE_COMMON_STRUCT_CLK
> > + bool
> > +
> > source "kernel/gcov/Kconfig"
> > diff --git a/include/linux/clk.h b/include/linux/clk.h
> > index 1d37f42..bb6957a 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -3,6 +3,7 @@
> > *
> > * Copyright (C) 2004 ARM Limited.
> > * Written by Deep Blue Solutions Limited.
> > + * Copyright (c) 2010 Jeremy Kerr <jeremy.kerr@...onical.com>
> > *
> > * This program is free software; you can redistribute it and/or modify
> > * it under the terms of the GNU General Public License version 2 as
> > @@ -11,36 +12,125 @@
> > #ifndef __LINUX_CLK_H
> > #define __LINUX_CLK_H
> >
> > -struct device;
> > +#include <linux/err.h>
> > +#include <linux/mutex.h>
> >
> > -/*
> > - * The base API.
> > +#ifdef CONFIG_USE_COMMON_STRUCT_CLK
> > +
> > +/* If we're using the common struct clk, we define the base clk object here,
> > + * which will be 'subclassed' by device-specific implementations. For example:
> > + *
> > + * struct clk_foo {
> > + * struct clk;
> > + * [device specific fields]
> > + * };
> > + *
> > + * We define the common clock API through a set of static inlines that call the
> > + * corresponding clk_operations. The API is exactly the same as that documented
> > + * in the !CONFIG_USE_COMMON_STRUCT_CLK case.
> > */
> >
> > +struct clk {
> > + const struct clk_ops *ops;
> > + unsigned int enable_count;
> > + struct mutex mutex;
> > +};
>
> how about defining a nice kerneldoc for this.
>
> > +#define INIT_CLK(name, o) \
> > + { .ops = &o, .enable_count = 0, \
> > + .mutex = __MUTEX_INITIALIZER(name.mutex) }
>
> how about doing the mutex initinitialisation at registration
> time, will save a pile of non-zero code in the image to mess up
> the compression.
>
> ~> +struct clk_ops {
> > + int (*enable)(struct clk *);
> > + void (*disable)(struct clk *);
> > + unsigned long (*get_rate)(struct clk *);
> > + void (*put)(struct clk *);
> > + long (*round_rate)(struct clk *, unsigned long);
> > + int (*set_rate)(struct clk *, unsigned long);
> > + int (*set_parent)(struct clk *, struct clk *);
> > + struct clk* (*get_parent)(struct clk *);
>
> should each clock carry a parent field and the this is returned by
> the get parent call.~~
>
> > +};
> > +
> > +static inline int clk_enable(struct clk *clk)
> > +{
> > + int ret = 0;
> > +
> > + if (!clk->ops->enable)
> > + return 0;
> > +
> > + mutex_lock(&clk->mutex);
> > + if (!clk->enable_count)
> > + ret = clk->ops->enable(clk);
> > +
> > + if (!ret)
> > + clk->enable_count++;
> > + mutex_unlock(&clk->mutex);
> > +
> > + return ret;
> > +}
>
> So we're leaving the enable parent code now to each implementation?
>
> I think this is a really bad decision, it leaves so much open to bad
> code repetition, as well as something the core should really be doing
> if it had a parent clock field.
>
> ~> +static inline void clk_disable(struct clk *clk)
> > +{
> > + if (!clk->ops->enable)
> > + return;
>
> so if we've no enable call we ignore disable too?
>
> also, we don't keep an enable count if this fields are in use,
> could people rely on this being correct even if the clock has
> no enable/disable fields.
>
> Would much rather see the enable_count being kept up-to-date
> no matter what, given it may be watched by other parts of the
> implementation, useful for debug info, and possibly useful if
> later in the start sequence the clk_ops get changed to have this
> field.~
>
> > +~ mutex_lock(&clk->mutex);
> > +
> > + if (!--clk->enable_count)
> > + clk->ops->disable(clk);
> > +
> > + mutex_unlock(&clk->mutex);
> > +}
> > +
> > +static inline unsigned long clk_get_rate(struct clk *clk)
> > +{
> > + if (clk->ops->get_rate)
> > + return clk->ops->get_rate(clk);
> > + return 0;
> > +}
> > +
> > +static inline void clk_put(struct clk *clk)
> > +{
> > + if (clk->ops->put)
> > + clk->ops->put(clk);
> > +}
>
> I'm beginging to wonder if we don't just have a set of default ops
> that get set into the clk+ops at registration time if these do
> not have an implementation.
> ~
> > +static inline long clk_round_rate(struct clk *clk, unsigned long rate)
> > +{
> > + if (clk->ops->round_rate)
> > + return clk->ops->round_rate(clk, rate);
> > + return -ENOSYS;
> > +}
> > +
> > +static inline int clk_set_rate(struct clk *clk, unsigned long rate)
> >~ +{
> > + if (clk->ops->set_rate)
> > + return clk->ops->set_rate(clk, rate);
> > + return -ENOSYS;
> > +}
> > +
> > +static inline int clk_set_parent(struct clk *clk, struct clk *parent)
> > +{
> > + if (clk->ops->set_parent)
> > + return clk->ops->set_parent(clk, parent);
> > + return -ENOSYS;
> > +}
>
> We have an interesting problem here which I belive should be dealt
> with, what happens when the clock's parent is changed with respect
> to the enable count of the parent.
>
> With the following instance:
>
> we have clocks a, b, c;
> a and b are possible parents for c;
> c starts off with a as parent
>
> then the driver comes along:
>
> 1) gets clocks a, b, c;
> 2) clk_enable(c);
> 3) clk_set_parent(c, b);
>
> now we have the following:
>
> A) clk a now has an enable count of non-zero
> B) clk b may not be enabled
> C) even though clk a may now be unused, it is still running
> D) even though clk c was enabled, it isn't running since step3
>
> this means that either any driver that is using a multi-parent clock
> has to deal with the proper enable/disable of the parents (this is
> is going to lead to code repetition, and I bet people will get it
> badly wrong).
>
> I belive the core of the clock code should deal with this, since
> otherwise we end up with the situation of the same code being
> repeated throughout the kernel.
>
> > +static inline struct clk *clk_get_parent(struct clk *clk)
> > +{
> > + if (clk->ops->get_parent)
> > + return clk->ops->get_parent(clk);
> > + return ERR_PTR(-ENOSYS);
> > +}
> >
> > +#else /* !CONFIG_USE_COMMON_STRUCT_CLK */
> >
> > /*
> > - * struct clk - an machine class defined object / cookie.
> > + * Global clock object, actual structure is declared per-machine
> > */
> > struct clk;
> >
> > /**
> > - * clk_get - lookup and obtain a reference to a clock producer.
> > - * @dev: device for clock "consumer"
> > - * @id: clock comsumer ID
> > - *
> > - * Returns a struct clk corresponding to the clock producer, or
> > - * valid IS_ERR() condition containing errno. The implementation
> > - * uses @dev and @id to determine the clock consumer, and thereby
> ~> - * the clock producer. (IOW, @id may be identical strings, but
> > - * clk_get may return different clock producers depending on @dev.)
> > - *
> > - * Drivers must assume that the clock source is not enabled.
> > - *
> > - * clk_get should not be called from within interrupt context.
> > - */
> > -struct clk *clk_get(struct device *dev, const char *id);
> > -
> > -/**
> > * clk_enable - inform the system when the clock source should be running.
> > * @clk: clock source
> > *
> > @@ -83,12 +173,6 @@ unsigned long clk_get_rate(struct clk *clk);
> > */
> > void clk_put(struct clk *clk);
> >
> > -
> > -/*
> > - * The remaining APIs are optional for machine class support.
> > - */
> > -
> > -
> > /**
> > * clk_round_rate - adjust a rate to the exact rate a clock can provide
> > * @clk: clock source
> > @@ -125,6 +209,27 @@ int clk_set_parent(struct clk *clk, struct clk *parent);
> > */
> > struct clk *clk_get_parent(struct clk *clk);
> >
> > +#endif /* !CONFIG_USE_COMMON_STRUCT_CLK */
> > +
> > +struct device;
> > +
> > +/**
> > + * clk_get - lookup and obtain a reference to a clock producer.
> > + * @dev: device for clock "consumer"
> > + * @id: clock comsumer ID
> > + *
> > + * Returns a struct clk corresponding to the clock producer, or
> > + * valid IS_ERR() condition containing errno. The implementation
> > + * uses @dev and @id to determine the clock consumer, and thereby
> > + * the clock producer. (IOW, @id may be identical strings, but
> > + * clk_get may return different clock producers depending on @dev.)
> > + *
> > + * Drivers must assume that the clock source is not enabled.
> > + *
> > + * clk_get should not be called from within interrupt context.
> ~~> + */
> > +struct clk *clk_get(struct device *dev, const char *id);
> > +
> > /**
> > * clk_get_sys - get a clock based upon the device name
> > * @dev_id: device name
> > --
> > 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/
>
--
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/
Powered by blists - more mailing lists