On 8 October 2014 12:48, Rajkumar Manoharan <rmanohar@xxxxxxxxxxxxxxxx> wrote:
> On Wed, Oct 08, 2014 at 12:52:04PM +0300, Kalle Valo wrote:
>> Rajkumar Manoharan <rmanohar@xxxxxxxxxxxxxxxx> writes:
>>
>> > The commit "ath10k: workaround fw beaconing bug" is freeing
>> > DMA-coherent memory in irq context which is hitting BUG ON
>> > in ARM platforms. Fix this by moving dma_free out of spin
>> > lock.
>
>> [...]
>>
>> > @@ -2404,8 +2389,15 @@ void ath10k_halt(struct ath10k *ar)
>> >
>> > spin_lock_bh(&ar->data_lock);
>> > list_for_each_entry(arvif, &ar->arvifs, list)
>> > - ath10k_mac_vif_beacon_cleanup(arvif);
>> > + ath10k_mac_vif_beacon_free(arvif);
>> > spin_unlock_bh(&ar->data_lock);
>> > + list_for_each_entry(arvif, &ar->arvifs, list) {
>> > + if (!arvif->beacon_buf)
>> > + continue;
>> > + dma_free_coherent(ar->dev, IEEE80211_MAX_FRAME_LEN,
>> > + arvif->beacon_buf, arvif->beacon_paddr);
>> > + arvif->beacon_buf = NULL;
>> > + }
>> > }
>>
>> Until now we have protected arvif->beacon_buf with data_lock. How do we
>> know that this is safe to do without taking data_lock?
>>
> As said, spin_lock can not be used for dma_free_coherent.
> arvif->beacon_buf is already protected by conf_mutex. At this state
> in ath10k_halt path, no one can access beacon_buf. So mutex lock itself
> is sufficient.
beacon_buf is protected by conf_mutex implicitly. It wasn't the main
intent. It is protected with data_lock spinlock.
Do not trust the device - if there's a spurious SWBA event while
ath10k_remove_interface() is running you could end up with invalid
memory access.
It might be acceptable to drop the spinlock for ath10k_halt() since
the device is guaranteed to be stopped at that point (effectively
reset) though.
Anyway I'm hoping this bug can be fixed with the gfp flag.
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
