Re: [RFC PATCH] mux: core: add exclusive mux controls support

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

On 07/04/2025 14:39, Peter Rosin wrote:
> 
> 
> 2025-04-07 at 12:36, Srinivas Kandagatla wrote:
>>
>> On 03/04/2025 21:58, Peter Rosin wrote:
>>>> @@ -479,6 +487,10 @@ int mux_control_deselect(struct mux_control *mux)
>>>>    {
>>>>        int ret = 0;
>>>>    +    /* exclusive mux control do not deselection */
>>>> +    if (mux->exclusive)
>>>> +        return -EINVAL;
>>> This is unfortunate. I think there is value in being able to deselect
>>> muxes in exclusive mode. Otherwise you might need to keep track of
>>> some idle-state in the code, when it would be more flexible to have
>>> it specified in e.g. the DT node. The best idle-state may very well
>>> be hardware dependent, and is often not some central thing for the
>>> consumer driver.
>>
>> Does it mean exclusive mux can deselect a mux however its not mandatory to deslect between each mux selects?
> 
> Yes, that was what I tried to say.
> 
>>
>>>
>>>> +
>>>>        if (mux->idle_state != MUX_IDLE_AS_IS &&
>>>>            mux->idle_state != mux->cached_state)
>>>>            ret = mux_control_set(mux, mux->idle_state);
>>>> @@ -523,13 +535,15 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
>>>>     * @mux_name: The name identifying the mux-control.
>>>>     * @state: Pointer to where the requested state is returned, or NULL when
>>>>     *         the required multiplexer states are handled by other means.
>>>> + * @get_type: Type of mux get, shared or exclusive
>>>>     *
>>>>     * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
>>>>     */
>>>>    static struct mux_control *mux_get(struct device *dev, const char *mux_name,
>>>> -                   unsigned int *state)
>>>> +                   unsigned int *state, enum mux_control_get_type get_type)
>>>>    {
>>>>        struct device_node *np = dev->of_node;
>>>> +    struct mux_control *mux_ctrl;
>>>>        struct of_phandle_args args;
>>>>        struct mux_chip *mux_chip;
>>>>        unsigned int controller;
>>>> @@ -606,7 +620,25 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
>>>>            return ERR_PTR(-EINVAL);
>>>>        }
>>>>    -    return &mux_chip->mux[controller];
>>>> +    mux_ctrl = &mux_chip->mux[controller];
>>>> +
>>>> +    if (mux_ctrl->exclusive) {
>>>> +        mux_ctrl = ERR_PTR(-EPERM);
>>>> +        put_device(&mux_chip->dev);
>>>> +        return mux_ctrl;
>>>> +    }
>>>> +
>>>> +    if (get_type == EXCLUSIVE_GET && mux_ctrl->open_count) {
>>>> +        mux_ctrl = ERR_PTR(-EBUSY);
>>>> +        put_device(&mux_chip->dev);
>>>> +        return mux_ctrl;
>>>> +    }
>>>> +
>>>> +    mux_ctrl->open_count++;
>>>> +    if (get_type == EXCLUSIVE_GET)
>>>> +        mux_ctrl->exclusive = true;
>>>> +
>>>> +    return mux_ctrl;
>>> This is racy with no guarantee that you are the only consumer after you
>>
>> Yes, there is a chance of race here. locking around mux_chip access should help fix this race.
> 
> Yes, some locking is indeed needed.
> 
>>> have gotten an exclusive mux. My sketchy vision was to have an API
>>> function that requests an ordinary shared mux to be exclusive, and then
>>> another to make the mux shared again. Those would take/release the same
>>
>> hm,  dynamically going between shared to exclusive is going bring more challenges and consumer side its going to be more complicated.
> 
> It is not important to be able to toggle between shared and exclusive.
> So, if that's difficult, don't do it. But if it somehow makes the
> implementation neater, it's certainly a possibility.
> 
>>
>> My idea to do this way was to allow more flags like optional to mux, in same likes  regulators or resets.. etc.
> 
> Yes, there are uses for optional muxes.
I will try to add these flags in v2.

> 
>>> lock as when selecting/deselecting, but also mark the mux as exclusive
>>> and trigger_not_  taking/releasing the lock in select/deselect.
>>>
>>> But then we have the little thing that conditional locking is not
>>> exactly ideal. Which is why I haven't done this before. I simply never
>>> got it to a point where I felt it was good enough...
>>>
>>> Another reason for me not having done it is that I also feel that it
>>> might not be ideal to reuse mux_control_select and mux_control_deselect
>>> at all since the rules for using those when the mux is shared are ...
>>> a bit difficult, and will remain that way. Thus, having those functions
>>> *sometimes*  behave like they are easy and sometimes requiring great
>>> detail will make the already bad shared case even more error prone.
>>>
>>> I wish I could see how to do this sanely.
>>
>> How about going with current approach with locking as suggested?
> 
> There needs to be operations to
> - acquire an exclusive mux
> - set the state of an exclusive mux
> - return an exclusive mux to its idle state
> - release an exclusive mux

> 
> Only adding the locking to kill races in acquire/release will still
> have the problem with reusing the same API for setting the state of
> an exclusive mux and selecting a state from a shared mux. And as I
> said, I think it's bad to try to keep those seemingly similar ops
> as the same APIs. I think it would be better to add mux_control_set()
> and mux_control_unset() APIs (naming?) and have those error out if
> tried with a shared mux. In the same vein, mux_control_select() and
> mux_control_deselect() should error out if tried with an exclusive
> mux.

Yes, having dedicated apis can make it bit more explicit to the consumer 
side.
I will try that in v2.

thanks,
Srini
> 
> Cheers,
> Peter