On Mon, May 11, 2009 at 2:53 AM, Johannes Berg
<johannes@xxxxxxxxxxxxxxxx> wrote:
> On Mon, 2009-05-11 at 05:25 -0400, Luis R. Rodriguez wrote:
>
>> /**
>> + * enum ieee80211_device_stop_reason - device stop reasons
>> + *
>> + * These are reasons why mac80211 would call the driver's stop()
>> + * callback. Devices should probably not care other than when
>> + * we call to stop during suspend, in these cases the device may
>> + * want to leave the radio on for WoW events for example.
>> + * @IEEE80211_DEV_STOP_INVALID_MAC: invalid mac address was detected for device
>> + * @IEEE80211_DEV_STOP_NO_OPEN_DEV: we were not able to open any
>> + netdevice for the current wireless device during user initialization.
>> + * @IEEE80211_DEV_STOP_DEV_CLOSE_REQUEST: user requested the device to be
>> + * closed.
>> + * @IEEE80211_DEV_STOP_SUSPEND: we are going to suspend
>> + */
>> +enum ieee80211_device_stop_reasons {
>> + IEEE80211_DEV_STOP_INVALID_MAC,
>> + IEEE80211_DEV_STOP_NO_OPEN_DEV,
>> + IEEE80211_DEV_STOP_DEV_CLOSE_REQUEST,
>> + IEEE80211_DEV_STOP_SUSPEND,
>> +};
>
>> +struct ieee80211_device_stop_info {
>> + enum ieee80211_device_stop_reasons reason;
>> +};
>
> Hmm. You're creating a code-flow API rather than a do-what-I-want API, I
> think it leaves the driver too much choice ;)
>
> The way you're using this here is:
>
> +#ifdef CONFIG_PM
> + if (sc->wow_enable &&
> + stop_info->reason == IEEE80211_DEV_STOP_SUSPEND) {
>
> So for one, all the other reasons are unnecessary.
Figured you'd want to nuke the rest, ok, then lets just have a bool for suspend.
> Additionally, as you
> said, you need to stay associated for some WoW modes, so mac80211 needs
> to be aware of that when going into suspend. It might even need to be
> aware of things like the computer waking up only to cycle keys.
>
> Therefore, IMHO the stop info struct (I would drop _info from the name,
> seems not very useful and makes the struct name very long) should
> contain the WoW parameters so that devices can program those parameters
> instead of turning off completely.
Sure, so the reason I used a struct was so that we leave room for this
exact stuff to be added as you had recommended, I just wanted to get
WoW working before finishing the APIs but .. it seems we do need to
work with mac80211 anyway so I might as well just finish the WoW API
stuff..
> At suspend time, mac80211 would fill
> in those parameters, whereas it wouldn't do that at the other times the
> device is stopped.
OK thanks.
>
> Of course, that means adding a WOW_CAPABLE HW flag, and API to mac80211
> to program WOW, but that will gain us more uniformity.
Sure well I was thinking of making this a cfg80211 thing.
> Question is
> whether we want to use nl80211 or ethtool.
I think we want to use nl80211 -- there are some things which are
wireless-specific like beacon miss triggers. Plus it just seems silly
to have to have drivers implement two ethtool ops just for WoW -- or
have mac80211 do that (or cfg80211).
I'll just get this properly implemented in cfg80211 then and send a new RFC.
Luis
--
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
