Hi,

On Wed, May 28, 2008 at 03:56:45PM +0200, Haavard Skinnemoen wrote:
quoted text
> 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.

quoted text
> 
> > 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...

quoted text
> 
> > 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...

:)

quoted text
> 
> > 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.

quoted text
> > 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?

quoted text
> > >> > 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.

quoted text
> 
> 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.

quoted text
> > >> > > + 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 :)

quoted text
> 
> > >> > > +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.

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

--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/