On 14 May 2014 21:50, Kalle Valo <kvalo@xxxxxxxxxxxxxxxx> wrote: > Michal Kazior <michal.kazior@xxxxxxxxx> writes: > >> All configuration sequences should be protected >> with conf_mutex to avoid concurrent/conflicting >> requests. >> >> This should make sure that wep tx key setup is not >> performed while hw is restarted (at least). > > Locks and mutexes should protect data, not thread of execution. What are > we exactly protecting here, arvif->def_wep_key_idx? Actually conf_mutex idea is to protect and serialize configuration sequences (WMI) too, not just data. Perhaps we should revise this? > The patch makes sense, I'm just trying to understand the idea behind the > implementation. > >> --- a/drivers/net/wireless/ath/ath10k/mac.c >> +++ b/drivers/net/wireless/ath/ath10k/mac.c >> @@ -1888,8 +1888,10 @@ static void ath10k_tx_wep_key_work(struct work_struct *work) >> wep_key_work); >> int ret, keyidx = arvif->def_wep_key_newidx; >> >> + mutex_lock(&arvif->ar->conf_mutex); >> + >> if (arvif->def_wep_key_idx == keyidx) >> - return; >> + goto unlock; > > BTW, shouldn't we also check the state and bail out if the state is not > ON (and possibly RESTARTED)? How can this work otherwise? Very good point! But actually.. this should be cancelled during recovery in the first place. I need to take a look at it. Michał -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
