Add managed token devres interfaces and change media drivers to use it
Subsystems (3)
cover Add managed token devres interfaces and change media drivers to use it
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
Signature · 1 line
1.7.10.4
PATCH 1/4 drivers/base: add managed token devres interfaces
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.
--- 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
drivers/base/Makefile +1 -1
@@ -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/
drivers/base/token_devres.c new file +146 -0
146 lines not on this page · raw message
include/linux/token_devres.h new file +19 -0
@@ -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 */
Signature · 1 line
1.7.10.4
Re: [PATCH 1/4] drivers/base: add managed token devres interfaces
Hello,
drivers/base/token_devres.c line 83
Quoted 10 lines · Shuah Khan
drivers/base/token_devres.c new file +10
-0+/* 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.
drivers/base/token_devres.c line 95
Quoted 12 lines
drivers/base/token_devres.c new file +12
-0+ +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.
drivers/base/token_devres.c line 106
Quoted 11 lines
drivers/base/token_devres.c new file +11
-0+{ +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.
drivers/base/token_devres.c line 111
Quoted 4 lines
drivers/base/token_devres.c new file +4
-0+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.
Signature · 1 line
tejun
Re: [PATCH 1/4] drivers/base: add managed token devres interfaces
Hi Tejun,
Quoted 11 lines · Tejun Heo
drivers/base/token_devres.c line 106
Quoted 2 lines
drivers/base/token_devres.c new file +2
-0+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
Signature · 4 lines
Shuah Khan Senior Linux Kernel Developer - Open Source Group Samsung Research America(Silicon Valley) [email protected] | (970) 672-0658
Re: [PATCH 1/4] drivers/base: add managed token devres interfaces
Hello, Shuah.
Quoted 6 lines · Shuah Khan
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_usefield 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.
Signature · 1 line
tejun
Re: [PATCH 1/4] drivers/base: add managed token devres interfaces
Hi Tejun,
Quoted 2 lines · Tejun Heo
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
Signature · 2 lines
Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com
Re: [PATCH 1/4] drivers/base: add managed token devres interfaces
Quoted 6 lines · Devin Heitmueller
Quoted 2 lines · Tejun Heo
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"?
Quoted 1 line
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.
Signature · 1 line
tejun
Re: [PATCH 1/4] drivers/base: add managed token devres interfaces
Quoted 9 lines · Tejun Heo
Quoted 6 lines · Devin Heitmueller
Quoted 2 lines · Tejun Heo
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
Signature · 2 lines
Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com
Re: [PATCH 1/4] drivers/base: add managed token devres interfaces
In reply to Tejun HeoQuoted 11 lines · Tejun Heo
Quoted 6 lines · Devin Heitmueller
Quoted 2 lines · Tejun Heo
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"?
Quoted 1 line
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
Signature · 4 lines
Shuah Khan Senior Linux Kernel Developer - Open Source Group Samsung Research America(Silicon Valley) [email protected] | (970) 672-0658
PATCH 2/4 media: dvb-fe changes to use tuner token
In reply to Shuah KhanAdd 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.
--- drivers/media/dvb-core/dvb_frontend.c | 15 +++++++++++++++ drivers/media/dvb-core/dvb_frontend.h | 1 + 2 files changed, 16 insertions(+)
drivers/media/dvb-core/dvb_frontend.c +15 -0
@@ -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;
drivers/media/dvb-core/dvb_frontend.h +1 -0
@@ -418,6 +418,7 @@ struct dvb_frontend {#define DVB_FRONTEND_COMPONENT_DEMOD 1int (*callback)(void *adapter_priv,int component,int cmd,int arg);int id; +char *tuner_tkn; }; extern int dvb_register_frontend(struct dvb_adapter *dvb,
Signature · 1 line
1.7.10.4
PATCH 3/4 media/em28xx: changes to create token for tuner access
In reply to Shuah KhanChanges 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().
--- drivers/media/usb/em28xx/em28xx-cards.c | 41 +++++++++++++++++++++++++++++++ drivers/media/usb/em28xx/em28xx.h | 4 +++ 2 files changed, 45 insertions(+)
drivers/media/usb/em28xx/em28xx-cards.c +41 -0
@@ -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
drivers/media/usb/em28xx/em28xx.h +4 -0
@@ -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;
Signature · 1 line
1.7.10.4
PATCH 4/4 media: em28xx dvb changes to initialze tuner token
In reply to Shuah KhanChanges 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.
--- drivers/media/usb/em28xx/em28xx-dvb.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/media/usb/em28xx/em28xx-dvb.c +4 -0
@@ -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 ) {
Signature · 1 line
1.7.10.4