On Mon, Jan 10, 2011 at 5:44 PM, Luciano Coelho <coelho@xxxxxx> wrote:
> On Mon, 2011-01-03 at 14:42 +0100, Shahar Levi wrote:
>> Add new ampdu_action ops to support receiver BA.
>> The BA initiator session management in FW independently.
>>
>> Signed-off-by: Shahar Levi <shahar_levi@xxxxxx>
>> ---
>
> Some comments.
Thanks for your review.
>
>
>
>> diff --git a/drivers/net/wireless/wl12xx/acx.c b/drivers/net/wireless/wl12xx/acx.c
>> index 54fd68d..f33ab50 100644
>> --- a/drivers/net/wireless/wl12xx/acx.c
>> +++ b/drivers/net/wireless/wl12xx/acx.c
>> @@ -1359,6 +1359,42 @@ out:
>> return ret;
>> }
>>
>> +/* setup BA session receiver setting in the FW. */
>> +int wl1271_acx_set_ba_receiver_session(struct wl1271 *wl, u8 tid_index,
>> + u16 *ssn, u8 policy)
>
> You don't modify ssn here, so why pass it as a pointer? Use u16 directly
> here instead.
>
> Actually it's even worse. As stated in mac80211.h, ssn can be NULL
> here, so you would be accessing a NULL pointer in that case.
>
> I see that you check "policy", which indicates whether ssn is valid or
> not, but why not make it cleaner by checking if ssn is NULL and setting
> it to zero before passing instead?
good chtch. it was left from previous implantation. will be fix.
>
>
>> + acx->enable = policy;
>
> Also, because of this, it would make more sense if policy was a boolean
> and was called "enable" instead.
np, will be fix
>
>
>> diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
>> index c44462d..04b69ab 100644
>> --- a/drivers/net/wireless/wl12xx/main.c
>> +++ b/drivers/net/wireless/wl12xx/main.c
>> @@ -2251,6 +2251,64 @@ static int wl1271_op_get_survey(struct ieee80211_hw *hw, int idx,
>> return 0;
>> }
>>
>> +int wl1271_op_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>> + enum ieee80211_ampdu_mlme_action action,
>> + struct ieee80211_sta *sta, u16 tid, u16 *ssn)
>> +{
>> + struct wl1271 *wl = hw->priv;
>> + int ret;
>> +
>> + mutex_lock(&wl->mutex);
>> +
>> + if (unlikely(wl->state == WL1271_STATE_OFF)) {
>> + ret = -EAGAIN;
>> + goto out;
>> + }
>> +
>> + ret = wl1271_ps_elp_wakeup(wl, false);
>> + if (ret < 0)
>> + goto out;
>> +
>> + switch (action) {
>> + case IEEE80211_AMPDU_RX_START:
>> + if ((wl->ba_allowed) && (wl->ba_support)) {
>
> I'm a bit confused. What is ba_allowed, actually? In the previous patch
> you always call wl1271_set_ba_policies() which always sets it to true.
> So it seems quite useless.
ba_allowed belong to next path "Stop BA session event from device". i
will move it.
>
>
>> + ret = wl1271_acx_set_ba_receiver_session(wl, tid, ssn,
>> + true);
>> + if (!ret)
>> + wl->ba_rx_bitmap |= BIT(tid);
>> + } else
>> + ret = -EPERM;
>
> The indentation is wrong here. And -EPERM is definitely not the right
> return code for this. It should probably -ENOTSUPP or something like
> that.
any error will be good. i go on "Operation not permitted". will be fix.
>
>> + /*
>> + * The BA initiator session management in FW independently.
>> + * Falling break here on purpose for all TX APDU commands.
>> + */
>> + case IEEE80211_AMPDU_TX_START:
>> + case IEEE80211_AMPDU_TX_STOP:
>> + case IEEE80211_AMPDU_TX_OPERATIONAL:
>> + ret = -EINVAL;
>> + break;
>
> -EINVAL? Actually, should mac80211 even call us with these actions? I
> don't remember now, did you implement the way to prevent mac80211 from
> trying to to AMPDU_TX by itself?
the mac souldn't call those due to the fact the rate management is in the FW.
>
>
>> +
>> + default:
>> + wl1271_error("Incorrect ampdu action id=%x\n", action);
>> + ret = -ENODEV;
>
> -ENODEV? All these error codes look quite random to me. Can you explain
> your choices?
amm, i am trying to remeber why i use that one without success. i will
switch it to EINVAL.
>
>
> --
> Cheers,
> Luca.
Shahar
--
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
