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

[Marcus Folkesson <marcus.folkesson@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]

Hi Peter!

Thank you so much for your thoughs!

On Thu, Feb 12, 2026 at 04:02:16PM +0100, Peter Rosin wrote:
> Hi!
> 

[...]

> > +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.

> 
> 	i2c_lock_bus(ancestor->parent, I2C_LOCK_ROOT_ADAPTER);
> 
> for it.
> 
> Again, I think. Famous last words. Handwaving...
> 
> Cheers,
> Peter


Best regards,
Marcus Folkesson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]