| Message ID | 6cb20ce23f540c883e60e6ce71302042b034c4aa.1398797955.git.shuah.kh@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, On Tue, Apr 29, 2014 at 01:49:23PM -0600, Shuah Khan wrote: > +/* creates a token devres and marks it available */ > +int devm_token_create(struct device *dev, const char *id) > +{ > + struct token_devres *tkn; > + size_t tkn_size; > + > + tkn_size = sizeof(struct token_devres) + strlen(id) + 1; > + tkn = devres_alloc(devm_token_release, tkn_size, GFP_KERNEL); > + if (!tkn) > + return -ENOMEM; Is nesting devres inside devres really necessary? I think it should work but why do it this way? Just kzalloc here and free from release. > + > + strcpy(tkn->id, id); > + tkn->in_use = false; > + mutex_init(&tkn->lock); > + > + devres_add(dev, tkn); > + return 0; > +} > +EXPORT_SYMBOL_GPL(devm_token_create); > + > +/* If token is available, lock it for the caller, If not return -EBUSY */ > +int devm_token_lock(struct device *dev, const char *id) trylock probably is a more apt name. > +{ > + struct token_devres *tkn_ptr; > + int rc = 0; > + > + tkn_ptr = devres_find(dev, devm_token_release, devm_token_match, > + (void *)id); > + if (tkn_ptr == NULL) > + return -ENODEV; > + > + if (!mutex_trylock(&tkn_ptr->lock)) > + return -EBUSY; How is this lock supposed to be used? Have you tested it with lockdep enabled? Does it ever get released by a task which is different from the one which locked it? If the lock ownership is really about driver association rather than tasks, it might be necessary to nullify lockdep protection and add your own annotation to at least track that unlocking driver (identified how? maybe impossible?) actually owns the lock. > + if (tkn_ptr->in_use) > + rc = -EBUSY; > + else > + tkn_ptr->in_use = true; Wat? Why would you have in_use protected by trylock? What's the reasonsing behind that? What would you need "try"lock there? Okay, strick everything I wrote above. Nacked-by: Tejun Heo <tj@kernel.org>
Hi Tejun, On 05/01/2014 08:53 AM, Tejun Heo wrote: >> + if (!mutex_trylock(&tkn_ptr->lock)) >> + return -EBUSY; > > How is this lock supposed to be used? Have you tested it with lockdep > enabled? Does it ever get released by a task which is different from > the one which locked it? If the lock ownership is really about driver > association rather than tasks, it might be necessary to nullify > lockdep protection and add your own annotation to at least track that > unlocking driver (identified how? maybe impossible?) actually owns the > lock. > This lock is associated with a driver. Let me describe the use-case, so you have more information to help me head in the right direction. Media devices have more than one function and one or more functions share a function without being aware that they are sharing it. For instance, USB TV stick that supports both analog and digital TV, tuner is shared by both these functions. When tuner is in use by analog, digital function should not be allowed to touch it. This a longterm lockout. So when an analog driver is using the tuner, if digital driver tries to access the tuner, it should know as early as possible, at the time user application requests tuning to a digital channel. The tuner hardware is protected by a mutex, however the longer path isn't protected. The path I am trying to protect is the between the time digital driver gets the request from user-space and gets ready to service it. It starts a kthread to handle the digital stream. Once user changes channel and/or stops the stream, the kthread is terminated and the tuner usage ends. The path that token hold is needed starts right before kthread gets started and ends when kthread exits. This is just an example, and analog use-case hold might start and end at times that makes more sense for that path. There is the audio stream as well. On some cases, snd-audio-usb handles the audio part. Audio stream has to be active only when video stream is active. So having a token construct at struct device level will have it be associated with the common data structure that is available to all drivers that provide full support for a media device. I started testing with a device that has both analog, digital, and uses snd-usb-audio instead of a media audio driver. I am hoping this will help me refine this locking construct and approach. You are right that there is a need for an owner field to indicate who has the token. Since the path is very long, I didn't want to use just the mutex and keep it tied up for long periods of time. That is the reason why I added in_use field that marks it in-use or free. I hold the mutex just to change the token status. This is what you are seeing on the the following path: + if (tkn_ptr->in_use) + rc = -EBUSY; + else + tkn_ptr->in_use = true; Hope this description helps you get a full picture of what I am trying to solve. Maybe there is a different approach that would work better than the one I am proposing. This new device I am testing with now presents all the use-cases that need addressing. So I am hoping to refine the approach and make course corrections as needed with this device. -- Shuah
Hello, Shuah. On Mon, May 05, 2014 at 01:16:46PM -0600, Shuah Khan wrote: > You are right that there is a need for an owner field to indicate who > has the token. Since the path is very long, I didn't want to use just > the mutex and keep it tied up for long periods of time. That is the > reason why I added in_use field that marks it in-use or free. I hold > the mutex just to change the token status. This is what you are seeing > on the the following path: Can you tell me the difference between the following two? my_trylock1() { if (!mutex_trylock(my_lock->lock)) return -EBUSY; was_busy = my_lock->busy; my_lock->busy = true; mutex_unlock(my_lock->lock); return was_busy ? -EBUSY : 0; } my_trylock2() { mutex_lock(); was_busy = my_lock->busy; my_lock->busy = true; mutex_unlock(my_lock->lock); return was_busy ? -EBUSY : 0; } Now, because the only operation you support is trylock and unlock, neither will malfunction (as contention on the inner lock can only happen iff there's another lock holder). That said, the code doesn't make any sense. Here's the problem. I really don't feel comfortable acking the submitted code which implements a locking primitive when the primary author who would probably be the primary caretaker of the code for the time being doesn't really seem to understand basics of synchronization. I'm sure that this could just be from lack of experience but at least for now I really think this should at least be gated through someone else who's more knowledgeable and I defintely don't think I'm setting the bar too high here. As such, please consider the patches nacked and try to find someone who can shepherd the code. Mauro, can you help out here? Thanks.
Hi Tejun, On Mon, May 5, 2014 at 3:26 PM, Tejun Heo <tj@kernel.org> wrote: > As such, please consider the patches nacked and try to find someone > who can shepherd the code. Mauro, can you help out here? We actually discussed this proposal at length at the media summit last week. The patches are being pulled pending further internal review and after Shuah has exercised some other use cases. Regards, Devin
On Mon, May 05, 2014 at 03:30:34PM -0400, Devin Heitmueller wrote: > On Mon, May 5, 2014 at 3:26 PM, Tejun Heo <tj@kernel.org> wrote: > > As such, please consider the patches nacked and try to find someone > > who can shepherd the code. Mauro, can you help out here? > > We actually discussed this proposal at length at the media summit last > week. The patches are being pulled pending further internal review "being pulled into a tree" or "being pulled for more work"? > and after Shuah has exercised some other use cases. I don't have anything against the use case, FWIW. I just wanna make sure the code itself ends up with an appropriate initial care taker so that when something goes wrong it can handled. Thanks.
On Mon, May 5, 2014 at 3:36 PM, Tejun Heo <tj@kernel.org> wrote: > On Mon, May 05, 2014 at 03:30:34PM -0400, Devin Heitmueller wrote: >> On Mon, May 5, 2014 at 3:26 PM, Tejun Heo <tj@kernel.org> wrote: >> > As such, please consider the patches nacked and try to find someone >> > who can shepherd the code. Mauro, can you help out here? >> >> We actually discussed this proposal at length at the media summit last >> week. The patches are being pulled pending further internal review > > "being pulled into a tree" or "being pulled for more work"? Sorry, I wasn't clear. They are being pulled for more work. After discussion with the rest of the linux-media developers, it was determined that the patches don't cover all the use cases and the patch submission was premature. Devin
On 05/05/2014 01:36 PM, Tejun Heo wrote: > On Mon, May 05, 2014 at 03:30:34PM -0400, Devin Heitmueller wrote: >> On Mon, May 5, 2014 at 3:26 PM, Tejun Heo <tj@kernel.org> wrote: >>> As such, please consider the patches nacked and try to find someone >>> who can shepherd the code. Mauro, can you help out here? >> >> We actually discussed this proposal at length at the media summit last >> week. The patches are being pulled pending further internal review > > "being pulled into a tree" or "being pulled for more work"? > >> and after Shuah has exercised some other use cases. As I said in my response: "This new device I am testing with now presents all the use-cases that need addressing. So I am hoping to refine the approach and make course corrections as needed with this device." More work is needed to address the use-cases. -- Shuah
diff --git a/drivers/base/Makefile b/drivers/base/Makefile index 04b314e..924665b 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -4,7 +4,7 @@ obj-y := component.o core.o bus.o dd.o syscore.o \ driver.o class.o platform.o \ cpu.o firmware.o init.o map.o devres.o \ attribute_container.o transport_class.o \ - topology.o container.o + topology.o container.o token_devres.o obj-$(CONFIG_DEVTMPFS) += devtmpfs.o obj-$(CONFIG_DMA_CMA) += dma-contiguous.o obj-y += power/ diff --git a/drivers/base/token_devres.c b/drivers/base/token_devres.c new file mode 100644 index 0000000..4644642 --- /dev/null +++ b/drivers/base/token_devres.c @@ -0,0 +1,146 @@ +/* + * drivers/base/token_devres.c - managed token resource + * + * Copyright (c) 2014 Shuah Khan <shuah.kh@samsung.com> + * Copyright (c) 2014 Samsung Electronics Co., Ltd. + * + * This file is released under the GPLv2. + */ +/* + * Media devices often have hardware resources that are shared + * across several functions. For instance, TV tuner cards often + * have MUXes, converters, radios, tuners, etc. that are shared + * across various functions. However, v4l2, alsa, DVB, usbfs, and + * all other drivers have no knowledge of what resources are + * shared. For example, users can't access DVB and alsa at the same + * time, or the DVB and V4L analog API at the same time, since many + * only have one converter that can be in either analog or digital + * mode. Accessing and/or changing mode of a converter while it is + * in use by another function results in video stream error. + * + * A shared devres that can be locked and unlocked by various drivers + * that control media functions on a single media device is needed to + * address the above problems. + * + * A token devres that can be looked up by a token for locking, try + * locking, unlocking will help avoid adding data structure + * dependencies between various media drivers. This token is a unique + * string that can be constructed from a common data structure such as + * struct device, bus_name, and hardware address. + * + * The devm_token_* interfaces manage access to token resource. + * + * Interfaces: + * devm_token_create() + * devm_token_destroy() + * devm_token_lock() + * devm_token_unlock() + * Usage: + * Create token: + * Call devm_token_create() with a token id which is + * a unique string. + * Lock token: + * Call devm_token_lock() to lock or try lock a token. + * Unlock token: + * Call devm_token_unlock(). + * Destroy token: + * Call devm_token_destroy() to delete the token. + * +*/ +#include <linux/device.h> +#include <linux/token_devres.h> + +struct token_devres { + struct mutex lock; + bool in_use; + char id[]; +}; + +static int devm_token_match(struct device *dev, void *res, void *data) +{ + struct token_devres *tkn = res; + + /* compare the token data and return 1 if it matches */ + return !strcmp(tkn->id, data); +} + +static void devm_token_release(struct device *dev, void *res) +{ + struct token_devres *tkn = res; + + mutex_destroy(&tkn->lock); +} + +/* creates a token devres and marks it available */ +int devm_token_create(struct device *dev, const char *id) +{ + struct token_devres *tkn; + size_t tkn_size; + + tkn_size = sizeof(struct token_devres) + strlen(id) + 1; + tkn = devres_alloc(devm_token_release, tkn_size, GFP_KERNEL); + if (!tkn) + return -ENOMEM; + + strcpy(tkn->id, id); + tkn->in_use = false; + mutex_init(&tkn->lock); + + devres_add(dev, tkn); + return 0; +} +EXPORT_SYMBOL_GPL(devm_token_create); + +/* If token is available, lock it for the caller, If not return -EBUSY */ +int devm_token_lock(struct device *dev, const char *id) +{ + struct token_devres *tkn_ptr; + int rc = 0; + + tkn_ptr = devres_find(dev, devm_token_release, devm_token_match, + (void *)id); + if (tkn_ptr == NULL) + return -ENODEV; + + if (!mutex_trylock(&tkn_ptr->lock)) + return -EBUSY; + + if (tkn_ptr->in_use) + rc = -EBUSY; + else + tkn_ptr->in_use = true; + + mutex_unlock(&tkn_ptr->lock); + return rc; +} +EXPORT_SYMBOL_GPL(devm_token_lock); + +/* If token is locked, unlock */ +int devm_token_unlock(struct device *dev, const char *id) +{ + struct token_devres *tkn_ptr; + + tkn_ptr = devres_find(dev, devm_token_release, devm_token_match, + (void *) id); + if (tkn_ptr == NULL) + return -ENODEV; + + mutex_lock(&tkn_ptr->lock); + tkn_ptr->in_use = false; + mutex_unlock(&tkn_ptr->lock); + return 0; +} +EXPORT_SYMBOL_GPL(devm_token_unlock); + +/* destroy an existing token */ +int devm_token_destroy(struct device *dev, const char *id) +{ + int rc; + + rc = devres_release(dev, devm_token_release, devm_token_match, + (void *) id); + WARN_ON(rc); + + return 0; +} +EXPORT_SYMBOL_GPL(devm_token_destroy); diff --git a/include/linux/token_devres.h b/include/linux/token_devres.h new file mode 100644 index 0000000..e411fd5 --- /dev/null +++ b/include/linux/token_devres.h @@ -0,0 +1,19 @@ +/* + * token_devres.h - managed token resource + * + * Copyright (c) 2014 Shuah Khan <shuah.kh@samsung.com> + * Copyright (c) 2014 Samsung Electronics Co., Ltd. + * + * This file is released under the GPLv2. + */ +#ifndef __LINUX_TOKEN_DEVRES_H +#define __LINUX_TOKEN_DEVRES_H + +struct device; + +extern int devm_token_create(struct device *dev, const char *id); +extern int devm_token_lock(struct device *dev, const char *id); +extern int devm_token_unlock(struct device *dev, const char *id); +extern int devm_token_destroy(struct device *dev, const char *id); + +#endif /* __LINUX_TOKEN_DEVRES_H */
Media devices often have hardware resources that are shared across several functions. These devices appear as a group of independent devices. Each device implements a function which could be shared by one or more functions supported by the same device. For example, tuner is shared by analog and digital TV functions. Media drivers that control a single media TV stick are a diversified group. Analog and digital TV function drivers have to coordinate access to their shared functions. Some media devices provide multiple almost-independent functions. USB and PCI core aids in allowwing multiple drivers to handle these almost-independent functions. In this model, a em28xx device could have snd-usb-audio driving the audio function. As a result, snd-usb-audio driver has to coordinate with the em28xx_* analog and digital function drivers. A shared managed resource framework at drivers/base level will allow a media device to be controlled by drivers that don't fall under drivers/media and share functions with other media drivers. A token devres that can be looked up by a token for locking, try locking, unlocking will help avoid adding data structure dependencies between various drivers. This token is a unique string that can be constructed from a common data structure such as struct device, bus_name, and hardware address. The devm_token_* interfaces manage access to token resource. Interfaces: devm_token_create() devm_token_destroy() devm_token_lock() devm_token_unlock() Usage: Create token: Call devm_token_create() with a token id string. Lock token: Call devm_token_lock() to lock or try lock a token. Unlock token: Call devm_token_unlock(). Destroy token: Call devm_token_destroy() to delete the token. Signed-off-by: Shuah Khan <shuah.kh@samsung.com> --- drivers/base/Makefile | 2 +- drivers/base/token_devres.c | 146 ++++++++++++++++++++++++++++++++++++++++++ include/linux/token_devres.h | 19 ++++++ 3 files changed, 166 insertions(+), 1 deletion(-) create mode 100644 drivers/base/token_devres.c create mode 100644 include/linux/token_devres.h