Message-ID: <20080528153434.GA10062@doriath.ww600.siemens.net>
Date:	Wed, 28 May 2008 19:34:34 +0400
From:	Dmitry Baryshkov <dbaryshkov@...il.com>
To:	Haavard Skinnemoen <haavard.skinnemoen@...el.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, rmk+lkml@....linux.org.uk,
	lethal@...ux-sh.org, philipp.zabel@...il.com, pavel@....cz,
	tony@...mide.com, paul@...an.com, david-b@...bell.net,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>
Subject: Re: [RFC][PATCH 1/2] Clocklib: add generic framework for managing
	clocks.

Hi,

On Wed, May 28, 2008 at 03:56:45PM +0200, Haavard Skinnemoen wrote:
> Dmitry <dbaryshkov@...il.com> wrote:
> > Hi,
> > 
> > Haavard, few words before detailed comments. In this version of the patchset
> > I tried to solve the problem of dynamic unregistration of clocks.
> > Consider this example:
> > 1) Module A register CLKA
> > 2) Module B clk_gets CLKA
> > 3) Module A is unloaded
> > 3.1) As it's unloaded module A unregisters CLKA
> > 4) Module B tries to clk_get_rate on CLKA
> 
> clk_get_rate() returns unsigned long, so we're screwed. I think we need
> to prevent unloading module A until module B clk_puts CLKA.

That can be changed to plain long.

> 
> > I can think of several solutions for this problem, but IMO the most clear one is
> > to force struct clk to be fully dynamic and to use kobjects for it's management.
> > This helped me:
> > 1) to cleanup the above case
> 
> You forgot to audit all drivers to make sure they handle bogus return
> values from the clk functions appropriately.

I don't think it's a primary task...

> 
> > 2) to cleanup that "clk_alloc_function" mess (required for clock aliasing, etc)
> 
> I have to say I'm glad the "clk function" stuff is gone as I never
> really understood it...

:)

> 
> > 3) to get sysfs integration for free
> 
> I wouldn't call making struct clk twice as big as it used to be "free".
> 
> > Also, please, bear in mind, that I titled this version of the patchset as "RFC".
> > I just wanted to know your opinion on this approach. If it's less acceptable,
> > I'll get back to my previous patchset.
> 
> Yes, I think it's less acceptable than the previous patchset. But if
> other people like this patchset better, I can always stay with my own
> implementation of the clk api on avr32.

No, that's not my goal.

> > 2008/5/28 Haavard Skinnemoen <haavard.skinnemoen@...el.com>:
> > > Dmitry Baryshkov <dbaryshkov@...il.com> wrote:
> > >> Hi,
> > >>
> > >> On Tue, May 27, 2008 at 03:09:19PM -0700, Andrew Morton wrote:
> > >> > On Fri, 23 May 2008 01:21:42 +0400
> > >> > Dmitry Baryshkov <dbaryshkov@...il.com> wrote:
> > >> >
> > >> > > Provide a generic framework that platform may choose
> > >> > > to support clocks api. In particular this provides
> > >> > > platform-independant struct clk definition, a full
> > >> > > implementation of clocks api and a set of functions
> > >> > > for registering and unregistering clocks in a safe way.
> > >> > >
> > >> >
> > >> > Nobody interested in this?
> > >
> > > Sorry. I am...or at least I used to be until struct clk went all porky
> > > and got hidden away in a .c file...
> > 
> > It get hidden because in this patchset it contains only private fields
> > not intended for public usage.
> 
> "private" fields for a mid-layer that doesn't actually do anything
> translates into useless fields in my book.

Sorry, I don't understand you. Here is the definition from the C file:
struct clk {
	struct kobject kobj;
	int usage;
	const struct clk_ops *ops;
	void *data;
};

Which fields are useless from your POV? The mid-layer manages clocks
registration, so kobj. It manages usage (enablement) counters.
It serves per-clock operations. What do you dislike here?

> > >> > The documentation should perhaps explain that set_mode() is called
> > >> > under spin_lock_irqsave() and hence cannot do much.
> > >>
> > >> All functions from clk_ops are run with spinlock held.
> > >
> > > Why is that necessary?
> > >
> > > I've been thinking a bit lately about starting up oscillators through
> > > the clk framework, and that may take up to several seconds. Doing that
> > > under an irqsafe spinlock would be madness...
> > 
> > Consider two kernel threads trying to enable single clock...
> > I'm sure locking conditions can be relaxed, but I can't come up with
> > the solution yet. Maybe you can?
> 
> No, I really wish I could, but I can't think of any way to remove the
> lock. A mutex is not an option I guess since these things might get
> called from interrupt handlers and whatnot.
> 
> > >
> > >> > > +EXPORT_SYMBOL(clks_register);
> > >> >
> > >> > The patch generally has nice documentation, but these two major
> > >> > interface functions were omitted.
> > >>
> > >> Oops...
> > >
> > > Where's the interface to register a single clock btw?
> > >
> > > I have to say I really hate this "struct clk_table" thing. Another
> > > reason why struct clk should be in a header file, not a .c file.
> > 
> > See above. The struct clk incorporates the kobject, so it should
> > never ever be declared.
> 
> I don't understand that reasoning. Why can't you declare a struct that
> contains a kobject? I've allocated platform_devices statically lots of
> times -- those have embedded kobjects too.

Citing Documentation/kobject.txt:
Because kobjects are dynamic, they must not be declared statically or on
the stack, but instead, always allocated dynamically.  Future versions of
the kernel will contain a run-time check for kobjects that are created
statically and will warn the developer of this improper usage.

However as static devices do work, we can probably go that way.

> 
> Why not split out the registration bits of clk_alloc() into a separate
> clk_register() or clk_add() call?
> 
> > > Why do you have to iterate over clks_kset->list twice?
> > 
> > There are two types of clocks. Ones that are bound to devices
> > and ones that aren't. E.g. on my development board there is
> > a MMCCLK which is driven by the chip and is used by my driver
> > and there is a MMCCLK which is driven by PXA and used by
> > pxa2xx-mci driver. That's why first you try to find the clock from
> > "bound" ones (ones wich provide can_get) and then from
> > all other.
> 
> Oh.
> 
> How about making can_get() mandatory and provide a default
> clk_can_always_get() function for clocks that aren't bound to devices?

ok.

> > >> > > + spin_lock_irqsave(&clocks_lock, flags);
> > >> > > +
> > >> > > + if (!clk->ops || !clk->ops->get_rate)
> > >> > > +         goto out;
> > >> >
> > >> > Did we really need to hold the lock to perform this check?
> > >> >
> > >> > Is it really valid for a clock to have no ->ops and no -ops->get_rate?
> > >> > Please consider mandating that these fields must be provided, then
> > >> > remove this check.
> > >>
> > >> With this patch clocks can be registered and unregistered at any time.
> > >> When the clock in unregistered I drop the reference to the clk_ops (so
> > >> the module can be unregistered, but if the clock is still referenced,
> > >> those functions still can be called. That's why I have to check for the
> > >> presense of ->ops. On the other hand ->ops->get_rate can be made mandatory.
> > >
> > > So...how does the user know that the clock disappeared? Is it really a
> > > good idea to allow clocks to be unregistered while someone is using it?
> > 
> > What should we do if a user explicitly unbinds the driver that has registered
> > a clock? We can't fail that operation and say "keep the device bound".
> > So the only way is to let him use the reference, but return some error
> > for each call.
> 
> Why can't we fail that operation? We certainly can't return an error
> for all calls with the existing API.
> 
> Isn't it quite common to prevent a module from being unloaded if
> another module is using it? try_module_get() in clk_get() and
> module_put() in clk_put() should do the trick, no?
> 
> Or you can stall the unregistration until all users are gone. The DMA
> engine layer does something like that.

try_module_get()/module_put() won't help.
Stalling the unregistration sounds pretty bad also. However maybe we
should just BUG() in such cases :)

> 
> > >> > > +static unsigned long clk_get_rate_parent(struct clk *clk, void *data)
> > >> > > +{
> > >> > > + struct clk *parent = __clk_get_parent(clk);
> > >> > > + unsigned long rate = 0;
> > >> > > +
> > >> > > + BUG_ON(!parent);
> > >> > > +
> > >> > > + if (!parent->ops || !parent->ops->get_rate)
> > >> > > +         WARN_ON(1);
> > >> > > + else
> > >> > > +         rate = parent->ops->get_rate(parent, parent->data);
> > >> > > +
> > >> > > + clk_put(parent);
> > >> > > +
> > >> > > + return rate;
> > >> > > +}
> > >> >
> > >> > Can we avoid the (mysterious) void* here?  Do something which will
> > >> > allow the C type system to check our stuff?
> > >>
> > >> I don't know how we can avoid it. Each clock can have "private" data
> > >> bound to it. Previously the proposed way was to embed struct clk. However
> > >> since struct clk is dynamic now, it's not directly possible. And all
> > >> solutions I can think upon are more or less hacky.
> > >
> > > Can't you just add some sort of additional_size parameter to the alloc
> > > function to make it possible for clock drivers to extend it?
> > 
> > ... and fill the "extension" with private information. Wait! That information
> > is already present in some form in the kernel. So if we won't use a pointer
> > to it, we'll duplicate that info and in fact increase the memory cost
> > of the clock.
> > That seems like the real drawback.
> 
> Huh? Isn't that a very strong argument against keeping struct clk
> hidden? That's certainly going to cause duplication since you can't
> share any data at all between the mid-layer and the hardware driver...
> 
> I guess what I'm not understanding is why this struct clk with an
> embedded kobject in it is so special that you can't allocate it in a
> normal way (even though clk_alloc() does a quite normal kmalloc() in
> order to allocate it...)

OK.

> 
> > > Removing the kobject would of course solve this too. I forgot why we
> > > wanted a kobject in there in the first place (I think I actually said
> > > that I _didn't_ want it.)
> > 
> > Yes, I remeber that mail. But things are so easy with kobjects...
> 
> Easy?
> 
> You have to use different data structures for registration and normal
> use, all users are burdened with the task of checking whether the clock
> is actually valid every time they try to use it, sharing of data
> between the mid-layer and the hardware driver is prevented, and all
> driver hooks got an extra parameter.

Haavard, if I return struct clk definition to header, permit clocks to
be allocated statically, drop again that "priv" in favour of embedding,
would you agree on kobject-based implementation? I'd really like to use
them. Because otherwise we are nearly reimplementing them from scratch.

-- 
With best wishes
Dmitry

--
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