Message-ID: <13300609.Rh0Q97QhcX@avalon>
Date:	Mon, 28 Oct 2013 21:44:25 +0100
From:	Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:	Sylwester Nawrocki <sylvester.nawrocki@...il.com>
Cc:	linux@....linux.org.uk, mturquette@...aro.org,
	Sylwester Nawrocki <s.nawrocki@...sung.com>,
	linux-arm-kernel@...ts.infradead.org, jiada_wang@...tor.com,
	kyungmin.park@...sung.com, myungjoo.ham@...sung.com,
	t.figa@...sung.com, g.liakhovetski@....de,
	linux-kernel@...r.kernel.org, linux-mips@...ux-mips.org,
	linux-sh@...r.kernel.org
Subject: Re: [PATCH v6 0/5] clk: clock deregistration support

Hi Sylwester,

On Wednesday 25 September 2013 22:51:08 Sylwester Nawrocki wrote:
> On 09/25/2013 11:47 AM, Laurent Pinchart wrote:
> > Doesn't that introduce race conditions ? If the sub-drivers require the
> > clock, they want to be sure that the clock won't disappear beyond their
> > backs. I agree that the circular dependency needs to be solved somehow,
> > but we probably need a more generic solution. The problem will become
> > more widespread in the future with DT-based device instantiation in both
> > V4L2 and KMS.
> 
> It doesn't introduce any new race conditions AFAICT. I doubt all these
> issues can be resolved in one single step. Currently the modular clock
> providers are seriously broken, there is no reference tracking and the clock
> consumers can easily get into a state where they have invalid references to
> clocks supplied by already unregistered drivers.
> 
> With this patch series the clock consumer drivers will not crash thanks
> to the clock object reference counting. So the worst thing may happen is a
> clock left in an unexpected state.
> 
> However there should be no problems with the v4l2-async API, the host driver
> in its de-initialization routine unregisters its sub-drivers (they should
> stop using the clock when notified of such an event), only then the host
> would unregister the clock (subsequently the sub-drivers get re-attached and
> put into deferred probing state).

That in itself is a workaround I believe. Unbinding/rebinding devices from/to 
drivers isn't something the v4l core should do.

> There may be issues when a sub-driver's file handle is opened while the host
> is about to de-initialize. But I doubt resolution of such problems belongs
> to the common clock framework. I have been trying to improve the situation
> in small steps, rather than waiting forever for a perfect solution.
> 
> Do you perhaps have any ideas WRT to a "more generic solution" ? In general
> I have been trying to avoid using v4l2-clk and add what's missing in the
> common clock framework.
> 
> Perhaps we should leave only the kref part in the __clk_get(), __clk_put()
> hooks and be taking reference to a clock in clk_prepare() and releasing it
> in clk_unprepare() ? This way circular reference would exist only between
> clk_prepare(), clk_unprepare() calls.
> 
> Assuming a driver prepares clock in device's open() and unprepares in device
> close() handler perhaps it could all work better, without relying on the
> host to ensure the resource reference tracking. I'm not sure if that is not
> making too many assumptions for a generic API.

This is indeed an architecture decision that goes beyond the boundaries of the 
clock framework. The question boils down to how we want to acquire/release and 
refcount resources. Should drivers acquire and release hotpluggable resources 
at probe and remove time respectively, or only when they need them ? Or should 
they acquire them at probe them and be notified when they should release them 
? The first option adds an overhead but could help solving the circular 
dependency problem in a simpler way.

-- 
Regards,

Laurent Pinchart

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