Message-ID: <dbe94425-8013-b866-9b6b-39ea499576e9@axentia.se>
Date: Mon, 7 Apr 2025 15:39:55 +0200
From: Peter Rosin <peda@...ntia.se>
To: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc: linux-kernel@...r.kernel.org, dmitry.baryshkov@....qualcomm.com
Subject: Re: [RFC PATCH] mux: core: add exclusive mux controls support



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.

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

Cheers,
Peter

Powered by blists - more mailing lists