Message-ID: <95b7eaf0-2040-a25a-4c41-ab86aba82a8d@axentia.se>
Date: Thu, 3 Apr 2025 22:58:51 +0200
From: Peter Rosin <peda@...ntia.se>
To: 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

Hi!

This one failed to reach my inbox for some reason? I only saw it by
"accident".

2025-03-26 at 16:46, srinivas.kandagatla@...aro.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
> 
> Mux control core by design supports mux controls that are shared with
> multiple consumers. However in some usecases where the mux is exclusively

use cases

> owned by one consumer do not need some of the locking and deselect apis.
> 
> exclusive apis makes the consumer side of code much simipler.

These exclusive APIs make the consumer side of the code much simpler.

> 
> ex:
> From
> 	if (is_mux_selected)
> 		mux_control_deselect()
> 
> 	if (mux_control_select())
> 		is_mux_selected = false;
> 	else
> 		is_mux_selected = true;
> 
> to
> 	if (mux_control_select())
> 		dev_err("mux select failed..");
> 
> This patch adds a new *_get_exclusive() api to request an exclusive mux
> control and rest of the apis usage remains same, except that exclusive
> mux do not need deselect api calling, drivers can simply select the
> desired state and its consumers responsiblity to make sure that correct
> state is selected.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
> ---
>  drivers/mux/core.c           | 123 ++++++++++++++++++++++++++++-------
>  include/linux/mux/consumer.h |   3 +
>  include/linux/mux/driver.h   |   9 +++
>  3 files changed, 113 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
> index 02be4ba37257..e0b8a723948b 100644
> --- a/drivers/mux/core.c
> +++ b/drivers/mux/core.c
> @@ -356,6 +356,10 @@ static void mux_control_delay(struct mux_control *mux, unsigned int delay_us)
>   * until mux_control_deselect() or mux_state_deselect() is called (by someone
>   * else).
>   *
> + * Exception to this is for exclusive mux control, which do not need
> + * mux_state_deselect() as the owner of mux has exclusive access to this mux
> + * and is responsible to set the correct state.
> + *
>   * Therefore, make sure to call mux_control_deselect() when the operation is
>   * complete and the mux-control is free for others to use, but do not call
>   * mux_control_deselect() if mux_control_select() fails.
> @@ -368,15 +372,17 @@ int mux_control_select_delay(struct mux_control *mux, unsigned int state,
>  {
>  	int ret;
>  
> -	ret = down_killable(&mux->lock);
> -	if (ret < 0)
> -		return ret;
> +	if (!mux->exclusive) {
> +		ret = down_killable(&mux->lock);
> +		if (ret < 0)
> +			return ret;
> +	}
>  
>  	ret = __mux_control_select(mux, state);
>  	if (ret >= 0)
>  		mux_control_delay(mux, delay_us);
>  
> -	if (ret < 0)
> +	if (!mux->exclusive && ret < 0)
>  		up(&mux->lock);
>  
>  	return ret;
> @@ -428,14 +434,16 @@ int mux_control_try_select_delay(struct mux_control *mux, unsigned int state,
>  {
>  	int ret;
>  
> -	if (down_trylock(&mux->lock))
> -		return -EBUSY;
> +	if (!mux->exclusive) {
> +		if (down_trylock(&mux->lock))
> +			return -EBUSY;
> +	}
>  
>  	ret = __mux_control_select(mux, state);
>  	if (ret >= 0)
>  		mux_control_delay(mux, delay_us);
>  
> -	if (ret < 0)
> +	if (!mux->exclusive && ret < 0)
>  		up(&mux->lock);
>  
>  	return ret;
> @@ -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.

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

Cheers,
Peter

>  }
>  
>  /**
> @@ -618,10 +650,33 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
>   */
>  struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>  {
> -	return mux_get(dev, mux_name, NULL);
> +	return mux_get(dev, mux_name, NULL, NORMAL_GET);
>  }
>  EXPORT_SYMBOL_GPL(mux_control_get);
>  
> +/**
> + * mux_control_get_exclusive() - Get the mux-control exclusive access for a device.
> + * @dev: The device that needs a exclusive mux-control.
> + * @mux_name: The name identifying the mux-control.
> + *
> + * Other consumers will be unable to obtain this mux-control while this
> + * reference is held and the use count for the mux-control will be
> + * initialised to reflect the current state of the mux-control.
> + *
> + * This is intended for use by consumers which do not need mux shared
> + * mux-control, and need exclusive control of mux.
> + * exclusive mux controls do not need mux_control_deselect() before
> + * selecting a mux state. Any mux state can be selected directly
> + * by calling mux_control_select() as long as state is supported.
> + *
> + * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
> + */
> +struct mux_control *mux_control_get_exclusive(struct device *dev, const char *mux_name)
> +{
> +	return mux_get(dev, mux_name, NULL, EXCLUSIVE_GET);
> +}
> +EXPORT_SYMBOL_GPL(mux_control_get_exclusive);
> +
>  /**
>   * mux_control_put() - Put away the mux-control for good.
>   * @mux: The mux-control to put away.
> @@ -630,6 +685,8 @@ EXPORT_SYMBOL_GPL(mux_control_get);
>   */
>  void mux_control_put(struct mux_control *mux)
>  {
> +	mux->open_count--;
> +	mux->exclusive = false;
>  	put_device(&mux->chip->dev);
>  }
>  EXPORT_SYMBOL_GPL(mux_control_put);
> @@ -641,16 +698,8 @@ static void devm_mux_control_release(struct device *dev, void *res)
>  	mux_control_put(mux);
>  }
>  
> -/**
> - * devm_mux_control_get() - Get the mux-control for a device, with resource
> - *			    management.
> - * @dev: The device that needs a mux-control.
> - * @mux_name: The name identifying the mux-control.
> - *
> - * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
> - */
> -struct mux_control *devm_mux_control_get(struct device *dev,
> -					 const char *mux_name)
> +static struct mux_control *__devm_mux_control_get(struct device *dev, const char *mux_name,
> +						  enum mux_control_get_type type)
>  {
>  	struct mux_control **ptr, *mux;
>  
> @@ -658,7 +707,10 @@ struct mux_control *devm_mux_control_get(struct device *dev,
>  	if (!ptr)
>  		return ERR_PTR(-ENOMEM);
>  
> -	mux = mux_control_get(dev, mux_name);
> +	if (type == EXCLUSIVE_GET)
> +		mux = mux_control_get_exclusive(dev, mux_name);
> +	else
> +		mux = mux_control_get(dev, mux_name);
>  	if (IS_ERR(mux)) {
>  		devres_free(ptr);
>  		return mux;
> @@ -669,8 +721,35 @@ struct mux_control *devm_mux_control_get(struct device *dev,
>  
>  	return mux;
>  }
> +
> +/**
> + * devm_mux_control_get() - Get the mux-control for a device, with resource
> + *			    management.
> + * @dev: The device that needs a mux-control.
> + * @mux_name: The name identifying the mux-control.
> + *
> + * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
> + */
> +struct mux_control *devm_mux_control_get(struct device *dev, const char *mux_name)
> +{
> +	return __devm_mux_control_get(dev, mux_name, NORMAL_GET);
> +}
>  EXPORT_SYMBOL_GPL(devm_mux_control_get);
>  
> +/**
> + * devm_mux_control_get_exclusive() - Get the mux-control exclusive for a device,
> + * 				 with resource management.
> + * @dev: The device that needs a mux-control.
> + * @mux_name: The name identifying the mux-control.
> + *
> + * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
> + */
> +struct mux_control *devm_mux_control_get_exclusive(struct device *dev, const char *mux_name)
> +{
> +	return __devm_mux_control_get(dev, mux_name, EXCLUSIVE_GET);
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_control_get_exclusive);
> +
>  /*
>   * mux_state_get() - Get the mux-state for a device.
>   * @dev: The device that needs a mux-state.
> @@ -686,7 +765,7 @@ static struct mux_state *mux_state_get(struct device *dev, const char *mux_name)
>  	if (!mstate)
>  		return ERR_PTR(-ENOMEM);
>  
> -	mstate->mux = mux_get(dev, mux_name, &mstate->state);
> +	mstate->mux = mux_get(dev, mux_name, &mstate->state, NORMAL_GET);
>  	if (IS_ERR(mstate->mux)) {
>  		int err = PTR_ERR(mstate->mux);
>  
> diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
> index 2e25c838f831..649b86c74bf3 100644
> --- a/include/linux/mux/consumer.h
> +++ b/include/linux/mux/consumer.h
> @@ -54,8 +54,11 @@ int mux_control_deselect(struct mux_control *mux);
>  int mux_state_deselect(struct mux_state *mstate);
>  
>  struct mux_control *mux_control_get(struct device *dev, const char *mux_name);
> +struct mux_control *mux_control_get_exclusive(struct device *dev, const char *mux_name);
>  void mux_control_put(struct mux_control *mux);
>  
> +struct mux_control *devm_mux_control_get_exclusive(struct device *dev,
> +					 const char *mux_name);
>  struct mux_control *devm_mux_control_get(struct device *dev,
>  					 const char *mux_name);
>  struct mux_state *devm_mux_state_get(struct device *dev,
> diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
> index 18824064f8c0..cda75b9b4775 100644
> --- a/include/linux/mux/driver.h
> +++ b/include/linux/mux/driver.h
> @@ -26,6 +26,12 @@ struct mux_control_ops {
>  	int (*set)(struct mux_control *mux, int state);
>  };
>  
> +enum mux_control_get_type {
> +	NORMAL_GET, /*  Shared */
> +	EXCLUSIVE_GET,
> +	MAX_GET_TYPE
> +};
> +
>  /**
>   * struct mux_control -	Represents a mux controller.
>   * @lock:		Protects the mux controller state.
> @@ -34,6 +40,7 @@ struct mux_control_ops {
>   * @states:		The number of mux controller states.
>   * @idle_state:		The mux controller state to use when inactive, or one
>   *			of MUX_IDLE_AS_IS and MUX_IDLE_DISCONNECT.
> + * @type:		Indicate type of mux control, Shared or Exclusive
>   * @last_change:	Timestamp of last change
>   *
>   * Mux drivers may only change @states and @idle_state, and may only do so
> @@ -50,6 +57,8 @@ struct mux_control {
>  	unsigned int states;
>  	int idle_state;
>  
> +	int open_count;
> +	bool exclusive;
>  	ktime_t last_change;
>  };
>  

Powered by blists - more mailing lists