lkml.org 
[lkml]   [2008]   [May]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@gmail.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@atmel.com>:
> > > Dmitry Baryshkov <dbaryshkov@gmail.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@gmail.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



\
 
 \ /
  Last update: 2008-05-28 17:37    [W:0.158 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site