> >> Adding to Ezequiel's point, shouldn't we take more granular lock > >> (devfreq->lock) first and then call devfreq_list_lock at the time of > >> adding to the list? > >> > > > > Not sure why we should do that. devfreq->lock should be used to > > protect the struct devfreq state, while the devfreq_list_lock > > is apparently protecting the two lists (which seem unrelated > > lists).
Correct. devfreq->lock protects an instance of devfreq. devfreq_list_lock protects the global devfreq data (list of devfreq / governors) > > > > So, the two locks are not correlated. > > > > Regards, > > Eze > In governor_store(), we do 'df->governor = governor;' without taking > df->lock. So it is possible to switch governor while update_devfreq() is > in progress. Yup. that's possible. > I smell trouble there. Don't you think so? > I am assuming df->lock protects 'struct devfreq' and devfreq_list_lock > protects both device and governor lists. devfreq_list_lock is not supposed to protect a device. Assuming a memory read of a word is atomic (I'm not aware of one that's not unless in a case where the address is unaligned in some archtectures), update_devfreq won't cause such issues because it reads "devfreq->governor" only one in its execution except for the null check. Thus, if there could be an error, it'd be a case where someone else is doing "devfreq->governor = NULL" without devfreq->lock. And, find_devfreq_governor() does not return NULL. Cheers, MyungJoo > > -Akhil. >
