[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20140416215821.GG26632@htj.dyndns.org>
Date: Wed, 16 Apr 2014 17:58:21 -0400
From: Tejun Heo <tj@...nel.org>
To: Shuah Khan <shuah.kh@...sung.com>
Cc: gregkh@...uxfoundation.org, m.chehab@...sung.com,
rafael.j.wysocki@...el.com, linux@...ck-us.net, toshi.kani@...com,
linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
shuahkhan@...il.com
Subject: 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_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@...nel.org>
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists