managed token devres interfaces

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

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.

---
 drivers/base/devres.c  |   36 ++++++++++++++++++++++++++++++++++++
 include/linux/device.h |    4 ++++
 2 files changed, 40 insertions(+)

index db4e264..8620600 100644
@@ -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

index 233bbbe..39749df 100644
@@ -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

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.

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

index 04b314e..924665b 100644
@@ -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/

new file mode 100644
index 0000000..e411fd5
@@ -0,0 +1,19 @@
+/*
+ * token_devres.h - managed token resource
+ *
+ * Copyright (c) 2014 Shuah Khan <[email protected]>
+ * 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

Hello,

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

tejun

Hi Tejun,

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.

Yes bool is fine.

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;

Right. I will remove that.

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.

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

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.

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.

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)
[email protected] | (970) 672-0658

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

tejun

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

Please do not implement locking

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)
[email protected] | (970) 672-0658

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)
[email protected] | (970) 672-0658

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

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

thanks,

greg k-h

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.

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)
[email protected] | (970) 672-0658

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

Hi Alan,

Em Thu, 10 Apr 2014 12:04:35 +0100
One Thousand Gnomes <[email protected]> escreveu:

Good point. Yes, this should be addressed.

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

Which is what mfd is designed to handle.

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.

Em Thu, 10 Apr 2014 12:46:53 +0100
One Thousand Gnomes <[email protected]> escreveu:

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

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

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

index 50aa5a5..3305250 100644
@@ -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

index 523d7e9..8129a9c 100644
@@ -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) {

index 2051fc9..b13ec6e 100644
@@ -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)
[email protected] | (970) 672-0658

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)
[email protected] | (970) 672-0658

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)
[email protected] | (970) 672-0658