Em Wed, 16 Mar 2016 10:28:35 +0200 Sakari Ailus sakari.ailus@iki.fi escreveu:
Hi Mauro,
On Tue, Mar 15, 2016 at 12:55:35PM -0300, Mauro Carvalho Chehab wrote:
Em Mon, 14 Mar 2016 14:09:09 +0200 Sakari Ailus sakari.ailus@iki.fi escreveu:
Hi Mauro,
...
Notify callbacks, perhaps not, but the list is still protected by the spinlock. It perhaps is not likely that another process would change it but I don't think we can rely on that.
I can see only 2 risks protected by the lock:
mdev gets freed while an entity is being created. This is a problem with the current memory protection schema we're using. I guess the only way to fix it is to use kref for mdev/entities/interfaces/links/pads. This change doesn't make it better or worse. Also, I don't think we have such risk with the current devices.
a notifier may be inserted or removed by another driver, while the loop is running.
To avoid (2), I see 3 alternatives:
a) keep the loop as proposed on this patch. As the list is navigated using list_for_each_entry_safe(), I guess[1] it should be safe to remove/add new notify callbacks there while the loop is running by some other process.
list_for_each_entry_safe() does not protect against concurrent access, only against adding and removing list entries by the same user. List access serialisation is still needed, whether you use _safe() functions or not.
[1] It *is* safe if the change were done inside the loop - but I'm not 100% sure that it is safe if some other CPU touches the notify list.
Indeed.
b) Unlock/relock the spinlock every time:
/* previous code that locks mdev->lock spinlock */
/* invoke entity_notify callbacks */ list_for_each_entry_safe(notify, next, &mdev->entity_notify, list) { spin_unlock(&mdev->lock); (notify)->notify(entity, notify->notify_data); spin_lock(&mdev->lock); }
spin_unlock(&mdev->lock);
c) use a separate lock for the notify list -this seems to be an overkill.
d) Protect it with the graph traversal mutex. That sounds the worse idea, IMHO, as we'll be abusing the lock.
I'd simply replace the spinlock with a mutex here. As we want to get rid of the graph mutex anyway in the long run, let's not mix the two as they're well separated now. As long as the mutex users do not sleep (i.e. the notify() callback) the mutex is about as fast to use as the spinlock.
It could work. I added such patch on an experimental branch, where I'm addressing a few troubles with au0828 unbind logic: https://git.linuxtv.org/mchehab/experimental.git/log/?h=au0828-unbind-fixes
The patch itself is at: https://git.linuxtv.org/mchehab/experimental.git/commit/?h=au0828-unbind-fix...
At least for au0828, it seems to work fine.
Regards, Mauro