diff mbox

[PATCHv2,3/4] brcm80211: make mgmt_tx in brcmfmac accept a NULL channel

Message ID 1370433187-1337-3-git-send-email-ordex@autistici.org (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Antonio Quartulli June 5, 2013, 11:53 a.m. UTC 
From: Antonio Quartulli <antonio@open-mesh.com>

cfg80211 passes a NULL channel to mgmt_tx if the frame has
to be sent on the one currently in use by the device.
Make the implementation of mgmt_tx correctly handle this
case

Cc: Arend van Spriel <arend@broadcom.com>
Cc: brcm80211-dev-list@broadcom.com
Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Arend van Spriel June 5, 2013, 1:53 p.m. UTC | #1 
On 06/05/2013 01:53 PM, Antonio Quartulli wrote:
> From: Antonio Quartulli <antonio@open-mesh.com>
>
> cfg80211 passes a NULL channel to mgmt_tx if the frame has
> to be sent on the one currently in use by the device.
> Make the implementation of mgmt_tx correctly handle this
> case
>
> Cc: Arend van Spriel <arend@broadcom.com>
> Cc: brcm80211-dev-list@broadcom.com
> Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
> ---
>   drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
> index 6d758f2..dcb0c00 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
> @@ -3917,6 +3917,7 @@ brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>   	struct brcmf_fil_af_params_le *af_params;
>   	bool ack;
>   	s32 chan_nr;
> +	u32 freq;
>
>   	brcmf_dbg(TRACE, "Enter\n");
>
> @@ -3968,8 +3969,13 @@ brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>   		memcpy(&af_params->bssid[0], &mgmt->bssid[0], ETH_ALEN);
>   		/* Add the length exepted for 802.11 header  */
>   		action_frame->len = cpu_to_le16(len - DOT11_MGMT_HDR_LEN);
> -		/* Add the channel */
> -		chan_nr = ieee80211_frequency_to_channel(chan->center_freq);
> +		/* Add the channel. Default to the current one, but use the one
> +		 * specified as parameter if any
> +		 */
> +		freq = cfg->channel;
> +		if (chan)
> +			freq = chan->center_freq;
> +		chan_nr = ieee80211_frequency_to_channel(freq);

Could you get rid of 'freq' variable and use 
ieee80211_frequency_to_channel() on cfg->channel or chan->center_freq.

Another thing is that cfg->channel can be zero resulting in chan_nr 
being zero. I had a quick look whether the device firmware can handle 
this. I suspect not, but I will need to ask to be sure.

Regards,
Arend


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Antonio Quartulli June 5, 2013, 1:57 p.m. UTC | #2 
On Wed, Jun 05, 2013 at 06:53:32AM -0700, Arend van Spriel wrote:

[...]

> > +		freq = cfg->channel;
> > +		if (chan)
> > +			freq = chan->center_freq;
> > +		chan_nr = ieee80211_frequency_to_channel(freq);
> 
> Could you get rid of 'freq' variable and use 
> ieee80211_frequency_to_channel() on cfg->channel or chan->center_freq.
> 

I tried that, but the line indented below the if would be longer than 80 chars
and breaking it is quite ugly.

> Another thing is that cfg->channel can be zero resulting in chan_nr 
> being zero. I had a quick look whether the device firmware can handle 
> this. I suspect not, but I will need to ask to be sure.
> 

ok. But when cfg->channel is zero, where is the current freq stored?

Regards,
Arend van Spriel June 5, 2013, 2:14 p.m. UTC | #3 
On 06/05/2013 03:57 PM, Antonio Quartulli wrote:
> On Wed, Jun 05, 2013 at 06:53:32AM -0700, Arend van Spriel wrote:
>
> [...]
>
>>> +		freq = cfg->channel;
>>> +		if (chan)
>>> +			freq = chan->center_freq;
>>> +		chan_nr = ieee80211_frequency_to_channel(freq);
>>
>> Could you get rid of 'freq' variable and use
>> ieee80211_frequency_to_channel() on cfg->channel or chan->center_freq.
>>
>
> I tried that, but the line indented below the if would be longer than 80 chars
> and breaking it is quite ugly.
>
>> Another thing is that cfg->channel can be zero resulting in chan_nr
>> being zero. I had a quick look whether the device firmware can handle
>> this. I suspect not, but I will need to ask to be sure.
>>
>
> ok. But when cfg->channel is zero, where is the current freq stored?

It is not. The rf on the device is set to a certain channel so it can be 
retrieved from the device:

brcmf_fil_cmd_int_get(vif->ifp, BRCMF_C_GET_CHANNEL, &freq);

The vif pointer can be obtained using container_of() on the wdev. That 
is done in mgmt_tx() already some lines above. May you should move 
determining the vif pointer and do it unconditional.

Regards,
Arend


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Antonio Quartulli June 5, 2013, 2:17 p.m. UTC | #4 
On Wed, Jun 05, 2013 at 07:14:07AM -0700, Arend van Spriel wrote:
> On 06/05/2013 03:57 PM, Antonio Quartulli wrote:
> > On Wed, Jun 05, 2013 at 06:53:32AM -0700, Arend van Spriel wrote:
> >
> > [...]
> >
> >>> +		freq = cfg->channel;
> >>> +		if (chan)
> >>> +			freq = chan->center_freq;
> >>> +		chan_nr = ieee80211_frequency_to_channel(freq);
> >>
> >> Could you get rid of 'freq' variable and use
> >> ieee80211_frequency_to_channel() on cfg->channel or chan->center_freq.
> >>
> >
> > I tried that, but the line indented below the if would be longer than 80 chars
> > and breaking it is quite ugly.
> >
> >> Another thing is that cfg->channel can be zero resulting in chan_nr
> >> being zero. I had a quick look whether the device firmware can handle
> >> this. I suspect not, but I will need to ask to be sure.
> >>
> >
> > ok. But when cfg->channel is zero, where is the current freq stored?
> 
> It is not. The rf on the device is set to a certain channel so it can be 
> retrieved from the device:
> 
> brcmf_fil_cmd_int_get(vif->ifp, BRCMF_C_GET_CHANNEL, &freq);
> 
> The vif pointer can be obtained using container_of() on the wdev. That 
> is done in mgmt_tx() already some lines above. May you should move 
> determining the vif pointer and do it unconditional.
> 

Yeah, I like this approach.

I'll send v3 of this patchset with this change in it.
@johannes: I'll also revert the order of the patches.

Thanks a lot Arend.

Regards,
diff mbox

Patch

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index 6d758f2..dcb0c00 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -3917,6 +3917,7 @@  brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
 	struct brcmf_fil_af_params_le *af_params;
 	bool ack;
 	s32 chan_nr;
+	u32 freq;
 
 	brcmf_dbg(TRACE, "Enter\n");
 
@@ -3968,8 +3969,13 @@  brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
 		memcpy(&af_params->bssid[0], &mgmt->bssid[0], ETH_ALEN);
 		/* Add the length exepted for 802.11 header  */
 		action_frame->len = cpu_to_le16(len - DOT11_MGMT_HDR_LEN);
-		/* Add the channel */
-		chan_nr = ieee80211_frequency_to_channel(chan->center_freq);
+		/* Add the channel. Default to the current one, but use the one
+		 * specified as parameter if any
+		 */
+		freq = cfg->channel;
+		if (chan)
+			freq = chan->center_freq;
+		chan_nr = ieee80211_frequency_to_channel(freq);
 		af_params->channel = cpu_to_le32(chan_nr);
 
 		memcpy(action_frame->data, &buf[DOT11_MGMT_HDR_LEN],