drivers/mux/core.c | 123 ++++++++++++++++++++++++++++------- include/linux/mux/consumer.h | 3 + include/linux/mux/driver.h | 9 +++ 3 files changed, 113 insertions(+), 22 deletions(-)
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Mux control core by design supports mux controls that are shared with
multiple consumers. However in some usecases where the mux is exclusively
owned by one consumer do not need some of the locking and deselect apis.
exclusive apis makes the consumer side of code much simipler.
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@linaro.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;
+
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;
}
/**
@@ -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;
};
--
2.39.5Hi!
This one failed to reach my inbox for some reason? I only saw it by
"accident".
2025-03-26 at 16:46, srinivas.kandagatla@linaro.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.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@linaro.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;
> };
>
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?
>
>> +
>> 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.
> 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.
My idea to do this way was to allow more flags like optional to mux, in
same likes regulators or resets.. etc.
> 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?
thanks,
Srini
>
> Cheers,
> Peter
>
>> }
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
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
© 2016 - 2025 Red Hat, Inc.