[RFC PATCH 0/2] managed token devres interfaces

[RFC PATCH 0/2] managed token devres interfaces

[Shuah Khan]

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.

A new devres_* interface to update the status of this token resource
to busy when locked and free when unlocked is necessary to implement
this new managed resource.

devres_update() searches for the resource that matches supplied match
criteria similar to devres_find(). When a match is found, it calls
the update function caller passed in.

This patch set adds a new devres_update) interface and token devres
interfaces.

Test Cases for token devres interfaces: (passed)
 - Create, lock, unlock, and destroy sequence.
 - Try lock while it is locked. Returns -EBUSY as expected.
 - Try lock after destroy. Returns -ENODEV as expected.
 - Unlock while it is unlocked. Returns 0 as expected. This is a no-op. 
 - Try unlock after destroy. Returns -ENODEV as expected.

Special notes for Mauro Chehab:
 - Please evaluate if these token devres interfaces cover all media driver
   use-cases. If not what is needed to cover them.
 - For use-case testing, I generated a string from em28xx device, as this
   is common for all em28xx extensions: (hope this holds true when em28xx
   uses snd-usb-audio
 - Construct string with (dev is struct em28xx *dev)
		format: "tuner:%s-%s-%d"
		with the following:
   		dev_name(&dev->udev->dev)
                dev->udev->bus->bus_name
                dev->tuner_addr
 - I added test code to em28xx_card_setup() to test the interfaces:
   example token from this test code generated with the format above:

usb 8-1: devm_token_create(): created token: tuner:8-1-0000:00:10.1-0

Shuah Khan (2):
  drivers/base: add new devres_update() interface to devres_*
  drivers/base: add managed token devres interfaces

 drivers/base/Makefile        |    2 +-
 drivers/base/devres.c        |   36 ++++++++
 drivers/base/token_devres.c  |  204 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h       |    4 +
 include/linux/token_devres.h |   19 ++++
 5 files changed, 264 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/token_devres.c
 create mode 100644 include/linux/token_devres.h

-- 
1.7.10.4

[RFC PATCH 1/2] drivers/base: add new devres_update() interface to devres_*

[Shuah Khan]

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.

A new devres_* interface to update the status of this token resource
to busy when locked and free when unlocked is necessary to implement
this new managed resource.

devres_update() searches for the resource that matches supplied match
criteria similar to devres_find(). When a match is found, it calls
the update function caller passed in.

Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
---
 drivers/base/devres.c  |   36 ++++++++++++++++++++++++++++++++++++
 include/linux/device.h |    4 ++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index db4e264..8620600 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -272,6 +272,42 @@ void * devres_find(struct device *dev, dr_release_t release,
 EXPORT_SYMBOL_GPL(devres_find);
 
 /**
+ * devres_update - Find device resource and call update function
+ * @dev: Device to lookup resource from
+ * @release: Look for resources associated with this release function
+ * @match: Match function - must be specified
+ * @match_data: Data for the match function
+ * @update: Update function - must be specified
+ *
+ * Find the latest devres of @dev which is associated with @release
+ * and for which @match returns 1. If match is found, update will be
+ * called. This is intended for changes to status type data in a devres
+ *
+ * RETURNS:
+ * Pointer to found and updated devres, NULL if not found.
+ */
+void *devres_update(struct device *dev, dr_release_t release,
+		   dr_match_t match, void *match_data, dr_update_t update)
+{
+	struct devres *dr;
+	unsigned long flags;
+
+	if (!match || !update)
+		return NULL;
+
+	spin_lock_irqsave(&dev->devres_lock, flags);
+	dr = find_dr(dev, release, match, match_data);
+	if (dr)
+		update(dev, dr->data);
+	spin_unlock_irqrestore(&dev->devres_lock, flags);
+
+	if (dr)
+		return dr->data;
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(devres_update);
+
+/**
  * devres_get - Find devres, if non-existent, add one atomically
  * @dev: Device to lookup or add devres for
  * @new_res: Pointer to new initialized devres to add if not found
diff --git a/include/linux/device.h b/include/linux/device.h
index 233bbbe..39749df 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -576,6 +576,7 @@ extern int device_schedule_callback_owner(struct device *dev,
 /* device resource management */
 typedef void (*dr_release_t)(struct device *dev, void *res);
 typedef int (*dr_match_t)(struct device *dev, void *res, void *match_data);
+typedef void (*dr_update_t)(struct device *dev, void *data);
 
 #ifdef CONFIG_DEBUG_DEVRES
 extern void *__devres_alloc(dr_release_t release, size_t size, gfp_t gfp,
@@ -593,6 +594,9 @@ extern void devres_free(void *res);
 extern void devres_add(struct device *dev, void *res);
 extern void *devres_find(struct device *dev, dr_release_t release,
 			 dr_match_t match, void *match_data);
+extern void *devres_update(struct device *dev, dr_release_t release,
+			dr_match_t match, void *match_data,
+			dr_update_t update);
 extern void *devres_get(struct device *dev, void *new_res,
 			dr_match_t match, void *match_data);
 extern void *devres_remove(struct device *dev, dr_release_t release,
-- 
1.7.10.4

[RFC PATCH 2/2] drivers/base: add managed token devres interfaces

[Shuah Khan]

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.

Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
---
 drivers/base/Makefile        |    2 +-
 drivers/base/token_devres.c  |  204 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/token_devres.h |   19 ++++
 3 files changed, 224 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..e7436c5
--- /dev/null
+++ b/drivers/base/token_devres.c
@@ -0,0 +1,204 @@
+/*
+ * 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>
+
+#define TOKEN_DEVRES_FREE	0
+#define TOKEN_DEVRES_BUSY	1
+
+struct token_devres {
+	int	status;
+	char	id[];
+};
+
+struct tkn_match {
+	int	status;
+	const	char *id;
+};
+
+static void __devm_token_lock(struct device *dev, void *data)
+{
+	struct token_devres *tptr = data;
+
+	if (tptr && tptr->status == TOKEN_DEVRES_FREE)
+		tptr->status = TOKEN_DEVRES_BUSY;
+
+	return;
+}
+
+static void __devm_token_unlock(struct device *dev, void *data)
+{
+	struct token_devres *tptr = data;
+
+	if (tptr && tptr->status == TOKEN_DEVRES_BUSY)
+		tptr->status = TOKEN_DEVRES_FREE;
+
+	return;
+}
+
+static int devm_token_match(struct device *dev, void *res, void *data)
+{
+	struct token_devres *tkn = res;
+	struct tkn_match *mptr = data;
+	int rc;
+
+	if (!tkn || !data) {
+		WARN_ON(!tkn || !data);
+		return 0;
+	}
+
+	/* compare the token data and return 1 if it matches */
+	if (strcmp(tkn->id, mptr->id) == 0)
+			rc = 1;
+	else
+		rc = 0;
+
+	return rc;
+}
+
+static void devm_token_release(struct device *dev, void *res)
+{
+	dev_info(dev, "devm_token_release(): release token\n");
+	return;
+}
+
+/* creates a token devres and marks it free */
+int devm_token_create(struct device *dev, const char *id)
+{
+	struct token_devres *tkn;
+	size_t tkn_size;
+
+	if (!id)
+		return -EFAULT;
+
+	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->status = TOKEN_DEVRES_FREE;
+
+	devres_add(dev, tkn);
+
+	dev_info(dev, "devm_token_create(): created token: %s\n", id);
+
+	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;
+	struct tkn_match tkn;
+	int rc = 0;
+
+	if (!id)
+		return -EFAULT;
+
+	tkn.id = id;
+
+	tkn_ptr = devres_find(dev, devm_token_release, devm_token_match, &tkn);
+	if (tkn_ptr == NULL)
+		return -ENODEV;
+
+	if (tkn_ptr->status == TOKEN_DEVRES_FREE) {
+		devres_update(dev, devm_token_release, devm_token_match,
+				&tkn, __devm_token_lock);
+		rc = 0;
+	} else
+		rc = -EBUSY;
+
+	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;
+	struct tkn_match tkn;
+
+	if (!id)
+		return -EFAULT;
+
+	tkn.id = id;
+
+	tkn_ptr = devres_find(dev, devm_token_release, devm_token_match, &tkn);
+	if (tkn_ptr == NULL)
+		return -ENODEV;
+
+	if (tkn_ptr->status == TOKEN_DEVRES_BUSY) {
+		devres_update(dev, devm_token_release, devm_token_match,
+				&tkn, __devm_token_unlock);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_token_unlock);
+
+/* destroy an existing token */
+int devm_token_destroy(struct device *dev, const char *id)
+{
+	struct tkn_match tkn;
+	int rc;
+
+	if (!id)
+		return -EFAULT;
+
+	tkn.id = id;
+
+	rc = devres_release(dev, devm_token_release, devm_token_match, &tkn);
+	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

Re: [RFC PATCH 2/2] drivers/base: add managed token devres interfaces

[Tejun Heo]

Hello,

On Wed, Apr 09, 2014 at 09:21:08AM -0600, Shuah Khan wrote:
> +#define TOKEN_DEVRES_FREE	0
> +#define TOKEN_DEVRES_BUSY	1
> +
> +struct token_devres {
> +	int	status;
> +	char	id[];
> +};

Please just do "bool busy" and drop the constants.

> +struct tkn_match {
> +	int	status;
> +	const	char *id;
> +};
> +
> +static void __devm_token_lock(struct device *dev, void *data)
> +{
> +	struct token_devres *tptr = data;
> +
> +	if (tptr && tptr->status == TOKEN_DEVRES_FREE)
> +		tptr->status = TOKEN_DEVRES_BUSY;

How can this function be called with NULL @tptr and what why would you
need to check tptr->status before assigning to it if the value is
binary anyway?  And how is this supposed to work as locking if the
outcome doesn't change depending on the current value?

> +
> +	return;

No need to return from void function.

> +static int devm_token_match(struct device *dev, void *res, void *data)
> +{
> +	struct token_devres *tkn = res;
> +	struct tkn_match *mptr = data;
> +	int rc;
> +
> +	if (!tkn || !data) {
> +		WARN_ON(!tkn || !data);
> +		return 0;
> +	}

How would the above be possible?

> +
> +	/* compare the token data and return 1 if it matches */
> +	if (strcmp(tkn->id, mptr->id) == 0)
> +			rc = 1;
> +	else
> +		rc = 0;
> +
> +	return rc;

	return !strcmp(tkn->id, mptr->id);

> +/* 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;
> +	struct tkn_match tkn;
> +	int rc = 0;
> +
> +	if (!id)
> +		return -EFAULT;

The function isn't supposed to be called with NULL @id, right?  I
don't really think it'd be necessary to do the above.

> +
> +	tkn.id = id;
> +
> +	tkn_ptr = devres_find(dev, devm_token_release, devm_token_match, &tkn);
> +	if (tkn_ptr == NULL)
> +		return -ENODEV;

What guarantees that the lock is not taken by someone else inbetween?

> +
> +	if (tkn_ptr->status == TOKEN_DEVRES_FREE) {
> +		devres_update(dev, devm_token_release, devm_token_match,
> +				&tkn, __devm_token_lock);
> +		rc = 0;
> +	} else
> +		rc = -EBUSY;
> +
> +	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;
> +	struct tkn_match tkn;
> +
> +	if (!id)
> +		return -EFAULT;
> +
> +	tkn.id = id;
> +
> +	tkn_ptr = devres_find(dev, devm_token_release, devm_token_match, &tkn);
> +	if (tkn_ptr == NULL)
> +		return -ENODEV;
> +
> +	if (tkn_ptr->status == TOKEN_DEVRES_BUSY) {
> +		devres_update(dev, devm_token_release, devm_token_match,
> +				&tkn, __devm_token_unlock);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_token_unlock);

Why is devres_update() even necessary?  You can just embed lock in the
data part and operate on it, no?

This is among the most poorly written code that I've seen in a long
time.  I don't know whether the token thing is the right appraoch or
not but just purely on code quality,

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

Thanks.

-- 
tejun

Re: [RFC PATCH 2/2] drivers/base: add managed token devres interfaces

[Shuah Khan]

Hi Tejun,

On 04/16/2014 03:58 PM, Tejun Heo wrote:
> Hello,

Thanks for the review. A brief description of the use-case first.
Token is intended to be used as a large grain lock and once locked,
it can be held in the locked state for long periods of time.

For instance, application will request video to be captured and the 
media driver digital video function wants to lock a resource that is
shared between analog and digital functions.

>
> On Wed, Apr 09, 2014 at 09:21:08AM -0600, Shuah Khan wrote:
>> +#define TOKEN_DEVRES_FREE	0
>> +#define TOKEN_DEVRES_BUSY	1
>> +
>> +struct token_devres {
>> +	int	status;
>> +	char	id[];
>> +};
>
> Please just do "bool busy" and drop the constants.

Yes bool is fine.

>
>> +struct tkn_match {
>> +	int	status;
>> +	const	char *id;
>> +};
>> +
>> +static void __devm_token_lock(struct device *dev, void *data)
>> +{
>> +	struct token_devres *tptr = data;
>> +
>> +	if (tptr && tptr->status == TOKEN_DEVRES_FREE)
>> +		tptr->status = TOKEN_DEVRES_BUSY;
>
> How can this function be called with NULL @tptr and what why would you
> need to check tptr->status before assigning to it if the value is
> binary anyway?  And how is this supposed to work as locking if the
> outcome doesn't change depending on the current value?

Right. tpr null check is not needed. It is an artifact of re-arranging
the code in devm_token_lock() and __devm_token_lock() and not doing
the proper cleanup.

It can simply be a check to see if token is still free. More on this
below.

if (tptr->status == TOKEN_DEVRES_FREE)
	tptr->status = TOKEN_DEVRES_BUSY;

>
>> +
>> +	return;
>
> No need to return from void function.

Right. I will remove that.

>
>> +static int devm_token_match(struct device *dev, void *res, void *data)
>> +{
>> +	struct token_devres *tkn = res;
>> +	struct tkn_match *mptr = data;
>> +	int rc;
>> +
>> +	if (!tkn || !data) {
>> +		WARN_ON(!tkn || !data);
>> +		return 0;
>> +	}
>
> How would the above be possible?

match function and match_data are optional parameters to devres_find().
find_dr() doesn't check for match_data == null condition. There is a
definite possibility that the match_data could be null. tkn won't be.
Checking tkn is not necessary.

>
>> +
>> +	/* compare the token data and return 1 if it matches */
>> +	if (strcmp(tkn->id, mptr->id) == 0)
>> +			rc = 1;
>> +	else
>> +		rc = 0;
>> +
>> +	return rc;
>
> 	return !strcmp(tkn->id, mptr->id);

Oops! I overlooked cleaning this up after removing debug
messages.

>
>> +/* 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;
>> +	struct tkn_match tkn;
>> +	int rc = 0;
>> +
>> +	if (!id)
>> +		return -EFAULT;
>
> The function isn't supposed to be called with NULL @id, right?  I
> don't really think it'd be necessary to do the above.
>

Yes that is correct that it shouldn't be called a null id, however,
there is no guarantee that it won't happen. Would you suggest letting
this code fail with null pointer dereference in those rare cases?
It is good way to find places where the interfaces isn't used
correctly. However, it is not a graceful failure.

>> +
>> +	tkn.id = id;
>> +
>> +	tkn_ptr = devres_find(dev, devm_token_release, devm_token_match, &tkn);
>> +	if (tkn_ptr == NULL)
>> +		return -ENODEV;
>
> What guarantees that the lock is not taken by someone else inbetween?

Yes someone else can grab the lock between devres_find() and
devres_update(). It is handled since __devm_token_lock() checks
again if token is still free.

>
> Why is devres_update() even necessary?  You can just embed lock in the
> data part and operate on it, no?

Operating on the lock should be atomic, which is what devres_update()
is doing. It can be simplified as follows by holding devres_lock
in devm_token_lock().

spin_lock_irqsave(&dev->devres_lock, flags);
if (tkn_ptr->status == TOKEN_DEVRES_FREE)
	tkn_ptr->status = TOKEN_DEVRES_BUSY;
spin_unlock_irqrestore(&dev->devres_lock, flags);

Is this in-line with what you have in mind?

-- Shuah

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

Re: [RFC PATCH 2/2] drivers/base: add managed token devres interfaces

[Tejun Heo]

On Thu, Apr 17, 2014 at 02:01:32PM -0600, Shuah Khan wrote:
> Operating on the lock should be atomic, which is what devres_update()
> is doing. It can be simplified as follows by holding devres_lock
> in devm_token_lock().
> 
> spin_lock_irqsave(&dev->devres_lock, flags);
> if (tkn_ptr->status == TOKEN_DEVRES_FREE)
> 	tkn_ptr->status = TOKEN_DEVRES_BUSY;
> spin_unlock_irqrestore(&dev->devres_lock, flags);
> 
> Is this in-line with what you have in mind?

How is that different from tkn_ptr->status = TOKEN_DEVRES_BUSY?

-- 
tejun

Re: [RFC PATCH 2/2] drivers/base: add managed token devres interfaces

[Tejun Heo]

On Thu, Apr 17, 2014 at 04:10:34PM -0400, Tejun Heo wrote:
> On Thu, Apr 17, 2014 at 02:01:32PM -0600, Shuah Khan wrote:
> > Operating on the lock should be atomic, which is what devres_update()
> > is doing. It can be simplified as follows by holding devres_lock
> > in devm_token_lock().
> > 
> > spin_lock_irqsave(&dev->devres_lock, flags);
> > if (tkn_ptr->status == TOKEN_DEVRES_FREE)
> > 	tkn_ptr->status = TOKEN_DEVRES_BUSY;
> > spin_unlock_irqrestore(&dev->devres_lock, flags);
> > 
> > Is this in-line with what you have in mind?
> 
> How is that different from tkn_ptr->status = TOKEN_DEVRES_BUSY?

Let me clear it up.  How could the code snippet that you wrote
possibly function as a lock between two threads?  You're doing the
following.


	if (state == busy)
		return -EBUSY;

	spin_lock;
	if (state == free)
		state = busy;
	spin_unlock;

	return SUCCESS!!!11!!1!!;

The above is equivalent to

	if (state == busy)
		return -EBUSY;
	state = busy;
	return SUCCESS!!!11!!1!!;

Now, if you let two threads compete on it, both can return SUCCESS.
Can you see that?

Please consult with somebody who has basic understanding of
concurrency and synchronization.  Please do not implement locking
primitive directly if at all avoidable.  Why can't it use a mutex
embedded in the data area of a devres entry?  And if you for some
reason have to implement it directly, at least add lockdep
annotations.

Anyways, I really think this needs help from somebody who knows
better.

Thanks.

-- 
tejun

Re: [RFC PATCH 2/2] drivers/base: add managed token devres interfaces

[Shuah Khan]

On 04/17/2014 02:22 PM, Tejun Heo wrote:
> On Thu, Apr 17, 2014 at 04:10:34PM -0400, Tejun Heo wrote:
Please do not implement locking
> primitive directly if at all avoidable.  Why can't it use a mutex
> embedded in the data area of a devres entry?  And if you for some
> reason have to implement it directly, at least add lockdep
> annotations.
>

Thanks. This is helpful. Yes it does simplify the code with mutex
embedded in the devres data area. I am working on a v2 patch with
that change.

-- Shuah

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

Re: [RFC PATCH 2/2] drivers/base: add managed token devres interfaces

[Shuah Khan]

On 04/17/2014 02:10 PM, Tejun Heo wrote:
> On Thu, Apr 17, 2014 at 02:01:32PM -0600, Shuah Khan wrote:
>> Operating on the lock should be atomic, which is what devres_update()
>> is doing. It can be simplified as follows by holding devres_lock
>> in devm_token_lock().
>>
>> spin_lock_irqsave(&dev->devres_lock, flags);
>> if (tkn_ptr->status == TOKEN_DEVRES_FREE)
>> 	tkn_ptr->status = TOKEN_DEVRES_BUSY;
>> spin_unlock_irqrestore(&dev->devres_lock, flags);
>>
>> Is this in-line with what you have in mind?
>
> How is that different from tkn_ptr->status = TOKEN_DEVRES_BUSY?
>

I see what you are saying. The code path doesn't ensure two threads
not getting the lock. I have a bug in here that my rc settings aren't
protected. You probably noticed that the RFC tag on the patch and this
isn't fully cooked yet.

I started working on driver changes that use this token and I might have
to add owner for the token as well. I hope to work these details out and
send a real patch.

thanks,
-- Shuah

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

Re: [RFC PATCH 0/2] managed token devres interfaces

[Greg KH]

On Wed, Apr 09, 2014 at 09:21:06AM -0600, Shuah Khan wrote:
> 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.
> 
> A new devres_* interface to update the status of this token resource
> to busy when locked and free when unlocked is necessary to implement
> this new managed resource.
> 
> devres_update() searches for the resource that matches supplied match
> criteria similar to devres_find(). When a match is found, it calls
> the update function caller passed in.
> 
> This patch set adds a new devres_update) interface and token devres
> interfaces.
> 
> Test Cases for token devres interfaces: (passed)
>  - Create, lock, unlock, and destroy sequence.
>  - Try lock while it is locked. Returns -EBUSY as expected.
>  - Try lock after destroy. Returns -ENODEV as expected.
>  - Unlock while it is unlocked. Returns 0 as expected. This is a no-op. 
>  - Try unlock after destroy. Returns -ENODEV as expected.

Any chance you can add these "test cases" as part of the kernel code so
it lives here for any future changes?

> Special notes for Mauro Chehab:
>  - Please evaluate if these token devres interfaces cover all media driver
>    use-cases. If not what is needed to cover them.
>  - For use-case testing, I generated a string from em28xx device, as this
>    is common for all em28xx extensions: (hope this holds true when em28xx
>    uses snd-usb-audio
>  - Construct string with (dev is struct em28xx *dev)
> 		format: "tuner:%s-%s-%d"
> 		with the following:
>    		dev_name(&dev->udev->dev)
>                 dev->udev->bus->bus_name
>                 dev->tuner_addr
>  - I added test code to em28xx_card_setup() to test the interfaces:
>    example token from this test code generated with the format above:

What would the driver changes look like to take advantage of these new
functions?

thanks,

greg k-h

Re: [RFC PATCH 0/2] managed token devres interfaces

[Shuah Khan]

On 04/09/2014 01:17 PM, Greg KH wrote:
> On Wed, Apr 09, 2014 at 09:21:06AM -0600, Shuah Khan wrote:

>>
>> Test Cases for token devres interfaces: (passed)
>>   - Create, lock, unlock, and destroy sequence.
>>   - Try lock while it is locked. Returns -EBUSY as expected.
>>   - Try lock after destroy. Returns -ENODEV as expected.
>>   - Unlock while it is unlocked. Returns 0 as expected. This is a no-op.
>>   - Try unlock after destroy. Returns -ENODEV as expected.
>
> Any chance you can add these "test cases" as part of the kernel code so
> it lives here for any future changes?

Yes. I am planning to add these test cases to the kernel to serve as 
regression tests when these interfaces change. I have to add these in a 
driver framework, i.e I might need to create dummy driver perhaps. I 
haven't given it much thought on how, but I do plan to add tests.

>
>> Special notes for Mauro Chehab:
>>   - Please evaluate if these token devres interfaces cover all media driver
>>     use-cases. If not what is needed to cover them.
>>   - For use-case testing, I generated a string from em28xx device, as this
>>     is common for all em28xx extensions: (hope this holds true when em28xx
>>     uses snd-usb-audio
>>   - Construct string with (dev is struct em28xx *dev)
>> 		format: "tuner:%s-%s-%d"
>> 		with the following:
>>     	            dev_name(&dev->udev->dev)
>>                  dev->udev->bus->bus_name
>>                  dev->tuner_addr
>>   - I added test code to em28xx_card_setup() to test the interfaces:
>>     example token from this test code generated with the format above:
>
> What would the driver changes look like to take advantage of these new
> functions?
>

I am working on changes to em28xx driver to create and lock/unlock tuner 
token when it starts analog/digital video streaming. I should have a 
patch ready in a day or two.

thanks,
-- Shuah


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

Re: [RFC PATCH 0/2] managed token devres interfaces

[One Thousand Gnomes]

> >>   - Construct string with (dev is struct em28xx *dev)
> >> 		format: "tuner:%s-%s-%d"
> >> 		with the following:
> >>     	            dev_name(&dev->udev->dev)
> >>                  dev->udev->bus->bus_name
> >>                  dev->tuner_addr

What guarantees this won't get confused by hot plugging and re-use of the
bus slot ?


I'm also not sure I understand why you can't have a shared parent device
and simply attach the resources to that. This sounds like a problem mfd
already solved ?

Alan

Re: [RFC PATCH 0/2] managed token devres interfaces

[Mauro Carvalho Chehab]

Hi Alan,

Em Thu, 10 Apr 2014 12:04:35 +0100
One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> escreveu:

> > >>   - Construct string with (dev is struct em28xx *dev)
> > >> 		format: "tuner:%s-%s-%d"
> > >> 		with the following:
> > >>     	            dev_name(&dev->udev->dev)
> > >>                  dev->udev->bus->bus_name
> > >>                  dev->tuner_addr
> 
> What guarantees this won't get confused by hot plugging and re-use of the
> bus slot ?

Good point. Yes, this should be addressed.

> I'm also not sure I understand why you can't have a shared parent device
> and simply attach the resources to that. This sounds like a problem mfd
> already solved ?

There are some devices that have lots of different functions spread
out on several subsystems.

For example, some devices provide standard USB Audio Class, handled by
snd-usb-audio for the audio stream, while the video stream is handled
via a separate driver, like some em28xx devices.

There are even more complex devices that provide 3G modem, storage
and digital TV, whose USB ID changes when either the 3G modem starts
or when the digital TV firmware is loaded.

So, we need to find a way to lock some hardware resources among
different subsystems that don't share anything in common. Not sure if
mfd has the same type of problem of a non-mfd driver using another
function of the same device that has some shared hardware resources
between the separate functions, and, if so, how they solved it.

Regards,
Mauro

Re: [RFC PATCH 0/2] managed token devres interfaces

[One Thousand Gnomes]

> For example, some devices provide standard USB Audio Class, handled by
> snd-usb-audio for the audio stream, while the video stream is handled
> via a separate driver, like some em28xx devices.

Which is what mfd is designed to handle.

> There are even more complex devices that provide 3G modem, storage
> and digital TV, whose USB ID changes when either the 3G modem starts
> or when the digital TV firmware is loaded.

But presumably you only have one driver at a time then ?

> So, we need to find a way to lock some hardware resources among
> different subsystems that don't share anything in common. Not sure if
> mfd has the same type of problem of a non-mfd driver using another
> function of the same device

The MFD device provides subdevices for all the functions. That is the
whole underlying concept.

Re: [RFC PATCH 0/2] managed token devres interfaces

[Mauro Carvalho Chehab]

Em Thu, 10 Apr 2014 12:46:53 +0100
One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> escreveu:

> > For example, some devices provide standard USB Audio Class, handled by
> > snd-usb-audio for the audio stream, while the video stream is handled
> > via a separate driver, like some em28xx devices.
> 
> Which is what mfd is designed to handle.
> 
> > There are even more complex devices that provide 3G modem, storage
> > and digital TV, whose USB ID changes when either the 3G modem starts
> > or when the digital TV firmware is loaded.
> 
> But presumably you only have one driver at a time then ?

In this specific device, before USB ID changes, only storage is
available, as it contains the manufacturer's Windows driver on it
(and the device firmware).

After the USB ID changes and after the 3G chip manufacturer is 
recognized (if the device is locked to an specific 3G carrier), 
the USB storage switches to work as a micro SD memory reader
and the TV and 3G modem functions also become available.

The Kernel drivers that are used by this device are:

smsdvb                 18471  0 
dvb_core              114974  1 smsdvb
option                 42468  0 
smsusb                 17819  0 
usb_wwan               19510  1 option
smsmdtv                52283  2 smsdvb,smsusb
rc_core                27210  1 smsmdtv
usb_storage            56690  0 

Those are the logs:

[210431.241488] usb 1-1.3: new high-speed USB device number 7 using ehci-pci
[210431.340676] usb 1-1.3: New USB device found, idVendor=19d2, idProduct=2000
[210431.340683] usb 1-1.3: New USB device strings: Mfr=3, Product=2, SerialNumber=4
[210431.340687] usb 1-1.3: Product: ZTE WCDMA Technologies MSM
[210431.340691] usb 1-1.3: Manufacturer: ZTE,Incorporated
[210431.340695] usb 1-1.3: SerialNumber: P675B1ZTED010000
[210431.344778] usb-storage 1-1.3:1.0: USB Mass Storage device detected
[210431.344891] scsi10 : usb-storage 1-1.3:1.0
[210432.701801] usb 1-1.3: USB disconnect, device number 7
[210438.735217] usb 1-1.3: new high-speed USB device number 8 using ehci-pci
[210438.834309] usb 1-1.3: New USB device found, idVendor=19d2, idProduct=0086
[210438.834314] usb 1-1.3: New USB device strings: Mfr=3, Product=2, SerialNumber=4
[210438.834317] usb 1-1.3: Product: ZTE WCDMA Technologies MSM
[210438.834319] usb 1-1.3: Manufacturer: ZTE,Incorporated
[210438.834321] usb 1-1.3: SerialNumber: P675B1ZTED010000
[210438.839042] option 1-1.3:1.0: GSM modem (1-port) converter detected
[210438.839369] usb 1-1.3: GSM modem (1-port) converter now attached to ttyUSB1
[210438.839566] option 1-1.3:1.1: GSM modem (1-port) converter detected
[210438.839749] usb 1-1.3: GSM modem (1-port) converter now attached to ttyUSB2
[210438.839880] usb-storage 1-1.3:1.2: USB Mass Storage device detected
[210438.840116] scsi11 : usb-storage 1-1.3:1.2
[210438.840407] option 1-1.3:1.3: GSM modem (1-port) converter detected
[210438.840593] usb 1-1.3: GSM modem (1-port) converter now attached to ttyUSB3
[210438.840811] option 1-1.3:1.4: GSM modem (1-port) converter detected
[210438.840961] usb 1-1.3: GSM modem (1-port) converter now attached to ttyUSB4
[210439.844891] scsi 11:0:0:0: Direct-Access     ZTE      MMC Storage      2.31 PQ: 0 ANSI: 2
[210439.845516] sd 11:0:0:0: Attached scsi generic sg4 type 0
[210439.854702] sd 11:0:0:0: [sdd] Attached SCSI removable disk
[210449.269010] smscore_detect_mode: line: 1274: MSG_SMS_GET_VERSION_EX_REQ failed first try
[210454.266114] smscore_set_device_mode: line: 1354: mode detect failed -62
[210454.266121] smsusb_init_device: line: 436: smscore_start_device(...) failed
[210454.266371] smsusb_onresponse: line: 143: error, urb status -2, 0 bytes
[210454.266618] smsusb_onresponse: line: 143: error, urb status -2, 0 bytes
[210454.266855] smsusb_onresponse: line: 143: error, urb status -2, 0 bytes
[210454.267090] smsusb_onresponse: line: 143: error, urb status -2, 0 bytes
[210454.267341] smsusb_onresponse: line: 143: error, urb status -2, 0 bytes
[210454.267593] smsusb_onresponse: line: 143: error, urb status -2, 0 bytes
[210454.267868] smsusb_onresponse: line: 143: error, urb status -2, 0 bytes
[210454.268106] smsusb_onresponse: line: 143: error, urb status -2, 0 bytes
[210454.268340] smsusb_onresponse: line: 143: error, urb status -2, 0 bytes
[210454.268609] smsusb_onresponse: line: 143: error, urb status -2, 0 bytes
[210454.268625] sms_ir_exit: 
[210454.274338] smsusb: probe of 1-1.3:1.5 failed with error -62
[210454.274413] option 1-1.3:1.5: GSM modem (1-port) converter detected
[210454.274768] usb 1-1.3: GSM modem (1-port) converter now attached to ttyUSB5
[210454.310745] sd 11:0:0:0: [sdd] 7744512 512-byte logical blocks: (3.96 GB/3.69 GiB)
[210454.316753] sd 11:0:0:0: [sdd] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
[210454.334348]  sdd: sdd1
[210455.709432] EXT4-fs (sdd1): mounting ext3 file system using the ext4 subsystem

> > So, we need to find a way to lock some hardware resources among
> > different subsystems that don't share anything in common. Not sure if
> > mfd has the same type of problem of a non-mfd driver using another
> > function of the same device
> 
> The MFD device provides subdevices for all the functions. That is the
> whole underlying concept.

I'll take a look on how it works, and how it locks resources on
other drivers. What drivers outside drivers/mfd use shared resources
that require locks controlled by mfd?

-- 

Regards,
Mauro

Re: [RFC PATCH 0/2] managed token devres interfaces

[Shuah Khan]

Here is an example driver use-case for this token resource: This is not 
polished, but should give an idea on this new token resource is intended 
to be used:

- create token resources in em28xx_usb_probe() right before the required 
modules are loaded. It creates tuner, video and audio tokens to use as a 
large grain lock on tuner, video DMA engine, and audio DMA engine 
respectively.

- lock video token in em28xx_init_usb_xfer()
- unlock in em28xx_uninit_usb_xfer()

- destroy token in em28xx_release_resources()


I did some hotplug testing with this:

dmesg | grep token
[  771.613332] usb 8-1: devm_token_create(): created token: 
tuner:8-1-0000:00:10.1-0
[  771.613335] em2874 #0: devm_token_create() returned 0
[  771.613340] usb 8-1: devm_token_create(): created token: 
video:8-1-0000:00:10.1
[  851.202931] usb 8-1: devm_token_release(): release token
[  851.202936] usb 8-1: devm_token_release(): release token
[ 1775.052273] usb 8-2: devm_token_create(): created token: 
tuner:8-2-0000:00:10.1-0
[ 1775.052276] em2874 #0: devm_token_create() returned 0
[ 1775.052285] usb 8-2: devm_token_create(): created token: 
video:8-2-0000:00:10.1
shuah@anduin:~$ dmesg | grep token
[  771.613332] usb 8-1: devm_token_create(): created token: 
tuner:8-1-0000:00:10.1-0
[  771.613335] em2874 #0: devm_token_create() returned 0
[  771.613340] usb 8-1: devm_token_create(): created token: 
video:8-1-0000:00:10.1
[  851.202931] usb 8-1: devm_token_release(): release token
[  851.202936] usb 8-1: devm_token_release(): release token
[ 1775.052273] usb 8-2: devm_token_create(): created token: 
tuner:8-2-0000:00:10.1-0
[ 1775.052276] em2874 #0: devm_token_create() returned 0
[ 1775.052285] usb 8-2: devm_token_create(): created token: 
video:8-2-0000:00:10.1
[ 1799.610007] usb 8-2: devm_token_release(): release token
[ 1799.610013] usb 8-2: devm_token_release(): release token


diff --git a/drivers/media/usb/em28xx/em28xx-cards.c 
b/drivers/media/usb/em28xx/em28xx-cards.c
index 50aa5a5..3305250 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -2428,6 +2428,51 @@ 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;
+
+       /* create token devres for tuner */
+       snprintf(dev->tuner_tkn_id, sizeof(dev->tuner_tkn_id),
+               "tuner:%s-%s-%d",
+               dev_name(&dev->udev->dev),
+               dev->udev->bus->bus_name,
+               dev->tuner_addr);
+
+       rc = devm_token_create(&dev->udev->dev, dev->tuner_tkn_id);
+       em28xx_info("devm_token_create() returned %d\n", rc);
+
+       if (dev->has_video || dev->board.has_dvb) {
+               snprintf(dev->video_tkn_id, sizeof(dev->video_tkn_id),
+                       "video:%s-%s",
+                       dev_name(&dev->udev->dev),
+                       dev->udev->bus->bus_name);
+               rc = devm_token_create(&dev->udev->dev, dev->video_tkn_id);
+       }
+       if (dev->audio_mode.has_audio) {
+               snprintf(dev->audio_tkn_id, sizeof(dev->audio_tkn_id),
+                       "audio:%s-%s",
+                       dev_name(&dev->udev->dev),
+                       dev->udev->bus->bus_name);
+               rc = devm_token_create(&dev->udev->dev, dev->audio_tkn_id);
+       }
+       return rc;
+}
+
+static int em28xx_destroy_token_resources(struct em28xx *dev)
+{
+       int rc = 0;
+
+       devm_token_destroy(&dev->udev->dev, dev->tuner_tkn_id);
+       if (dev->has_video || dev->board.has_dvb)
+               rc = devm_token_destroy(&dev->udev->dev, dev->video_tkn_id);
+       if (dev->audio_mode.has_audio)
+               rc = devm_token_destroy(&dev->udev->dev, dev->audio_tkn_id);
+
+       return rc;
+}

  int em28xx_tuner_callback(void *ptr, int component, int command, int arg)
  {
         struct em28xx_i2c_bus *i2c_bus = ptr;
@@ -2949,6 +2994,10 @@ static void em28xx_release_resources(struct 
em28xx *dev)
                 em28xx_i2c_unregister(dev, 1);
         em28xx_i2c_unregister(dev, 0);

+       /* destroy token resources */
+       if (em28xx_destroy_token_resources(dev))
+               em28xx_info("Error destroying token resources\n");
+
         usb_put_dev(dev->udev);

         /* Mark device as unused */
@@ -3431,6 +3480,10 @@ 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_info("Error creating token resources\n");
+
         request_modules(dev);

         /* Should be the last thing to do, to avoid newer udev's to
diff --git a/drivers/media/usb/em28xx/em28xx-core.c 
b/drivers/media/usb/em28xx/em28xx-core.c
index 523d7e9..8129a9c 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -824,6 +824,9 @@ void em28xx_uninit_usb_xfer(struct em28xx *dev, enum 
em28xx_mode mode)
         usb_bufs->num_bufs = 0;

         em28xx_capture_start(dev, 0);
+
+       /* unlock token */
+       devm_token_unlock(&dev->udev->dev, dev->video_tkn_id);
  }
  EXPORT_SYMBOL_GPL(em28xx_uninit_usb_xfer);

@@ -993,6 +996,11 @@ int em28xx_init_usb_xfer(struct em28xx *dev, enum 
em28xx_mode mode,
         em28xx_isocdbg("em28xx: called em28xx_init_usb_xfer in mode %d\n",
                        mode);

+       /* lock token */
+       rc = devm_token_lock(&dev->udev->dev, dev->video_tkn_id);
+       if (rc)
+               return rc;
+
         dev->usb_ctl.urb_data_copy = urb_data_copy;

         if (mode == EM28XX_DIGITAL_MODE) {
diff --git a/drivers/media/usb/em28xx/em28xx.h 
b/drivers/media/usb/em28xx/em28xx.h
index 2051fc9..b13ec6e 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,11 @@ struct em28xx {
         int devno;              /* marks the number of this device */
         enum em28xx_chip_id chip_id;

+       /* token resources */
+       char tuner_tkn_id[64]; /* tuner token id */
+       char video_tkn_id[64]; /* video dma engine token */
+       char audio_tkn_id[64]; /* audio dma engine 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;

thanks
-- Shuah

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

Re: [RFC PATCH 0/2] managed token devres interfaces

[Shuah Khan]

On 04/10/2014 08:39 AM, Mauro Carvalho Chehab wrote:
> Em Thu, 10 Apr 2014 12:46:53 +0100
> One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> escreveu:
>
>>> For example, some devices provide standard USB Audio Class, handled by
>>> snd-usb-audio for the audio stream, while the video stream is handled
>>> via a separate driver, like some em28xx devices.
>>
>> Which is what mfd is designed to handle.
>>
>>> There are even more complex devices that provide 3G modem, storage
>>> and digital TV, whose USB ID changes when either the 3G modem starts
>>> or when the digital TV firmware is loaded.
>>
>> But presumably you only have one driver at a time then ?
>
>> The MFD device provides subdevices for all the functions. That is the
>> whole underlying concept.
>
> I'll take a look on how it works, and how it locks resources on
> other drivers. What drivers outside drivers/mfd use shared resources
> that require locks controlled by mfd?
>

One question that came up during the RFC review is:

Can media drivers use mfd framework for sharing functions across drivers?

I looked into the mfd framework and see if it helps the problem at hand 
hand. My conclusions as follows:

1. device presentation
- media devices appear as a group of devices that can be clearly
   identified as independent devices. Each device implements a
   function which could be shared by one or more functions.

- mfd devices appear as a single device. mfd framework helps present
   them as discrete platform devices. This model seems to be similar
   to a multi-function I/O card.

This above is an important difference between these two device types.

Media devices could benefit from using mfd framework in grouping
the device to make it easier to treat these devices as a group. mfd
framework could handle adding and removing devices. Something that
could be looked into in detail to see if it makes sense to use mfd
framework.

2. function sharing
- each media device supports a function and one or more functions share
   another function. For example, tuner is shared by analog and digital
   TV functions.

- mfd devices don't seem to work this way. Each function seems to be
   independent. Sharing is limited to the attach point. It works more
   like a bus, where sub-devices attach. I couldn't find any examples
   of a device/function being shared by other functions like in the case
   of media devices.

3. drivers
-  mfd/viperboard uses mfd to group gpio, i2c and does load viperboard
    i2c and viperboard gpio drivers. These drivers seem to have anything
    in common during run-time. i.e once mfd framework is used to carve
    the device up to appear as discrete devices, each device can function
    independently.

-  media drivers that drive one single media TV stick are a diversified
    group. In general, Analog and digital TV functions have to coordinate
    access to their shared functions.

4. Locking
    Both media and mfd drivers have mechanisms to lock/unlock hardware.
    mfd framework itself doesn't have any locking constructs.

    mfd drivers use mutex similar to the way media drivers use to protect
    access to hardware. This is a fine grain locking.

However, media drivers need a large grain media function level locking
mechanism, where a token devres will come in handy. I think this type of
locking is needed even when media drivers take advantage of the mfd 
framework to group sub-devices.

Another question to ask is:
Why add token resources to drivers/base? Why not add at drivers/media?

Some medial devices provide multiple almost-independent functions. USB
and PCI core work is to allow multiple drivers to handle those
almost-independent functions.

For instance, snd-usb-audio driver will handle USB Audio Class devices,
including some em28x devices, as it provides an independent UAC interface,
while analog and digital TV are provided via another interface.

What this means is a em28xx device could have snd-usb-audio driving the
audio function.  However, the audio stream is only available if the
video stream is working. As a result, snd-usb-audio driver has to 
coordinate with the analog and digital functions em28xx_* drivers 
control. Hence, the need for a shared managed resource interfaces such 
as the proposed token devres interfaces at drivers/base level. This will
allow a media device to be controlled by drivers that don't necessarily
fall under drivers/media as in this above example.

Webcams with microphone on the other hand have completely independent
devices for audio and video. They don't need a shared devres type 
feature, as the USB core handles that properly.

Based on the above, I think, the mfd framework will not help address the 
media driver problem at hand. However, I do think media drivers can 
benefit from using the mfd framework for streamlined handling of 
sub-devices. I would propose going forward with adding the token devres 
to solve the issue of sharing functions across drivers that control the 
functions.

-- Shuah

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

Re: [RFC PATCH 0/2] managed token devres interfaces

[Shuah Khan]

On 04/10/2014 05:38 AM, Mauro Carvalho Chehab wrote:
> Hi Alan,
>
> Em Thu, 10 Apr 2014 12:04:35 +0100
> One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> escreveu:
>
>>>>>    - Construct string with (dev is struct em28xx *dev)
>>>>> 		format: "tuner:%s-%s-%d"
>>>>> 		with the following:
>>>>>      	            dev_name(&dev->udev->dev)
>>>>>                   dev->udev->bus->bus_name
>>>>>                   dev->tuner_addr
>>
>> What guarantees this won't get confused by hot plugging and re-use of the
>> bus slot ?
>
> Good point. Yes, this should be addressed.
>

This resource should be destroyed when em28xx_ handles unplug from its 
em28xx_usb_disconnect() or in generic words, in its "uninit". It is a 
matter of making sure this resource is handled in the "uninit" for this 
media device/driver(s) like any other shared resource.

Would that cover the hot plugging and re-use of the bus slot scenario?

-- Shuah


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