| Message ID | 5f21c7e53811aba63f86bcf3e3bfdfdd5aeedf59.1397050852.git.shuah.kh@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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?
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.
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
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
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 */
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