[PATCH 0/4] Add managed token devres interfaces and change media drivers to use it

[PATCH 0/4] Add managed token devres interfaces and change media drivers to use it

[Shuah Khan]

Media drivers that control a single media TV stick are a
diversified group. Analog and digital TV function drivers have
to coordinate access to their shared functions. In some cases,
snd-usb-audio is used to support audio function on media devices.

A shared managed resource framework at drivers/base level will
allow a media device to be controlled by drivers that don't
fall under drivers/media and share functions with other media
drivers.

A token devres that can be looked up by a token for locking, try
locking, unlocking will help avoid adding data structure
dependencies between various drivers. This token is a unique
string that can be constructed from a common data structure such as
struct device, bus_name, and hardware address.

This patch series adds devm_token_* interfaces to manage access to
token resource and a tuner token to allow sharing tuner function
across analog and digital functions. em28xx and dvb-core make
use of this new tuner token to control tuner access. Analog changes
will be added in a subsequent patch series.

Patch series is tested with Kworld UB435-Q V3 USB TV stick and
Kaffeine media player.

Shuah Khan (4):
  drivers/base: add managed token devres interfaces
  media: dvb-fe changes to use token for shared access control
  media/em28xx: changes to create token for tuner access
  media: em28xx dvb changes to initialze dvb fe tuner token

 drivers/base/Makefile                   |    2 +-
 drivers/base/token_devres.c             |  146 +++++++++++++++++++++++++++++++
 drivers/media/dvb-core/dvb_frontend.c   |   15 ++++
 drivers/media/dvb-core/dvb_frontend.h   |    1 +
 drivers/media/usb/em28xx/em28xx-cards.c |   41 +++++++++
 drivers/media/usb/em28xx/em28xx-dvb.c   |    4 +
 drivers/media/usb/em28xx/em28xx.h       |    4 +
 include/linux/token_devres.h            |   19 ++++
 8 files changed, 231 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/token_devres.c
 create mode 100644 include/linux/token_devres.h

-- 
1.7.10.4

[PATCH 1/4] drivers/base: add managed token devres interfaces

[Shuah Khan]

Media devices often have hardware resources that are shared
across several functions. These devices appear as a group of
independent devices. Each device implements a function which
could be shared by one or more functions supported by the same
device. For example, tuner is shared by analog and digital TV
functions.

Media drivers that control a single media TV stick are a
diversified group. Analog and digital TV function drivers have
to coordinate access to their shared functions.

Some media devices provide multiple almost-independent functions.
USB and PCI core aids in allowwing multiple drivers to handle these
almost-independent functions. In this model, a em28xx device could
have snd-usb-audio driving the audio function.

As a result, snd-usb-audio driver has to coordinate with the em28xx_*
analog and digital function drivers.

A shared managed resource framework at drivers/base level will
allow a media device to be controlled by drivers that don't
fall under drivers/media and share functions with other media
drivers.

A token devres that can be looked up by a token for locking, try
locking, unlocking will help avoid adding data structure
dependencies between various drivers. This token is a unique
string that can be constructed from a common data structure such as
struct device, bus_name, and hardware address.

The devm_token_* interfaces manage access to token resource.

Interfaces:
    devm_token_create()
    devm_token_destroy()
    devm_token_lock()
    devm_token_unlock()
Usage:
    Create token: Call devm_token_create() with a token id string.
    Lock token: Call devm_token_lock() to lock or try lock a token.
    Unlock token: Call devm_token_unlock().
    Destroy token: Call devm_token_destroy() to delete the token.

Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
---
 drivers/base/Makefile        |    2 +-
 drivers/base/token_devres.c  |  146 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/token_devres.h |   19 ++++++
 3 files changed, 166 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/token_devres.c
 create mode 100644 include/linux/token_devres.h

diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 04b314e..924665b 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -4,7 +4,7 @@ obj-y			:= component.o core.o bus.o dd.o syscore.o \
 			   driver.o class.o platform.o \
 			   cpu.o firmware.o init.o map.o devres.o \
 			   attribute_container.o transport_class.o \
-			   topology.o container.o
+			   topology.o container.o token_devres.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y			+= power/
diff --git a/drivers/base/token_devres.c b/drivers/base/token_devres.c
new file mode 100644
index 0000000..4644642
--- /dev/null
+++ b/drivers/base/token_devres.c
@@ -0,0 +1,146 @@
+/*
+ * drivers/base/token_devres.c - managed token resource
+ *
+ * Copyright (c) 2014 Shuah Khan <shuah.kh@samsung.com>
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ *
+ * This file is released under the GPLv2.
+ */
+/*
+ * Media devices often have hardware resources that are shared
+ * across several functions. For instance, TV tuner cards often
+ * have MUXes, converters, radios, tuners, etc. that are shared
+ * across various functions. However, v4l2, alsa, DVB, usbfs, and
+ * all other drivers have no knowledge of what resources are
+ * shared. For example, users can't access DVB and alsa at the same
+ * time, or the DVB and V4L analog API at the same time, since many
+ * only have one converter that can be in either analog or digital
+ * mode. Accessing and/or changing mode of a converter while it is
+ * in use by another function results in video stream error.
+ *
+ * A shared devres that can be locked and unlocked by various drivers
+ * that control media functions on a single media device is needed to
+ * address the above problems.
+ *
+ * A token devres that can be looked up by a token for locking, try
+ * locking, unlocking will help avoid adding data structure
+ * dependencies between various media drivers. This token is a unique
+ * string that can be constructed from a common data structure such as
+ * struct device, bus_name, and hardware address.
+ *
+ * The devm_token_* interfaces manage access to token resource.
+ *
+ * Interfaces:
+ *	devm_token_create()
+ *	devm_token_destroy()
+ *	devm_token_lock()
+ *	devm_token_unlock()
+ * Usage:
+ *	Create token:
+ *		Call devm_token_create() with a token id which is
+ *		a unique string.
+ *	Lock token:
+ *		Call devm_token_lock() to lock or try lock a token.
+ *	Unlock token:
+ *		Call devm_token_unlock().
+ *	Destroy token:
+ *		Call devm_token_destroy() to delete the token.
+ *
+*/
+#include <linux/device.h>
+#include <linux/token_devres.h>
+
+struct token_devres {
+	struct mutex	lock;
+	bool		in_use;
+	char		id[];
+};
+
+static int devm_token_match(struct device *dev, void *res, void *data)
+{
+	struct token_devres *tkn = res;
+
+	/* compare the token data and return 1 if it matches */
+	return !strcmp(tkn->id, data);
+}
+
+static void devm_token_release(struct device *dev, void *res)
+{
+	struct token_devres *tkn = res;
+
+	mutex_destroy(&tkn->lock);
+}
+
+/* creates a token devres and marks it available */
+int devm_token_create(struct device *dev, const char *id)
+{
+	struct token_devres *tkn;
+	size_t tkn_size;
+
+	tkn_size = sizeof(struct token_devres) + strlen(id) + 1;
+	tkn = devres_alloc(devm_token_release, tkn_size, GFP_KERNEL);
+	if (!tkn)
+		return -ENOMEM;
+
+	strcpy(tkn->id, id);
+	tkn->in_use = false;
+	mutex_init(&tkn->lock);
+
+	devres_add(dev, tkn);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_token_create);
+
+/* If token is available, lock it for the caller, If not return -EBUSY */
+int devm_token_lock(struct device *dev, const char *id)
+{
+	struct token_devres *tkn_ptr;
+	int rc = 0;
+
+	tkn_ptr = devres_find(dev, devm_token_release, devm_token_match,
+				(void *)id);
+	if (tkn_ptr == NULL)
+		return -ENODEV;
+
+	if (!mutex_trylock(&tkn_ptr->lock))
+		return -EBUSY;
+
+	if (tkn_ptr->in_use)
+		rc = -EBUSY;
+	else
+		tkn_ptr->in_use = true;
+
+	mutex_unlock(&tkn_ptr->lock);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(devm_token_lock);
+
+/* If token is locked, unlock */
+int devm_token_unlock(struct device *dev, const char *id)
+{
+	struct token_devres *tkn_ptr;
+
+	tkn_ptr = devres_find(dev, devm_token_release, devm_token_match,
+				(void *) id);
+	if (tkn_ptr == NULL)
+		return -ENODEV;
+
+	mutex_lock(&tkn_ptr->lock);
+	tkn_ptr->in_use = false;
+	mutex_unlock(&tkn_ptr->lock);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_token_unlock);
+
+/* destroy an existing token */
+int devm_token_destroy(struct device *dev, const char *id)
+{
+	int rc;
+
+	rc = devres_release(dev, devm_token_release, devm_token_match,
+				(void *) id);
+	WARN_ON(rc);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_token_destroy);
diff --git a/include/linux/token_devres.h b/include/linux/token_devres.h
new file mode 100644
index 0000000..e411fd5
--- /dev/null
+++ b/include/linux/token_devres.h
@@ -0,0 +1,19 @@
+/*
+ * token_devres.h - managed token resource
+ *
+ * Copyright (c) 2014 Shuah Khan <shuah.kh@samsung.com>
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ *
+ * This file is released under the GPLv2.
+ */
+#ifndef __LINUX_TOKEN_DEVRES_H
+#define __LINUX_TOKEN_DEVRES_H
+
+struct device;
+
+extern int devm_token_create(struct device *dev, const char *id);
+extern int devm_token_lock(struct device *dev, const char *id);
+extern int devm_token_unlock(struct device *dev, const char *id);
+extern int devm_token_destroy(struct device *dev, const char *id);
+
+#endif	/* __LINUX_TOKEN_DEVRES_H */
-- 
1.7.10.4

[PATCH 2/4] media: dvb-fe changes to use tuner token

[Shuah Khan]

Add a new field tuner_tkn to struct dvb_frontend. Drivers can
create tuner token using devm_token_create() and initialize
the tuner_tkn when frontend is registered with the dvb-core.
This change enables drivers to provide a token devres for tuner
access control.

Change dvb_frontend to lock tuner token for exclusive access to
tuner function for digital TV function use. When Tuner token is
present, dvb_frontend_start() calls devm_token_lock() to lock
the token. If token is busy, -EBUSY is returned to the user-space.
Tuner token is unlocked if kdvb adapter fe kthread fails to start.
This token is held in use as long as the kdvb adapter fe kthread
is running. Tuner token is unlocked in dvb_frontend_thread() when
kdvb adapter fe thread exits.

Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
---
 drivers/media/dvb-core/dvb_frontend.c |   15 +++++++++++++++
 drivers/media/dvb-core/dvb_frontend.h |    1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 6ce435a..2b35e3f 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -41,6 +41,7 @@
 #include <linux/jiffies.h>
 #include <linux/kthread.h>
 #include <asm/processor.h>
+#include <linux/token_devres.h>
 
 #include "dvb_frontend.h"
 #include "dvbdev.h"
@@ -747,6 +748,9 @@ restart:
 	if (semheld)
 		up(&fepriv->sem);
 	dvb_frontend_wakeup(fe);
+
+	if (fe->tuner_tkn)
+		devm_token_unlock(fe->dvb->device, fe->tuner_tkn);
 	return 0;
 }
 
@@ -840,6 +844,15 @@ static int dvb_frontend_start(struct dvb_frontend *fe)
 	fepriv->state = FESTATE_IDLE;
 	fepriv->exit = DVB_FE_NO_EXIT;
 	fepriv->thread = NULL;
+
+	if (fe->tuner_tkn) {
+		ret = devm_token_lock(fe->dvb->device, fe->tuner_tkn);
+		if (ret) {
+			dev_info(fe->dvb->device, "Tuner is busy %d\n", ret);
+			return ret;
+		}
+	}
+
 	mb();
 
 	fe_thread = kthread_run(dvb_frontend_thread, fe,
@@ -850,6 +863,8 @@ static int dvb_frontend_start(struct dvb_frontend *fe)
 				"dvb_frontend_start: failed to start kthread (%d)\n",
 				ret);
 		up(&fepriv->sem);
+		if (fe->tuner_tkn)
+			devm_token_unlock(fe->dvb->device, fe->tuner_tkn);
 		return ret;
 	}
 	fepriv->thread = fe_thread;
diff --git a/drivers/media/dvb-core/dvb_frontend.h b/drivers/media/dvb-core/dvb_frontend.h
index 371b6ca..c9ba5fd 100644
--- a/drivers/media/dvb-core/dvb_frontend.h
+++ b/drivers/media/dvb-core/dvb_frontend.h
@@ -418,6 +418,7 @@ struct dvb_frontend {
 #define DVB_FRONTEND_COMPONENT_DEMOD 1
 	int (*callback)(void *adapter_priv, int component, int cmd, int arg);
 	int id;
+	char *tuner_tkn;
 };
 
 extern int dvb_register_frontend(struct dvb_adapter *dvb,
-- 
1.7.10.4

[PATCH 3/4] media/em28xx: changes to create token for tuner access

[Shuah Khan]

Changes add a new tuner_tkn field to struct em28xx and create
a token devres to allow sharing tuner function across analog
and digital functions. Tuner token is created during probe in
em28xx_usb_probe() and destroyed during disconnect in
em28xx_release_resources().

Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
---
 drivers/media/usb/em28xx/em28xx-cards.c |   41 +++++++++++++++++++++++++++++++
 drivers/media/usb/em28xx/em28xx.h       |    4 +++
 2 files changed, 45 insertions(+)

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index 50aa5a5..01c1955 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -2428,6 +2428,37 @@ static struct em28xx_hash_table em28xx_i2c_hash[] = {
 };
 /* NOTE: introduce a separate hash table for devices with 16 bit eeproms */
 
+/* interfaces to create and destroy token resources */
+static int em28xx_create_token_resources(struct em28xx *dev)
+{
+	int rc = 0, len;
+	char buf[64];
+
+	/* create token devres for tuner */
+	len = snprintf(buf, sizeof(buf),
+		"tuner:%s-%s-%d",
+		dev_name(&dev->udev->dev),
+		dev->udev->bus->bus_name,
+		dev->tuner_addr);
+
+	dev->tuner_tkn = devm_kzalloc(&dev->udev->dev, len + 1, GFP_KERNEL);
+	if (!dev->tuner_tkn)
+		return -ENOMEM;
+
+	strcpy(dev->tuner_tkn, buf);
+	rc = devm_token_create(&dev->udev->dev, dev->tuner_tkn);
+	if (rc)
+		return rc;
+
+	em28xx_info("Tuner token created\n");
+	return rc;
+}
+
+static void em28xx_destroy_token_resources(struct em28xx *dev)
+{
+	devm_token_destroy(&dev->udev->dev, dev->tuner_tkn);
+}
+
 int em28xx_tuner_callback(void *ptr, int component, int command, int arg)
 {
 	struct em28xx_i2c_bus *i2c_bus = ptr;
@@ -2949,6 +2980,9 @@ static void em28xx_release_resources(struct em28xx *dev)
 		em28xx_i2c_unregister(dev, 1);
 	em28xx_i2c_unregister(dev, 0);
 
+	/* destroy token resources */
+	em28xx_destroy_token_resources(dev);
+
 	usb_put_dev(dev->udev);
 
 	/* Mark device as unused */
@@ -3431,6 +3465,13 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 
 	kref_init(&dev->ref);
 
+	/* create token resources before requesting for modules */
+	if (em28xx_create_token_resources(dev)) {
+		em28xx_errdev("Error creating token resources!\n");
+		retval = -ENOMEM;
+		goto err_free;
+	}
+
 	request_modules(dev);
 
 	/* Should be the last thing to do, to avoid newer udev's to
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 2051fc9..e82f868 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -34,6 +34,7 @@
 #include <linux/mutex.h>
 #include <linux/kref.h>
 #include <linux/videodev2.h>
+#include <linux/token_devres.h>
 
 #include <media/videobuf2-vmalloc.h>
 #include <media/v4l2-device.h>
@@ -547,6 +548,9 @@ struct em28xx {
 	int devno;		/* marks the number of this device */
 	enum em28xx_chip_id chip_id;
 
+	/* token resources */
+	char *tuner_tkn; /* tuner token id */
+
 	unsigned int is_em25xx:1;	/* em25xx/em276x/7x/8x family bridge */
 	unsigned char disconnected:1;	/* device has been diconnected */
 	unsigned int has_video:1;
-- 
1.7.10.4

[PATCH 4/4] media: em28xx dvb changes to initialze tuner token

[Shuah Khan]

Changes to em28xx-dvb to initialze dvb fe tuner token when
dvb fe is registered. This will provide dvb fe the em28xx
tuner token devres for sharing tuner across analog and
digital functions.

Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
---
 drivers/media/usb/em28xx/em28xx-dvb.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
index f599b18..829e7c8 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -916,6 +916,10 @@ static int em28xx_register_dvb(struct em28xx_dvb *dvb, struct module *module,
 
 	dvb->adapter.priv = &dev->i2c_bus[dev->def_i2c_bus];
 
+	dvb->fe[0]->tuner_tkn = dev->tuner_tkn;
+	if (dvb->fe[1])
+		dvb->fe[1]->tuner_tkn = dev->tuner_tkn;
+
 	/* register frontend */
 	result = dvb_register_frontend(&dvb->adapter, dvb->fe[0]);
 	if (result < 0) {
-- 
1.7.10.4

Re: [PATCH 1/4] drivers/base: add managed token devres interfaces

[Tejun Heo]

Hello,

On Tue, Apr 29, 2014 at 01:49:23PM -0600, Shuah Khan wrote:
> +/* creates a token devres and marks it available */
> +int devm_token_create(struct device *dev, const char *id)
> +{
> +	struct token_devres *tkn;
> +	size_t tkn_size;
> +
> +	tkn_size = sizeof(struct token_devres) + strlen(id) + 1;
> +	tkn = devres_alloc(devm_token_release, tkn_size, GFP_KERNEL);
> +	if (!tkn)
> +		return -ENOMEM;

Is nesting devres inside devres really necessary?  I think it should
work but why do it this way?  Just kzalloc here and free from release.

> +
> +	strcpy(tkn->id, id);
> +	tkn->in_use = false;
> +	mutex_init(&tkn->lock);
> +
> +	devres_add(dev, tkn);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_token_create);
> +
> +/* If token is available, lock it for the caller, If not return -EBUSY */
> +int devm_token_lock(struct device *dev, const char *id)

trylock probably is a more apt name.

> +{
> +	struct token_devres *tkn_ptr;
> +	int rc = 0;
> +
> +	tkn_ptr = devres_find(dev, devm_token_release, devm_token_match,
> +				(void *)id);
> +	if (tkn_ptr == NULL)
> +		return -ENODEV;
> +
> +	if (!mutex_trylock(&tkn_ptr->lock))
> +		return -EBUSY;

How is this lock supposed to be used?  Have you tested it with lockdep
enabled?  Does it ever get released by a task which is different from
the one which locked it?  If the lock ownership is really about driver
association rather than tasks, it might be necessary to nullify
lockdep protection and add your own annotation to at least track that
unlocking driver (identified how? maybe impossible?) actually owns the
lock.

> +	if (tkn_ptr->in_use)
> +		rc = -EBUSY;
> +	else
> +		tkn_ptr->in_use = true;

Wat?  Why would you have in_use protected by trylock?  What's the
reasonsing behind that?  What would you need "try"lock there?  Okay,
strick everything I wrote above.

 Nacked-by: Tejun Heo <tj@kernel.org>

-- 
tejun

Re: [PATCH 1/4] drivers/base: add managed token devres interfaces

[Shuah Khan]

Hi Tejun,

On 05/01/2014 08:53 AM, Tejun Heo wrote:

>> +	if (!mutex_trylock(&tkn_ptr->lock))
>> +		return -EBUSY;
>
> How is this lock supposed to be used?  Have you tested it with lockdep
> enabled?  Does it ever get released by a task which is different from
> the one which locked it?  If the lock ownership is really about driver
> association rather than tasks, it might be necessary to nullify
> lockdep protection and add your own annotation to at least track that
> unlocking driver (identified how? maybe impossible?) actually owns the
> lock.
>

This lock is associated with a driver. Let me describe the use-case,
so you have more information to help me head in the right direction.

Media devices have more than one function and one or more functions
share a function without being aware that they are sharing it. For
instance, USB TV stick that supports both analog and digital TV, tuner
is shared by both these functions. When tuner is in use by analog,
digital function should not be allowed to touch it. This a longterm
lockout. So when an analog driver is using the tuner, if digital
driver tries to access the tuner, it should know as early as possible,
at the time user application requests tuning to a digital channel.

The tuner hardware is protected by a mutex, however the longer path
isn't protected. The path I am trying to protect is the between the
time digital driver gets the request from user-space and gets ready
to service it. It starts a kthread to handle the digital stream. Once
user changes channel and/or stops the stream, the kthread is terminated
and the tuner usage ends. The path that token hold is needed starts
right before kthread gets started and ends when kthread exits. This is
just an example, and analog use-case hold might start and end at times
that makes more sense for that path.

There is the audio stream as well. On some cases, snd-audio-usb handles
the audio part. Audio stream has to be active only when video stream is
active. So having a token construct at struct device level will have it
be associated with the common data structure that is available to all
drivers that provide full support for a media device.

I started testing with a device that has both analog, digital, and uses
snd-usb-audio instead of a media audio driver. I am hoping this will
help me refine this locking construct and approach.

You are right that there is a need for an owner field to indicate who
has the token. Since the path is very long, I didn't want to use just
the mutex and keep it tied up for long periods of time. That is the
reason why I added in_use field that marks it in-use or free. I hold
the mutex just to change the token status. This is what you are seeing
on the the following path:

+ if (tkn_ptr->in_use)
+		rc = -EBUSY;
+	else
+		tkn_ptr->in_use = true;


Hope this description helps you get a full picture of what I am trying
to solve. Maybe there is a different approach that would work better
than the one I am proposing.

This new device I am testing with now presents all the use-cases that 
need addressing. So I am hoping to refine the approach and make course
corrections as needed with this device.

-- Shuah

-- 
Shuah Khan
Senior Linux Kernel Developer - Open Source Group
Samsung Research America(Silicon Valley)
shuah.kh@samsung.com | (970) 672-0658

Re: [PATCH 1/4] drivers/base: add managed token devres interfaces

[Tejun Heo]

Hello, Shuah.

On Mon, May 05, 2014 at 01:16:46PM -0600, Shuah Khan wrote:
> You are right that there is a need for an owner field to indicate who
> has the token. Since the path is very long, I didn't want to use just
> the mutex and keep it tied up for long periods of time. That is the
> reason why I added in_use field that marks it in-use or free. I hold
> the mutex just to change the token status. This is what you are seeing
> on the the following path:

Can you tell me the difference between the following two?

my_trylock1() {
	if (!mutex_trylock(my_lock->lock))
		return -EBUSY;
	was_busy = my_lock->busy;
	my_lock->busy = true;
	mutex_unlock(my_lock->lock);
	return was_busy ? -EBUSY : 0;
}

my_trylock2() {
	mutex_lock();
	was_busy = my_lock->busy;
	my_lock->busy = true;
	mutex_unlock(my_lock->lock);
	return was_busy ? -EBUSY : 0;
}

Now, because the only operation you support is trylock and unlock,
neither will malfunction (as contention on the inner lock can only
happen iff there's another lock holder).  That said, the code doesn't
make any sense.

Here's the problem.  I really don't feel comfortable acking the
submitted code which implements a locking primitive when the primary
author who would probably be the primary caretaker of the code for the
time being doesn't really seem to understand basics of
synchronization.

I'm sure that this could just be from lack of experience but at least
for now I really think this should at least be gated through someone
else who's more knowledgeable and I defintely don't think I'm setting
the bar too high here.

As such, please consider the patches nacked and try to find someone
who can shepherd the code.  Mauro, can you help out here?

Thanks.

-- 
tejun

Re: [PATCH 1/4] drivers/base: add managed token devres interfaces

[Devin Heitmueller]

Hi Tejun,

On Mon, May 5, 2014 at 3:26 PM, Tejun Heo <tj@kernel.org> wrote:
> As such, please consider the patches nacked and try to find someone
> who can shepherd the code.  Mauro, can you help out here?

We actually discussed this proposal at length at the media summit last
week.  The patches are being pulled pending further internal review
and after Shuah has exercised some other use cases.

Regards,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 1/4] drivers/base: add managed token devres interfaces

[Tejun Heo]

On Mon, May 05, 2014 at 03:30:34PM -0400, Devin Heitmueller wrote:
> On Mon, May 5, 2014 at 3:26 PM, Tejun Heo <tj@kernel.org> wrote:
> > As such, please consider the patches nacked and try to find someone
> > who can shepherd the code.  Mauro, can you help out here?
> 
> We actually discussed this proposal at length at the media summit last
> week.  The patches are being pulled pending further internal review

"being pulled into a tree" or "being pulled for more work"?

> and after Shuah has exercised some other use cases.

I don't have anything against the use case, FWIW.  I just wanna make
sure the code itself ends up with an appropriate initial care taker so
that when something goes wrong it can handled.

Thanks.

-- 
tejun

Re: [PATCH 1/4] drivers/base: add managed token devres interfaces

[Devin Heitmueller]

On Mon, May 5, 2014 at 3:36 PM, Tejun Heo <tj@kernel.org> wrote:
> On Mon, May 05, 2014 at 03:30:34PM -0400, Devin Heitmueller wrote:
>> On Mon, May 5, 2014 at 3:26 PM, Tejun Heo <tj@kernel.org> wrote:
>> > As such, please consider the patches nacked and try to find someone
>> > who can shepherd the code.  Mauro, can you help out here?
>>
>> We actually discussed this proposal at length at the media summit last
>> week.  The patches are being pulled pending further internal review
>
> "being pulled into a tree" or "being pulled for more work"?

Sorry, I wasn't clear.  They are being pulled for more work.  After
discussion with the rest of the linux-media developers, it was
determined that the patches don't cover all the use cases and the
patch submission was premature.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 1/4] drivers/base: add managed token devres interfaces

[Shuah Khan]

On 05/05/2014 01:36 PM, Tejun Heo wrote:
> On Mon, May 05, 2014 at 03:30:34PM -0400, Devin Heitmueller wrote:
>> On Mon, May 5, 2014 at 3:26 PM, Tejun Heo <tj@kernel.org> wrote:
>>> As such, please consider the patches nacked and try to find someone
>>> who can shepherd the code.  Mauro, can you help out here?
>>
>> We actually discussed this proposal at length at the media summit last
>> week.  The patches are being pulled pending further internal review
>
> "being pulled into a tree" or "being pulled for more work"?
>
>> and after Shuah has exercised some other use cases.

As I said in my response:

"This new device I am testing with now presents all the use-cases that
need addressing. So I am hoping to refine the approach and make course
corrections as needed with this device."

More work is needed to address the use-cases.

-- Shuah

-- 
Shuah Khan
Senior Linux Kernel Developer - Open Source Group
Samsung Research America(Silicon Valley)
shuah.kh@samsung.com | (970) 672-0658