linux-kernel - Re: [PATCH v2 1/7] i2c: mux: add the ability to share mux core address with child nodes

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] [day] [month] [year] [list]

Date: Tue, 7 May 2024 11:48:41 +0200
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Peter Rosin <peda@...ntia.se>, Farouk Bouabid <farouk.bouabid@...rry.de>,
 Wolfram Sang <wsa+renesas@...g-engineering.com>,
 Andi Shyti <andi.shyti@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Quentin Schulz <quentin.schulz@...obroma-systems.com>
Cc: linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v2 1/7] i2c: mux: add the ability to share mux core
 address with child nodes

Hi Peter,

On 5/6/24 11:26 PM, Peter Rosin wrote:
> Hi!
> 
> Regarding the subject (and elsewhere) I think of "mux core" as roughly
> the code in the i2c-mux.c file. So, for me, the "mux core" does not have
> an address, it is a mux "driver instance" or "device" that sits on the
> I2C address that you need to share.
> 

I'm the one who suggested mux core here (privately) :)

The issue is that in my head a mux device is the i2c adapter/bus (from 
i2c-mux.yaml dt binding example):

"""
     i2c {
         #address-cells = <1>;
         #size-cells = <0>;

         i2c-mux@70 {
             compatible = "nxp,pca9548";
             reg = <0x70>;
             #address-cells = <1>;
             #size-cells = <0>;

             i2c@3 {
                 #address-cells = <1>;
                 #size-cells = <0>;
                 reg = <3>;

                 gpio@20 {
                     compatible = "nxp,pca9555";
                     gpio-controller;
                     #gpio-cells = <2>;
                     reg = <0x20>;
                 };
             };
             i2c@4 {
                 #address-cells = <1>;
                 #size-cells = <0>;
                 reg = <4>;

                 gpio@20 {
                     compatible = "nxp,pca9555";
                     gpio-controller;
                     #gpio-cells = <2>;
                     reg = <0x20>;
                 };
             };
         };
     };
"""

"mux core" here would refer to i2c-mux@70, "mux device"/"mux" i2c@3 or 
i2c@4. E.g. when I'm saying "in mux 3", I'm talking about i2c@3 here.

For me a driver instance is a device, so "mux driver instance" would be 
a "mux device". Ah... naming is hard. Anyway, up to you, I just wanted 
to make sure we're talking about the same thing and there's no confusion 
here.

[...]
> I also wonder if that second condition (...->type == &i2c_client_type) should
> be a WARN_ON_ONCE? I don't see how the flag can be set sanely on an adapter
> that is not itself an I2C client. Can it?
> 

Agreed, good suggestion here... Though... 
https://lwn.net/Articles/969923/ it seems new additions of WARN_ON are 
now discouraged? Not looking to start a discussion here about whether 
WARN_ON is good or bad, merely pointing at this if it was missed somehow.

>> +
>> +		if (!quirks)
>> +			return -ENOMEM;
>> +
>> +		if (parent->quirks)
>> +			memcpy(quirks, parent->quirks, sizeof(*quirks));
>> +
>> +		quirks->flags |= I2C_AQ_SKIP_ADDR_CHECK;
>> +		quirks->skip_addr_in_parent = client->addr;
>> +		priv->adap.quirks = quirks;
> 
> The I2C_AQ_SKIP_ADDR_CHECK flag should probably not be propagated?
> 

Oh... you mean if we have a mux on an i2c adapter of a mux and the 
adapters handled by the parent mux have SKIP_ADDR set and we don't want 
the adapters handled by the leaf mux to have this flag as well? Is that 
what you meant?

Cheers,
Quentin