Re: [PATCH 2/2] ath10k: add spectral scan feature

[Simon Wunderlich <sw@simonwunderlich.de>]

> >> > +       arvif = ath10k_get_spectral_vdev(ar);
> >> > +       if (!arvif)
> >> > +               return -ENODEV;
> >> > +       vdev_id = arvif->vdev_id;
> >> > +
> >> > +       if (ar->spectral_mode == SPECTRAL_DISABLED)
> >> > +               return 0;
> >> > +
> >> > +       res = ath10k_wmi_vdev_spectral_enable(ar, vdev_id,
> >> > +
> >> > WMI_SPECTRAL_TRIGGER_CMD_CLEAR, +
> >> > 
> >> >      WMI_SPECTRAL_ENABLE_CMD_ENABLE); +       if (res < 0)
> >> > 
> >> > +               return res;
> >> > +
> >> > +       res = ath10k_wmi_vdev_spectral_enable(ar, vdev_id,
> >> > +
> >> > WMI_SPECTRAL_TRIGGER_CMD_TRIGGER, +
> >> > 
> >> >        WMI_SPECTRAL_ENABLE_CMD_ENABLE); +       if (res < 0)
> >> > 
> >> > +               return res;
> >> > +
> >> > +       return 0;
> >> > +}
> >> > +static int ath10k_spectral_scan_config(struct ath10k *ar,
> >> > +                                      enum ath10k_spectral_mode mode)
> >> > +{
> >> > +       struct wmi_vdev_spectral_conf_arg arg;
> >> > +       struct ath10k_vif *arvif;
> >> > +       int vdev_id, count, res = 0;
> >> > +
> >> 
> >> Ditto. lockdep_assert_held().
> > 
> > Actually both functions are calling ath10k_get_spectral_vdev() which
> > already has that assert, but it doesn't hurt to put it here as well.
> > Will do. :)
> 
> True but those functions (scan_config, scan_trigger) also access
> conf_mutex protected data themselves directly. If
> ath10k_get_spectral_vdev() was to be changed to not require conf_mutex
> you'd have to remember to move lockdep_assert_held() up the call
> stack.

Ah, that's right. I'll add that anyway. :)

> 
> >> Did you test this against a firmware crash while spectral scan is
> >> enabled? ar->spectral_mode will be left as-is when restart is
> >> performed but no spectral scan will be actually configured. You either
> >> need to restart spectral scan (better from user perspective) or clear
> >> out the old mode (easier to code, but bad from user perspective
> >> because suddenly spectral scan would be stopped implicitly by device
> >> crash).
> > 
> > No, I didn't test that against a firmware crash yet ...
> 
> There's 'simulate_fw_crash' in debugfs. I suggest 'hard' method.
> 'soft' works only with fw 636 now. Watch out for possible host lockups
> though.
> 

Thanks, the hard firmware crash worked for me.

> > Restarting spectral
> > would probably a good idea in endless mode, but not if "count" has been
> > set.
> 
> Hmm good point, but then what if you have count=20 and fw crashes in
> the middle of performing the spectral scan run (e.g. at iteration 10)?
> 
> But then this is just a debug interface (or is it?). I don't know if
> it's worth it going to great lengths to provide a super smooth user
> experience.

Well yeah, it's in debugfs so its a debug interface. ;) Since we don't really 
keep track whether a scan was triggered or not, and the firmware takes some 
time to recover, I don't think it's worth the hassle - without creating more 
confusion. I'll reset the spectral mode in any case so users at least see that 
its disabled.

I'll send the next round of patches in some minutes. :)

Thanks!
    Simon