Oliver Neukum <oliver@xxxxxxxxxx> writes:
> Am Mittwoch, 1. Februar 2012, 16:05:22 schrieb Bjørn Mork:
>> @@ -578,11 +580,11 @@ static int wdm_open(struct inode *inode, struct file *file)
>> dev_err(&desc->intf->dev, "Error autopm - %d\n", rv);
>> goto out;
>> }
>> - intf->needs_remote_wakeup = 1;
>>
>> /* using write lock to protect desc->count */
>> mutex_lock(&desc->wlock);
>> if (!desc->count++) {
>> + desc->manage_power(intf, 1);
>
> It makes me very nervous to see manage_power
> called under a lock. Are you positively sure this cannot
> deadlock?
Yes, given the current code I am. usb_autopm_get_interface was called
right before this so we are sure manage_power won't have to wait for
resume. Not that that would need this lock anyway. And I cannot think
of anything else that could possibly cause manage_power to require the
same lock.
But I do understand the question, and you're right that it would be
better to call it without holding any locks. Obviously problem free
code is much better than have-to-be-explained-why-its-problem-free code.
This might not be obvious, but the call was moved inside the "if
(!desc->count++)" block to make the calls symmetric. That wasn't
required for setting needs_remote_wakeup directly, and wdm_open would
set it unconditionally while wdm_release only would clear it only if
desc->count was zero. Which made sense.
Will think about how to fix this best so the lock can be avoided. I do
not want to make manage_power aware of the cdc-wdm internal "count", as
that would either require it to make an additional device list lookup or
the internal state struct to be exported to the main driver. I don't
find any of those solutions acceptable.
Hmm, looking at the code, and realizing that the reason we're taking
wlock here only is that the driver used to take the combined read/write
lock, I don't think it's necessary at all. desc->count is only modified
by wdm_open() and wdm_release(), both holding the global wdm_mutex at
the time. The only place desc->count is tested without holding the
wdm_mutex lock is in recover_from_urb_loss() which is called from
wdm_resume() and wdm_post_reset(). I don't think that is a problem
though, as both require holding the device lock which will protect them
from competing against each other.
So in short: I don't think the wlock offers any extra protection in
wdm_open and wdm_release and I would like to just drop it if you too
think that's OK.
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
