Re: [PATCH 1/3] ath10k: add phyerr/dfs handling

[Kalle Valo <kvalo@qca.qualcomm.com>]

Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:

> On 6 November 2013 10:47, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Marek Puzyniak <marek.puzyniak@tieto.com> writes:
>>
>>> +     struct ath10k *ar = file->private_data;
>>> +     char *buf;
>>> +
>>> +     buf = kzalloc(size, GFP_KERNEL);
>>> +     if (buf == NULL)
>>> +             return -ENOMEM;
>>> +
>>> +     if (!ar->dfs_detector) {
>>> +             len += scnprintf(buf + len, size - len, "DFS not enabled\n");
>>> +             goto exit;
>>> +     }
>>> +
>>> +     ar->debug.dfs_pool_stats = ar->dfs_detector->get_stats(ar->dfs_detector);
>>
>> I think we need to take conf_mutex to make sure ar->dfs_detector is not
>> destroyed while we use it.
>>
> We deregister dfs_detector in ath10k_mac_unregister() so we will first
> destroy debugfs, then we don't need any mutex here. BTW I see we don't
> call debugfs_remove*() - so, currently mac clear debugfs for us -
> ieee80211_unregister_hw() -> wiphy_unregister I think. After that we
> deregister dfs_detector.

I don't have the code at hand but yeah, that sounds sensible.

>>> +static void ath10k_dfs_radar_report(struct ath10k *ar,
>>> +                                 struct wmi_single_phyerr_rx_event *event,
>>> +                                 struct phyerr_radar_report *rr,
>>> +                                 u64 tsf)
>>> +{
>>> +     u32 reg0, reg1, tsf32l;
>>> +     struct pulse_event pe;
>>> +     u64 tsf64;
>>> +     u8 rssi, width;
>>
>> What about locking? Does this function assume that conf_mutex is held?
>> If yes, please document that with lockdep_assert_held(). If no, we have
>> a problem :)
>>
>> (Reads wmi.c)
>>
>> Ah, wmi events don't sleep anymore, forgot that. So we can't really use
>> conf_mutex here. I guess our options are bring back worker for wmi
>> events or use spinlock.
>
> I think we can use here spin_lock_bh(&ar->data_lock) when setting
> ar->debug.dfs_stats and when reading this from debugfs.
> But, is that really needed (in worst case we will get older values via debugfs)?

I would prefer not to have any race conditions in the driver, even if
it's just statistics. If there's only a race with statistics atomic
variables are also one option.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k