Re: [PATCH v4 2/5] i2c: mux: add support for per channel bus frequency

[Peter Rosin]

Hi!

2026-02-12 at 22:47, Marcus Folkesson wrote:
>>> +static int i2c_mux_select_chan(struct i2c_adapter *adap, u32 chan_id)
>>> +{
>>> +	struct i2c_mux_priv *priv = adap->algo_data;
>>> +	struct i2c_mux_core *muxc = priv->muxc;
>>> +	struct i2c_adapter *parent = muxc->parent;
>>> +	struct i2c_adapter *root;
>>> +	int ret;
>>> +
>>> +	if (priv->adap.clock_hz && priv->adap.clock_hz != parent->clock_hz) {
>>> +		root = i2c_root_adapter(&adap->dev);
>>> +
>>> +		/* if we are parent-locked and the root adapter is our parent,
>>> +		 * we already have the lock we need. Otherwise take the bus lock for the root
>>> +		 * adapter before changing bus clock.
>>> +		 */
>>
>> The assumptions made for the "otherwise" case is wrong, I think.
> 
> I think you are right.
> 
>> Consider e.g. the case where we are parent-locket, our parent is
>> another parent-locked mux and our grand-parent is the root adapter.
>> In that case we also have all the locks we need. Trying to grab
>> them again will be a deadlock.
>>
>> The more correct approach is to do the parent walk in search of
>> the first ancestor mux that is not parent-locked and then call
> 
> Hrm, does it not apply for all mutex-locked ancestors?
> 
> Consider the following chain:
> Root - P1 - M1 - M2 - P2 - D1
> 
> P - Parent locked
> M - Mux locked
> D - Device
> 
> In this case we need to lock both M1 and M2, not just M2 ?
> I'm not completely sure though, I need to refresh myself on the code
> base.

No, that should not be needed. The reason is that when you initiate a
xfer for D1 the following happens (xfer is a locked transfer, __xfer
is unlocked):

- xfer using P2
 - lock(P2, I2C_LOCK_SEGMENT)
  + take mux_lock for P2->parent == M2
  + P2 is parent-locked -> recurse to P2->parent == M2
   + lock(M2, I2C_LOCK_SEGMENT)
    + take mux_lock for M2->parent == M1
    + M2 is mux-locked -> no recursion
 - ***** (see below)
 - P2->select (commonly __xfer using M2, elided here)
 - __xfer using M2 (unlocked since P2 is parent-locked)
  - §§§§§ (see below)
  - M2->select (commonly xfer using M1, elided here)
  - locked xfer using M1 (locked since M2 is mux-locked)
   - lock(M1, I2C_LOCK_SEGMENT)
    + take mux_lock for M1->parent == P1
    + M1 is mux-locked -> no recursion
   - M1->select (commonly xfer using P1, elided here)
   - xfer using P1 (locked since M1 is mux-locked)
    - lock(P1, I2C_LOCK_SEGMENT)
     + take mux_lock for P1->parent == Root
     + P1 is parent-locked -> recurse to P1->parent == Root
      + lock(Root, I2C_LOCK_SEGMENT)
       + take bus_lock for Root
       + Root is Root -> no recursion
    - P1->select (commonly __xfer using Root, elided here)

    - __xfer using Root (unlocked since P1 is parent-locked)

    - P1->deselect (commonly __xfer using Root, elided here)
    - unlock(P1, I2C_LOCK_SEGMENT)
     + P1 is parent-locked -> recurse to P1->parent == Root
      + unlock(Root, I2C_LOCK_SEGMENT)
       + Root is Root -> no recursion
       + release bus_lock for Root
     + release mux_lock for P1->parent == Root
   - M1->deselect (commonly xfer using P1, elided here)
   - unlock(M1, I2C_LOCK_SEGMENT)
    + M1 is mux-locked -> no recursion
    + release mux_unlock for M1->parent == P1
  - M2->deselect (commonly xfer using M1, elided here)
 - P2->deselect (commonly __xfer using M2, elided here)
 - unlock(P2, I2C_LOCK_SEGMENT)
  + P2 is parent-locked -> recurse to P2->parent == M2
   + unlock(M2, I2C_LOCK_SEGMENT)
    + M2 is mux-locked -> no recursion
    + release mux_lock for M2->parent == P2
  + release mux_lock for P2->parent == M2

(Phhew, I wonder how many typos are in there...)

So, between the steps lock(P2,...) and P2->select (at the ***** mark,
which is where you add set_clk_freq), what you need to lock is M1,
i.e. the parent of the first ancestor that is not mux-locked. When
you lock with I2C_LOCK_ROOT_ADAPTER, this happens:

 - lock(M1, I2C_LOCK_ROOT_ADAPTER)
  + take mux_lock for M1->parent == P1
  + recures to M1->parent == P1
   + lock(P1, I2C_LOCK_ROOT_ADAPTER)
    + take mux_lock for P1->parent == Root
    + recurse to P1->parent == Root
     + lock(Root, I2C_LOCK_ROOT_ADAPTER)
      + take bus_lock for Root
      + Root is Root -> no recursion
 - Root->set_clk_freq                  <<<< the new thing
 - unlock(M1, I2C_LOCK_ROOT_ADAPTER)
  + recurse to M1->parent == P1
   + unlock(P1, I2C_LOCK_ROOT_ADAPTER)
    + recurse to P1->parent == Root
     + unlock(Root, I2C_LOCK_ROOT_ADAPTER)
      + Root is Root, no recursion
      + release bus_lock for Root
    + release mux_lock for P1->parent == Root
  + release mux_lock for M1->parent == P1

However, spelling that out makes it clearer that Root->set_clk_freq
will be inserted in more places in the "call tree". It will be added
before every ->select call, e.g. at §§§§§. Since any of these
additional set_clk_freq calls happen after the one at *****, they will
take precedence and ruin the whole thing if any of them should request
an intermediate frequency (1MHz at Root, 400kHz for any intermediate
mux and 100kHz for D1, for example).

I don't immediately see how to reverse that such that the set_clk_freq
for the top-most level happens closest to the xfer on the root
adapter.

Cheers,
Peter