linux-kernel - Re: [RFC 2/3] regmap: Use the enhancement of i2c API to address circular dependency problem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150116162314.GB3856@sirena.org.uk>
Date:	Fri, 16 Jan 2015 16:23:14 +0000
From:	Mark Brown <broonie@...nel.org>
To:	Paul Osmialowski <p.osmialowsk@...sung.com>
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 Fri, Jan 16, 2015 at 03:39:53PM +0100, Paul Osmialowski wrote:

> This uses the enhancement of i2c API in order to address following problem
> caused by circular lock dependency:

Please don't just dump enormous backtraces into commit messages as
explanations, explain in words what the problem you are trying to
address is.  If the backtrace is longer than the commit message things
probably aren't working well, and similarly if the first thing the
reader sees is several screenfuls of backtrace that's not really aiding
understanding.

This is all particularly important with something like locking where a
clear understanding of the rules and assumptions being made is very
important to ensuring correctness, it's easy to just paper over a
specific problem and miss a bigger problem or introduce new ones.

> Apparently regulator and clock try to acquire lock which is protecting the
> same regmap. Communication over i2c requires clock to be started. Both things
> require access to the same regmap in order to complete.
> To solve this, i2c clock should be started before attempting operation on
> regulator (which requires locked regmap).

It sounds awfully like something is not doing the right thing with
preparing clocks somewhere along the line but since there's no
analysis it's hard to tell (I don't propose to spend time trawling
backtraces for something I don't know).

Please also use blank lines between paragraphs like all the other commit
messages, it makes things easier to read.

> Exposing preparation and unpreparation stage of i2c transfer serves this
> purpose.

I don't know what this means, sorry.  I'm also very worried about the
fact that this is being discussed purely in terms of I2C - why would
this not affect other buses?

> Note that this change does not require modifications in other places.
> 
> Signed-off-by: Paul Osmialowski <p.osmialowsk@...sung.com>
> ---
>  drivers/base/regmap/internal.h   |   2 +
>  drivers/base/regmap/regmap-i2c.c |  18 +++++++
>  drivers/base/regmap/regmap.c     | 107 ++++++++++++++++++++++++++++++++++++++-
>  include/linux/regmap.h           |   7 +++
>  4 files changed, 133 insertions(+), 1 deletion(-)

Modification may not be required in other places but this is an
*enormous* change in the code which I can't really review.

> +	int (*reg_prepare_sync_io)(void *context);
> +	void (*reg_unprepare_sync_io)(void *context);

The first question here is why this only affects synchronous I/O or
alternatively why these operations have _sync in the name if they aren't
for synchronous I/O.

> +	if (bus) {
> +		map->reg_prepare_sync_io = regmap_bus_prepare_sync_io;
> +		map->reg_unprepare_sync_io = regmap_bus_unprepare_sync_io;
> +	}

Why are we using these indirections instead of assigning the operation
directly?  They...

> +static int regmap_bus_prepare_sync_io(void *context)
> +{
> +	struct regmap *map = context;
> +
> +	if (map->bus->prepare_sync_io)
> +		return map->bus->prepare_sync_io(map->bus_context);
> +
> +	return 0;
> +}

...seem to simply check for the operation which appears redundant
especially given that the caller...

> +static void regmap_unprepare_sync_io(struct regmap *map)
> +{
> +	void *context = _regmap_map_get_context(map);
> +
> +	if (map->reg_unprepare_sync_io)
> +		map->reg_unprepare_sync_io(context);
> +}

...does essentially the same check again on every call anyway.

> @@ -1491,12 +1536,18 @@ int regmap_write(struct regmap *map, unsigned int reg, unsigned int val)
>  	if (reg % map->reg_stride)
>  		return -EINVAL;
>  
> +	ret = regmap_prepare_sync_io(map);
> +	if (ret)
> +		return ret;
> +
>  	map->lock(map->lock_arg);

So what are the rules for calling this operation and how are they
different to those for locking the map?  It looks like they might be the
same in which case it seems better to combine them rather than having
to update every single caller and remember why they're always being
worked with in tandem.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)