On Tue, 2011-08-09 at 12:13 +0300, Eliad Peller wrote:
> +static u8 wl1271_get_role_type(struct wl1271 *wl)
> +{
> + switch (wl->bss_type) {
> + case BSS_TYPE_AP_BSS:
> + return WL1271_ROLE_AP;
> +
> + case BSS_TYPE_STA_BSS:
> + return WL1271_ROLE_STA;
> +
> + default:
> + wl1271_info("invalid bss_type: %d", wl->bss_type);
> + }
> + return 0xff;
> +}
> +
> static int wl1271_op_add_interface(struct ieee80211_hw *hw,
> struct ieee80211_vif *vif)
> {
> struct wl1271 *wl = hw->priv;
> struct wiphy *wiphy = hw->wiphy;
> int retries = WL1271_BOOT_RETRIES;
> @@ -1836,12 +1851,17 @@ static int wl1271_op_add_interface(struct ieee80211_hw *hw,
> goto power_off;
>
> ret = wl1271_boot(wl);
> if (ret < 0)
> goto power_off;
>
> + ret = wl1271_cmd_role_enable(wl, wl1271_get_role_type(wl),
> + &wl->role_id);
> + if (ret < 0)
> + goto irq_disable;
> +
This doesn't look right, because if the type is invalid, you will return
0xff and it will still be sent anyway to the firmware via
CMD_ROLE_ENABLE. It will _probably_ still work as expected, because the
firmware will hopefully return an error to that command and we will fail
the op_add_interface. But this will be ugly, because we'll print
"firmware boot failed", which is not true and so on.
Maybe you should add a role_type to wl (we will most likely need it in
the vif struct later anyway) or somehow merge this with the existing
bss_type. At least you could check the role type in the same "switch
(vif->type)".
> static void __wl1271_op_remove_interface(struct wl1271 *wl,
> bool reset_tx_queues)
> {
> + int ret;
>
> wl1271_debug(DEBUG_MAC80211, "mac80211 remove interface");
>
> /* because of hardware recovery, we may get here twice */
> if (wl->state != WL1271_STATE_ON)
> return;
> @@ -1924,12 +1945,29 @@ static void __wl1271_op_remove_interface(struct wl1271 *wl,
> wl->scan.state = WL1271_SCAN_STATE_IDLE;
> memset(wl->scan.scanned_ch, 0, sizeof(wl->scan.scanned_ch));
> wl->scan.req = NULL;
> ieee80211_scan_completed(wl->hw, true);
> }
>
> + if (!test_bit(WL1271_FLAG_RECOVERY_IN_PROGRESS, &wl->flags)) {
> + /* disable active roles */
> + ret = wl1271_ps_elp_wakeup(wl);
> + if (ret < 0) {
> + /*
> + * do nothing. we are going to power off the chip
> + * anyway. handle this case when we'll really
> + * support multi-role...
> + */
> + }
> + wl1271_cmd_role_disable(wl, &wl->role_id);
Okay, we can do nothing for now, but should we then at least skip the
wl1271_cmd_role_disable() call? I guess that will also fail if we could
not wake up...
--
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
