linux-kernel - Re: [PATCH v3] i2c: mux: remove duplicated i2c

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]

Date:   Sun, 18 Nov 2018 12:13:51 +0100
From:   Luca Ceresoli <luca@...aceresoli.net>
To:     Luca Ceresoli <lucaceresoli77@...il.com>,
        Peter Rosin <peda@...ntia.se>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Wolfram Sang <wsa@...-dreams.de>
Subject: Re: [PATCH v3] i2c: mux: remove duplicated i2c_algorithm

Hi Peter,

On 10/10/18 17:48, Luca Ceresoli wrote:
> Hi Peter,
> 
> On 08/10/2018 23:43, Peter Rosin wrote:
>> On 2018-10-03 17:19, Luca Ceresoli wrote:
>>> From: Luca Ceresoli <luca@...aceresoli.net>
>>>
>>> i2c-mux instantiates one i2c_algorithm for each downstream adapter.
>>> However these algorithms are all identical, depending only on the
>>> parent adapter.
>>>
>>> Avoid duplication by hoisting the i2c_algorithm from the adapters to
>>> the i2c_mux_core object, and reuse it in all the adapters.
>>
>> Ouch, while I like the concept of having one i2c_algorithm per mux,
>> this patch is not working. Various i2c-mux drivers set the
>> muxc->mux_locked variable *after* the i2c_mux_alloc call, and this
>> patch breaks such use.

I finally had a look into this issue. Three drivers are setting
mux_locked after i2c_mux_alloc: i2c-mux-gpmux, i2c-mux-gpio and
i2c-mux-pinctrl.

i2c-mux-gpmux is trivial to fix.

The other two are not trivial because:

 1. they compute mux_locked from other variables
 2. those variables are stored in the drivers "private" data
 3. their private data is stored inside struct i2c_mux_core
    (muxc->priv) which exists only after i2c_mux_alloc()

In those cases computing mux_locked before i2c_mux_alloc() involves
quite invasive changes. It took 3 non-trivial commits just for
i2c-mux-gpio, and I still have to look into i2c-mux-pinctrl.

So the question is: do we really want to do this?

Using the private storage provided by i2c_mux_alloc() is a handy
feature, at least for simpler drivers which know in advance the flags
they need to set. OTOH I don't like individual drivers to manipulate
mux_core flags that look very much like internal data. It makes any
change to the i2c mux core harder, since every changed line could have
side effects in some drivers, which is what's happening here.

What's your opinion about this issue?

Thanks,
-- 
Luca