Message-id: <alpine.DEB.2.10.1501201214200.8428@AMDC1262.digital.local>
Date:	Tue, 20 Jan 2015 12:14:31 +0100 (CET)
From:	Paul Osmialowski <p.osmialowsk@...sung.com>
To:	Mark Brown <broonie@...nel.org>
Cc:	Wolfram Sang <wsa@...-dreams.de>, Jonathan Corbet <corbet@....net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Kukjin Kim <kgene@...nel.org>, linux-i2c@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-samsung-soc@...r.kernel.org
Subject: Re: [RFC 2/3] regmap: Use the enhancement of i2c API to address
 circular dependency problem



On Mon, 19 Jan 2015, Mark Brown wrote:

> On Mon, Jan 19, 2015 at 10:31:22AM +0100, Paul Osmialowski wrote:
>> On Fri, 16 Jan 2015, Mark Brown wrote:
>
>>> What I'm saying is that I want to understand this change from a point of
>>> view that isn't tied to I2C - at the regmap level what is this doing,
>
>> From the regmap point of view, it allows its functions to have a chance to
>> prepare transfer medium for (synchronous) transfer (no matter what bus
>> handles it) before it actually start to happen (then unprepare it when it's
>> done) and crucially before any lock is obtained in functions like
>> regmap_write(), regmap_read() or regmap_update_bits.
>
> OK, so that's what should go in the changelog (along with an explanation
> of why this preparation is required at all) - but I still don't see the
> async bit of this I'm afraid.
>

I don't think preparation stage should be exposed for asynchronous 
transfer. Due to its nature, it shouldn't cause circular lock dependency 
as we can observe for synchronous io. Since i2c does not support async 
transfer anyway (so (map->async && map->bus->async_write) will be always 
false for i2c transfers), let's use spi as an example.

With spi we have curious situation where both sync and async are handled 
by the same __spi_async() function, though for sync transfer 
wait_for_completion() is called soon after __spi_async() in order to 
ensure that transfer is completed.

Actual transfer is handled by spi_transfer_one_message() called from 
spi_pump_messages(). Note that spi_pump_message() before it calls 
spi_transfer_one_message() also calls prepare_message() callback (if 
provided):

 	if (master->prepare_message) {
 		ret = master->prepare_message(master, master->cur_msg);

So we have some candidate to expose if we would like to modify 
regmap-spi.c similarly to regmap-i2c.c. Thing is, we don't need to expose 
it for asynchronous transfer. If master->prepare_message() callback 
obtains some lock, which is unfortunately held by other task that waits 
for a lock already obtained by regmap_write_async() it will not wait 
forever since call path started in regmap_write_async() is not blocked by 
things like wait_for_completion() and should soon end calling 
map->unlock(map->lock_arg).

>> Maybe adding a pair of callbacks (map->reg_write_sync_prepared(),
>> map->reg_read_sync_prepared()) would make situation clearer.
>
> No, I don't think so - it'd just complicate the callers.
>
>>> I2C is a bus that has some properties which you're saying needs some
>>> changes, what are those properties and those changes?
>
>> I'm not saying I2C as a bus requires changes. What I'm saying is that I2C
>> API can be extended to allow more detailed control on what happens with the
>> transfer.
>
> My point here is that your explanation is in terms of I2C specifics and
> not really at a generic regmap level.
>
>>> Can you be more specific please?  If something needs preparing it seems
>>> like it'd need preparing over an async transaction just as much as over
>>> a synchronous one.
>
>> Even with those preparation and unpreparation stages, this transfer is still
>> synchronous. For example, it starts when regmap_read() starts and ends when
>> regmap_read() ends. Nothing is queued or deferred. Namely, when
>> max_gen_clk_unprepare() function calls regmap_update_bits() it expects that
>> when regmap_update_bits() returned, no outstanding transfer are happening
>> nor waiting to proceed. Everything must be completed before returning to
>> max_gen_clk_unprepare().
>
> That doesn't address my question - all you're saying is that in a
> synchronous call path things are synchronous which is fine but obviously
> regmap supports async I/O too.
>
>>> Not in this pattern where the caller needs to check too.
>
>> I don't persist on that. Apparently, you're the author of this file, though
>> regmap_init() function was later expanded by other guys. They never assigned
>> bus callback function pointers directly to map operation callbacks. It is
>> possible to replace 'map->reg_prepare_sync_io = regmap_bus_prepare_sync_io'
>> with 'map->reg_prepare_sync_io = map->bus->prepare_sync_io' - this will
>> compile and this will work properly. But IMHO it wouldn't match with what
>> the others did.
>
> If you look at the other callbacks they're doing other things beyond
> simply forwarding the functions on.  That's the problem here, the
> functions just add a layer of indirection and nothing else.
>

Yes, I added layer of indirection just because the others added layer of 
indirection - there is no functional requirement behind that. It can be 
easily removed.
--
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