Re: [PATCH v2] mux: gpio-mux: add support for 4:1 2-channels mux

[Andrea Tomassetti <andrea.tomassetti@sipearl.com>]

Hi Srinivas,
thank you for your thorough review.

On 6/19/26 19:23, Srinivas Kandagatla wrote:
> "mux: gpio-mux: add support for multiple mux controls" would be more
> reflective of what is done in  the patch.
>
Noted, I will update it in v3.
>>
>> -	mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_gpio));
>> +	ret = device_property_read_u32(dev, "#mux-control-cells", &cells);
>> +	if (ret < 0)
>> +		cells = 0;
>> +
>> +	if (cells >= 2) {
>
> This code snippet is duplicated once again, may its time for this to go
> to as helper function, either in core or somewhere common.
>
What were you envisioning? A simple macro in mux/driver.h? Or more a proper
function in drivers/mux/core.c?

>> +		dev_err(dev, "invalid control-cells %u, must be 0 or 1\n", cells);
>
>  mux-control-cells would give useful message to user rather than just
> control-cells.
>
Noted, I will update it in v3.

>> +		return -EINVAL;
>> +	}
>> +
>> +	mux_chip = devm_mux_chip_alloc(dev, cells + 1, sizeof(*mux_gpio));
>
> We are now allocating n mux_controls, however mux_gpio_set is not
> protected against multiple controllers accessing the same gpio resource.
>
>
This is a great point. I see two different type of "race conditions" here:
    1. serialize accesses in mux_gpio_set
    2. "logical race condition" due to how the upper locking mechanism is
handled by mux/core.c

The first issue can be easily solved by adding a mutex inside struct mux_gpio
and use it to serialize accesses in mux_gpio_set.

OTOH, the second issue is more nasty and depends on how the upper locking
mechanism is handled by mux/core.c. Please, correct me if I'm wrong, but to me
it looks like that the core's locking model assumes each mux_control is an
independently lockable resource. While this is generally true, it doesn't apply
in the case when there's only one controller that jointly controls two or more
gpio-muxes. Like in this case.

So, adding a mutex at gpio-mux level would fix the race condition of two
consumers changing the state at the same time (issue 1). But, to my
understanding, it won't fix issue 2: mutual exclusion on the write makes each
write atomic, but consumer A's selection can still be immediately hijacked by
consumer B's, leaving A believing it is reading what it just set when it's
actually reading whatever B last selected. This happens because the lock used in
mux/core.c is allocated _per controller_ and not _per chip_. Do you agree with
my analysis?

If you agree, I can fix issue 1 but I'd lean towards treating issue 2 as out of
scope for this patch. Let me know if you see it differently.

Br,
Andrea