On Sun, 2011-04-03 at 15:37 +0300, Guy Eilam wrote:
> Add tx_spare_blocks member to the wl1271 struct
> for more generic configuration of the amount
> of spare TX blocks that should be used.
> The default value is 1.
> in case GEM cipher is used by the STA, we need
> 2 spare TX blocks instead of just 1.
>
> Signed-off-by: Guy Eilam <guy@xxxxxxxxxx>
> ---
Looks good, but I have a couple of comments.
> diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> index 85cb4da..f962e43 100644
> --- a/drivers/net/wireless/wl12xx/main.c
> +++ b/drivers/net/wireless/wl12xx/main.c
[..]
> @@ -2039,6 +2043,17 @@ static int wl1271_set_key(struct wl1271 *wl, u16 action, u8 id, u8 key_type,
> 0xff, 0xff, 0xff, 0xff, 0xff, 0xff
> };
>
> + /*
> + * A STA set to GEM cipher requires 2 tx spare blocks.
> + * Return to default value when GEM cipher key is removed
> + */
> + if (key_type == KEY_GEM) {
> + if (action == KEY_ADD_OR_REPLACE)
> + wl->tx_spare_blocks = 2;
> + else
> + wl->tx_spare_blocks = TX_HW_BLOCK_SPARE_DEFAULT;
> + }
> +
This won't make a real difference in the code flow, but wouldn't it be
better to make it explicit that the "else" case is KEY_REMOVE? There is
also KEY_SET_ID, which is only used with WEP, so it doesn't matter now.
But I think it's more consistent to be clear about it.
> diff --git a/drivers/net/wireless/wl12xx/tx.c b/drivers/net/wireless/wl12xx/tx.c
> index db9e47e..2c79b6e 100644
> --- a/drivers/net/wireless/wl12xx/tx.c
> +++ b/drivers/net/wireless/wl12xx/tx.c
> @@ -135,12 +135,10 @@ static int wl1271_tx_allocate(struct wl1271 *wl, struct sk_buff *skb, u32 extra,
> u32 len;
> u32 total_blocks;
> int id, ret = -EBUSY;
> - u32 spare_blocks;
> + u32 spare_blocks = wl->tx_spare_blocks;
>
> if (unlikely(wl->quirks & WL12XX_QUIRK_USE_2_SPARE_BLOCKS))
> spare_blocks = 2;
> - else
> - spare_blocks = 1;
Do we still need the quirk now? Wouldn't it be nicer to change the
wl->tx_spare_blocks value directly instead?
--
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
