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 <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
