Messages in this thread | | | Date | Wed, 28 May 2008 19:34:34 +0400 | From | Dmitry Baryshkov <> | 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@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
| |