On Fri, 2010-12-10 at 21:15 +0100, ext Johannes Berg wrote:
> See ... I see nothing here in main.c that would set the wiphy flag, so
> you really should've checked it in cfg8021 :-)
Heheh! Well, as we discussed on IRC, I had set the wiphy flag directly
in the driver code. But as you recommended, I have now changed it so
that mac80211 will check if the driver has an op->sched_scan_start and
set the flag accordingly.
> > +struct ieee80211_sched_scan_ies {
> > + u8 *ie[IEEE80211_NUM_BANDS];
>
> const?
As discussed on IRC, in this case this doesn't need to be const. This
struct belongs to mac80211 and it needs to write to it when preparing
the sched_scan, so no need to const it and cast back to non-const when
assigning values to it. There no need to restrict the access to it from
the driver's point-of-view. In normal cases, the driver should not
write to the IEs, but there's no reason to prevent it from doing it.
> > + * @sched_scan_start: Ask the hardware to start scanning repeatedly at
> > + * specific intervals. The driver must call the
> > + * ieee80211_sched_scan_results() function whenever it finds results.
> > + * This process will continue until sched_scan_stop is called.
> > + *
> > + * @sched_scan_stop: Tell the hardware to stop an ongoing periodic scan.
> > + *
> > + * ieee80211_sched_scan_results() each time it finds some results.
>
> I think that should talk about filtering as well? Maybe a DOC: section
> would be good, dunno.
As discussed in your comments to the previous patch, I decided to leave
the filtering out for now and will add it later with a separate patch,
to keep things simple.
There was this extra "ieee80211_sched_scan_results..." pasted wrongly
there, which I have removed.
> > /**
> > + * ieee80211_sched_scan_results - got results from periodic scan
> > + *
> > + * When a periodic scan is running, this function needs to be called by the
> > + * driver whenever there are new scan results availble.
>
> typo: available
Fixed.
> > +TRACE_EVENT(drv_sched_scan_results,
> > + TP_PROTO(struct ieee80211_local *local),
> > +
> > + TP_ARGS(local),
> > +
> > + TP_STRUCT__entry(
> > + LOCAL_ENTRY
> > + ),
> > +
> > + TP_fast_assign(
> > + LOCAL_ASSIGN;
> > + ),
> > +
> > + TP_printk(
> > + LOCAL_PR_FMT, LOCAL_PR_ARG
> > + )
> > +);
>
> Shouldn't that be in the _api_ section?
Indeed. In fact this trace was not even used anywhere. I'll move it to
the api_ section and call it properly.
> > @@ -642,6 +642,7 @@ enum queue_stop_reason {
> > * that the scan completed.
> > * @SCAN_ABORTED: Set for our scan work function when the driver reported
> > * a scan complete for an aborted scan.
> > + * @SCAN_SCHED_SCANNING: We're currently performing periodic scans
>
> That reminds me ... can you scan and sched_scan at the same time?
> sched_scan while associated? Should these be prohibited, or documented
> as being implementation dependent?
With the wl12xx firmware, you can scan and sched_scan at the same time.
In theory, I haven't tried it very thoroughly. It doesn't support
sched_scan while associated, yet. But I think it's a good feature, eg.
for roaming, and we shouldn't restrict it in the mac80211.
I think the best is to document that it is implementation dependent.
And again, for the record, I'll implement a NL80211_SCHED_SCAN_STOPPED
event that the driver can send to userspace at any time when the
sched_scan is running. By doing so, we allow drivers that need to stop
the scan in certain scenarios (eg. while associating or starting a
one-shot scan) to inform the userspace, which then decides whether to
restart it or not.
Maybe we just mandate that sched_scan must work when idle. In other
cases the driver can always return -EBUSY, for instance if it doesn't
support sched_scan while associated.
> Also, how does this interact with IDLE? Obviously, the device won't be
> idle with this, but you still want it to be "otherwise" idle, no? Should
> we take this out of scanning flags and specify that it must be supported
> while the device is told that it's idle by mac80211? Do you expect this
> to be stopped before trying to associate?
The last question is easy to answer: see above. :)
About idle... I have made the assumption that we will consider
sched_scanning as idle, so mac80211 is not calling config() with
IEEE80211_CONF_CHANGE_IDLE when starting or stopping the sched_scan (it
checks local->scan_data when recalculating idle).
But indeed, now checking it more carefully, I can see that the scanning
flags are checked in many different places. I guess the best thing to
do is to take the sched_scan out of the scanning flags and check case by
case. Some kind of new state... we shouldn't suspend things while
sched_scan is running.
> > @@ -392,7 +392,8 @@ ieee80211_rx_h_passive_scan(struct ieee80211_rx_data *rx)
> > if (likely(!(status->rx_flags & IEEE80211_RX_IN_SCAN)))
> > return RX_CONTINUE;
> >
> > - if (test_bit(SCAN_HW_SCANNING, &local->scanning))
> > + if (test_bit(SCAN_HW_SCANNING, &local->scanning) ||
> > + test_bit(SCAN_SCHED_SCANNING, &local->scanning))
> > return ieee80211_scan_rx(rx->sdata, skb);
>
> This won't work while associated...
Damn, this is getting more complicated than I expected. As discussed,
this would eat all beacons during the entire duration of the sched_scan,
so association would break.
Can I change my mind and just forbid sched_scan while not idle? :D
No seriously, I'll continue thinking aboout how to solve this tomorrow.
> > + for (i = 0; i < IEEE80211_NUM_BANDS; i++) {
> > + local->sched_scan_ies.ie[i] = kzalloc(2 +
> > + IEEE80211_MAX_SSID_LEN +
> > + local->scan_ies_len,
> > + GFP_KERNEL);
>
>
> Oops ... how about if this allocation fails?
Oorgh! Fixed.
> > +void ieee80211_sched_scan_results(struct ieee80211_hw *hw)
> > +{
> > + struct ieee80211_local *local = hw_to_local(hw);
> > +
> > + mutex_lock(&local->mtx);
> > +
> > + cfg80211_sched_scan_results(hw->wiphy);
> > +
> > + mutex_unlock(&local->mtx);
>
> Does that really need locking? Seems ... pointless since cfg80211 will
> have to take care of its locking.
Indeed. Removed the locking here.
> Finally: how does this interact with HW reset? Should it be re-started
> if it was ever started?
As described earlier, we can rely on the NL80211_SCHED_SCAN_STOPPED
event, so the userspace may decided whether to restart the sched_scan or
not.
--
Cheers,
Luca.
--
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
