Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1629426 > unrolled thread

Re: [PATCH v14 00/11] mux controller abstraction and iio/i2c muxes

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v14 00/11] mux controller abstraction and iio/i2c muxes Philipp Zabel <p.zabel@pengutronix.de> - 2017-04-24 13:00 +0200
    Re: [PATCH v14 00/11] mux controller abstraction and iio/i2c muxes Philipp Zabel <p.zabel@pengutronix.de> - 2017-04-24 14:40 +0200
    Re: [PATCH v14 00/11] mux controller abstraction and iio/i2c muxes Philipp Zabel <p.zabel@pengutronix.de> - 2017-04-24 16:20 +0200
      Re: [PATCH v14 00/11] mux controller abstraction and iio/i2c muxes Philipp Zabel <p.zabel@pengutronix.de> - 2017-04-24 17:00 +0200

#1629426 — Re: [PATCH v14 00/11] mux controller abstraction and iio/i2c muxes

On Mon, 2017-04-24 at 10:36 +0200, Peter Rosin wrote:
> Hi!
> 
> The big change since v13 is that the mux state is now locked with a mutex
> instead of an rwsem. Other that that, it is mostly restructuring and doc
> changes. There are a few other "real" changes as well, but those changes
> feel kind of minor. I guess what I'm trying to say is that although the
> list of changes for v14 is longish, it's still basically the same as last
> time.

I have hooked this up to the video-multiplexer and now I trigger
the lockdep debug_check_no_locks_held error message when selecting the
mux input from userspace:

$ media-ctl --links "'imx6-mipi-csi2':1->'ipu1_csi0_mux':0[1]"
[   66.258368] 
[   66.259919] =====================================
[   66.265369] [ BUG: media-ctl/258 still has locks held! ]
[   66.270810] 4.11.0-rc8-20170424-1+ #1305 Not tainted
[   66.275863] -------------------------------------
[   66.282158] 1 lock held by media-ctl/258:
[   66.286464]  #0:  (&mux->lock){+.+.+.}, at: [<8074a6c0>] mux_control_select+0x24/0x50
[   66.294424] 
[   66.294424] stack backtrace:
[   66.298980] CPU: 0 PID: 258 Comm: media-ctl Not tainted 4.11.0-rc8+ #1305
[   66.306771] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[   66.313334] Backtrace: 
[   66.315858] [<8010e5a4>] (dump_backtrace) from [<8010e8ac>] (show_stack+0x20/0x24)
[   66.323473]  r7:80e518d0 r6:80e518d0 r5:600d0013 r4:00000000
[   66.329190] [<8010e88c>] (show_stack) from [<80496cf4>] (dump_stack+0xb0/0xdc)
[   66.336470] [<80496c44>] (dump_stack) from [<8017e404>] (debug_check_no_locks_held+0xb8/0xbc)
[   66.345043]  r9:ae8566b8 r8:ad88dc84 r7:ad88df58 r6:ad88dc84 r5:ad88df58 r4:ae856400
[   66.352837] [<8017e34c>] (debug_check_no_locks_held) from [<8012b258>] (do_exit+0x79c/0xcc8)
[   66.361321] [<8012aabc>] (do_exit) from [<8012d25c>] (do_group_exit+0x4c/0xcc)
[   66.368581]  r7:000000f8
[   66.371161] [<8012d210>] (do_group_exit) from [<8012d2fc>] (__wake_up_parent+0x0/0x30)
[   66.379120]  r7:000000f8 r6:76f71798 r5:00000000 r4:00000001
[   66.384837] [<8012d2dc>] (SyS_exit_group) from [<80109380>] (ret_fast_syscall+0x0/0x1c)

That correctly warns that the media-ctl process caused the mux->lock to
be locked and still held when the process exited. Do we need a usage
counter based mechanism for muxes that are (indirectly) controlled from
userspace?

regards
Philipp

[toc] | [next] | [standalone]

#1629484

On Mon, 2017-04-24 at 13:37 +0200, Peter Rosin wrote:
[...]
> Ok, so the difference is probably that the rwsem locking primitive
> don't have any lockdep checking hooked up. Because the rwsem was
> definitely held in the same way in v13 as the mutex is now held in
> v14, so there's no fundamental difference.
>
> So, yes, we can use some kind of refcount scheme to not hold an actual
> mutex for the duration of the mux select/deselect, but that doesn't
> really change anything. Userspace is still locking something, and that
> seems dangerous. How do you make sure that mux_control_deselect is
> called as it should?

My current video_mux link setup implementation looks like this (it is
called from userspace via the MEDIA_IOC_SETUP_LINK ioctl):

----------8<----------
static int video_mux_link_setup(struct media_entity *entity,
				const struct media_pad *local,
				const struct media_pad *remote, u32 flags)
{
	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
	struct video_mux *video_mux = v4l2_subdev_to_video_mux(sd);
	int ret;

	/*
	 * The mux state is determined by the enabled sink pad link.
	 * Enabling or disabling the source pad link has no effect.
	 */
	if (!is_sink_pad(video_mux, local->index))
		return 0;

	dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]", remote->entity->name,
		remote->index, local->entity->name, local->index,
		flags & MEDIA_LNK_FL_ENABLED);

	if (flags & MEDIA_LNK_FL_ENABLED) {
		if (video_mux->active == local->index)
			return 0;

		if (video_mux->active >= 0)
			return -EBUSY;

		dev_dbg(sd->dev, "setting %d active\n", local->index);
		ret = mux_control_try_select(video_mux->mux, local->index);
		if (ret < 0)
			return ret;
		video_mux->active = local->index;
	} else {
		if (video_mux->active != local->index)
			return 0;

		dev_dbg(sd->dev, "going inactive\n");
		mux_control_deselect(video_mux->mux);
		video_mux->active = -1;
	}

	return 0;
}
---------->8----------

If a mux state was already selected, this should return -EBUSY, until a
call to disable the active link deselects the mux.

> What I don't like about abandoning the lock is that there is still a
> need to support the multi-consumer case and then you need some way
> of keeping track of waiters. A lock does this, and any attempt to open
> code that will get messy.
> 
> What might be better is to support some kind of exclusive mux, i.e. a
> mux that only allows one consumer per mux controller. Then the mux core
> could trust that exclusive consumer to not fuck things up for itself and
> thus have no lock at all for select/deselect for the exclusive case. Or
> perhaps keep a refcount (as you suggested) for the exclusive case so
> that mux_control_try_select still makes sense, because you still want
> that, right? 

In the case of an exclusive mux without lock, I don't see any need for a
try_select call.

> The question then becomes how to best tell the mux core that you want
> an exclusive mux. I see two options. Either you declare a mux controller
> as exclusive up front somehow (in the device tree presumably), or you
> add a mux_control_get_exclusive call that makes further calls to
> mux_control_get{,_exclusive} fail with -EBUSY. I think I like the
> latter better, if that can be made to work...

There is a precedent for the latter in the reset controller framework
(reset_control_get_shared and reset_control_get_exclusive variants).
Since this distinction is a matter of usage, and not a hardware property
of the mux/reset controller itself, I'd also prefer that.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1629607

On Mon, 2017-04-24 at 13:37 +0200, Peter Rosin wrote:
> On 2017-04-24 12:52, Philipp Zabel wrote:
> > On Mon, 2017-04-24 at 10:36 +0200, Peter Rosin wrote:
> >> Hi!
> >>
> >> The big change since v13 is that the mux state is now locked with a mutex
> >> instead of an rwsem. Other that that, it is mostly restructuring and doc
> >> changes. There are a few other "real" changes as well, but those changes
> >> feel kind of minor. I guess what I'm trying to say is that although the
> >> list of changes for v14 is longish, it's still basically the same as last
> >> time.
> > 
> > I have hooked this up to the video-multiplexer and now I trigger
> > the lockdep debug_check_no_locks_held error message when selecting the
> > mux input from userspace:
> > 
> > $ media-ctl --links "'imx6-mipi-csi2':1->'ipu1_csi0_mux':0[1]"
> > [   66.258368] 
> > [   66.259919] =====================================
> > [   66.265369] [ BUG: media-ctl/258 still has locks held! ]
> > [   66.270810] 4.11.0-rc8-20170424-1+ #1305 Not tainted
> > [   66.275863] -------------------------------------
> > [   66.282158] 1 lock held by media-ctl/258:
> > [   66.286464]  #0:  (&mux->lock){+.+.+.}, at: [<8074a6c0>] mux_control_select+0x24/0x50
> > [   66.294424] 
> > [   66.294424] stack backtrace:
> > [   66.298980] CPU: 0 PID: 258 Comm: media-ctl Not tainted 4.11.0-rc8+ #1305
> > [   66.306771] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> > [   66.313334] Backtrace: 
> > [   66.315858] [<8010e5a4>] (dump_backtrace) from [<8010e8ac>] (show_stack+0x20/0x24)
> > [   66.323473]  r7:80e518d0 r6:80e518d0 r5:600d0013 r4:00000000
> > [   66.329190] [<8010e88c>] (show_stack) from [<80496cf4>] (dump_stack+0xb0/0xdc)
> > [   66.336470] [<80496c44>] (dump_stack) from [<8017e404>] (debug_check_no_locks_held+0xb8/0xbc)
> > [   66.345043]  r9:ae8566b8 r8:ad88dc84 r7:ad88df58 r6:ad88dc84 r5:ad88df58 r4:ae856400
> > [   66.352837] [<8017e34c>] (debug_check_no_locks_held) from [<8012b258>] (do_exit+0x79c/0xcc8)
> > [   66.361321] [<8012aabc>] (do_exit) from [<8012d25c>] (do_group_exit+0x4c/0xcc)
> > [   66.368581]  r7:000000f8
> > [   66.371161] [<8012d210>] (do_group_exit) from [<8012d2fc>] (__wake_up_parent+0x0/0x30)
> > [   66.379120]  r7:000000f8 r6:76f71798 r5:00000000 r4:00000001
> > [   66.384837] [<8012d2dc>] (SyS_exit_group) from [<80109380>] (ret_fast_syscall+0x0/0x1c)
> > 
> > That correctly warns that the media-ctl process caused the mux->lock to
> > be locked and still held when the process exited. Do we need a usage
> > counter based mechanism for muxes that are (indirectly) controlled from
> > userspace?
[...]
> The question then becomes how to best tell the mux core that you want
> an exclusive mux. I see two options. Either you declare a mux controller
> as exclusive up front somehow (in the device tree presumably), or you
> add a mux_control_get_exclusive call that makes further calls to
> mux_control_get{,_exclusive} fail with -EBUSY. I think I like the
> latter better, if that can be made to work...

How about an atomic use_count on the mux_control, a bool shared that is
only set by the first consumer, and controls whether selecting locks?

----------8<----------
diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
index c02fa4dd2d099..1d18bb7f15e07 100644
--- a/drivers/mux/mux-core.c
+++ b/drivers/mux/mux-core.c
@@ -372,11 +372,12 @@ int mux_control_select(struct mux_control *mux, unsigned int state)
 {
 	int ret;
 
-	mutex_lock(&mux->lock);
+	if (mux->shared)
+		mutex_lock(&mux->lock);
 
 	ret = __mux_control_select(mux, state);
 
-	if (ret < 0)
+	if (ret < 0 && mux->shared)
 		mutex_unlock(&mux->lock);
 
 	return ret;
@@ -399,12 +400,12 @@ int mux_control_try_select(struct mux_control *mux, unsigned int state)
 {
 	int ret;
 
-	if (!mutex_trylock(&mux->lock))
+	if (mux->shared && !mutex_trylock(&mux->lock))
 		return -EBUSY;
 
 	ret = __mux_control_select(mux, state);
 
-	if (ret < 0)
+	if (ret < 0 && mux->shared)
 		mutex_unlock(&mux->lock);
 
 	return ret;
@@ -427,7 +428,8 @@ int mux_control_deselect(struct mux_control *mux)
 	    mux->idle_state != mux->cached_state)
 		ret = mux_control_set(mux, mux->idle_state);
 
-	mutex_unlock(&mux->lock);
+	if (mux->shared)
+		mutex_unlock(&mux->lock);
 
 	return ret;
 }
@@ -447,18 +449,13 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
 	return dev ? to_mux_chip(dev) : NULL;
 }
 
-/**
- * mux_control_get() - Get the mux-control for a device.
- * @dev: The device that needs a mux-control.
- * @mux_name: The name identifying the mux-control.
- *
- * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
- */
-struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
+static struct mux_control *__mux_control_get(struct device *dev,
+					     const char *mux_name, bool shared)
 {
 	struct device_node *np = dev->of_node;
 	struct of_phandle_args args;
 	struct mux_chip *mux_chip;
+	struct mux_control *mux;
 	unsigned int controller;
 	int index = 0;
 	int ret;
@@ -505,9 +502,56 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
 	}
 
 	get_device(&mux_chip->dev);
-	return &mux_chip->mux[controller];
+
+	mux = &mux_chip->mux[controller];
+
+	ret = atomic_inc_return(&mux->use_count);
+	if (ret == 1 && shared)
+		mux->shared = true;
+	if (ret > 1 && (!shared || !mux->shared)) {
+		atomic_dec(&mux->use_count);
+		put_device(&mux_chip->dev);
+		return ERR_PTR(-EBUSY);
+	}
+
+	return mux;
+}
+
+/**
+ * mux_control_get_shared() - Get the mux-control for a device.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * The returned mux control is in shared mode and can not be controlled from
+ * userspace. The mux control lock is taken when the mux is selected and
+ * released when the mux is deselected.
+ *
+ * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
+ */
+struct mux_control *mux_control_get_shared(struct device *dev,
+					   const char *mux_name)
+{
+	return __mux_control_get(dev, mux_name, true);
+}
+EXPORT_SYMBOL_GPL(mux_control_get_shared);
+
+/**
+ * mux_control_get_exclusive() - Get the mux-control for a device.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * The returned mux control is in exclusive mode and does not lock the mux
+ * control lock when the mux is selected. The exclusive consumer has to take
+ * care of locking, if necessary.
+ *
+ * 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_control_get(dev, mux_name, false);
 }
-EXPORT_SYMBOL_GPL(mux_control_get);
+EXPORT_SYMBOL_GPL(mux_control_get_exclusive);
 
 /**
  * mux_control_put() - Put away the mux-control for good.
@@ -517,6 +561,7 @@ EXPORT_SYMBOL_GPL(mux_control_get);
  */
 void mux_control_put(struct mux_control *mux)
 {
+	atomic_dec(&mux->use_count);
 	put_device(&mux->chip->dev);
 }
 EXPORT_SYMBOL_GPL(mux_control_put);
@@ -528,16 +573,9 @@ 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,
+						  bool shared)
 {
 	struct mux_control **ptr, *mux;
 
@@ -545,7 +583,7 @@ struct mux_control *devm_mux_control_get(struct device *dev,
 	if (!ptr)
 		return ERR_PTR(-ENOMEM);
 
-	mux = mux_control_get(dev, mux_name);
+	mux = __mux_control_get(dev, mux_name, shared);
 	if (IS_ERR(mux)) {
 		devres_free(ptr);
 		return mux;
@@ -556,7 +594,36 @@ struct mux_control *devm_mux_control_get(struct device *dev,
 
 	return mux;
 }
-EXPORT_SYMBOL_GPL(devm_mux_control_get);
+
+/**
+ * devm_mux_control_get_shared() - 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_shared(struct device *dev,
+						const char *mux_name)
+{
+	return __devm_mux_control_get(dev, mux_name, true);
+}
+EXPORT_SYMBOL_GPL(devm_mux_control_get_shared);
+
+/**
+ * devm_mux_control_get_exclusive() - 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_exclusive(struct device *dev,
+						   const char *mux_name)
+{
+	return __devm_mux_control_get(dev, mux_name, false);
+}
+EXPORT_SYMBOL_GPL(devm_mux_control_get_exclusive);
 
 static int devm_mux_control_match(struct device *dev, void *res, void *data)
 {
diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
index a2a61834bd3ae..26634c3dda124 100644
--- a/include/linux/mux/consumer.h
+++ b/include/linux/mux/consumer.h
@@ -23,11 +23,16 @@ int __must_check mux_control_try_select(struct mux_control *mux,
 					unsigned int state);
 int mux_control_deselect(struct mux_control *mux);
 
-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);
+struct mux_control *mux_control_get_shared(struct device *dev,
+					   const char *mux_name);
 void mux_control_put(struct mux_control *mux);
 
-struct mux_control *devm_mux_control_get(struct device *dev,
-					 const char *mux_name);
+struct mux_control *devm_mux_control_get_exclusive(struct device *dev,
+						   const char *mux_name);
+struct mux_control *devm_mux_control_get_shared(struct device *dev,
+						const char *mux_name);
 void devm_mux_control_put(struct device *dev, struct mux_control *mux);
 
 #endif /* _LINUX_MUX_CONSUMER_H */
diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
index 95269f40670a7..0ac5d3db9cd3e 100644
--- a/include/linux/mux/driver.h
+++ b/include/linux/mux/driver.h
@@ -31,17 +31,20 @@ struct mux_control_ops {
 
 /**
  * struct mux_control -	Represents a mux controller.
- * @lock:		Protects the mux controller state.
+ * @lock:		Protects the mux controller state, if shared.
  * @chip:		The mux chip that is handling this mux controller.
  * @cached_state:	The current mux controller state, or -1 if none.
  * @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.
+ * @use_count:		Number of consumers that have requested this mux.
+ * @shared:		True if the mux control was requested as shared and
+ *			must therefore be locked.
  *
  * Mux drivers may only change @states and @idle_state, and may only do so
  * between allocation and registration of the mux controller. Specifically,
- * @cached_state is internal to the mux core and should never be written by
- * mux drivers.
+ * @cached_state, @use_count, and @shared are internal to the mux core and
+ * should never be written by mux drivers.
  */
 struct mux_control {
 	struct mutex lock; /* protects the state of the mux */
@@ -51,6 +54,9 @@ struct mux_control {
 
 	unsigned int states;
 	int idle_state;
+
+	atomic_t use_count;
+	bool shared;
 };
 
 /**
---------->8----------

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1629651

On Mon, 2017-04-24 at 16:36 +0200, Peter Rosin wrote:
[...]
> > How about an atomic use_count on the mux_control, a bool shared that is
> > only set by the first consumer, and controls whether selecting locks?
> 
> That has the drawback that it is hard to restore the mux-control in a safe
> way so that exclusive consumers are allowed after the last shared consumer
> puts the mux away.

True.

> Agreed, it's a corner case, but I had this very similar
> patch going through the compiler when I got this mail. Does it work as well
> as what you suggested?

Yes, this patch works just as well.

regards
Philipp

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web