Re: [PATCH 1/4] drivers/base: add managed token devres interfaces

[Tejun Heo <tj@kernel.org>]

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>

-- 
tejun