Re: [RFC PATCH 2/2] drivers/base: add managed token devres interfaces
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;
+ charid[];
+};
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...@samsung.com | (970) 672-0658
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/2] drivers/base: add managed token devres interfaces
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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/2] drivers/base: add managed token devres interfaces
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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/2] drivers/base: add managed token devres interfaces
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...@samsung.com | (970) 672-0658 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/2] drivers/base: add managed token devres interfaces
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...@samsung.com | (970) 672-0658 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/2] drivers/base: add managed token devres interfaces
Hello,
On Wed, Apr 09, 2014 at 09:21:08AM -0600, Shuah Khan wrote:
+#define TOKEN_DEVRES_FREE0
+#define TOKEN_DEVRES_BUSY1
+
+struct token_devres {
+ int status;
+ charid[];
+};
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 t...@kernel.org
Thanks.
--
tejun
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
