diff mbox

[2/2] mwifiex: use get_random_mask_addr() helper

Message ID 1505720537-13362-1-git-send-email-gbhat@marvell.com (mailing list archive)
State Accepted
Commit e9a3846afaa4d2d44f33a6aa2dd919e481be599e
Delegated to: Kalle Valo
Headers show

Commit Message

Ganapathi Bhat Sept. 18, 2017, 7:42 a.m. UTC
Avoid calculating random MAC address in driver. Instead make
use of 'get_random_mask_addr()' function.

Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
---
 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Comments

Kalle Valo Sept. 18, 2017, 1:17 p.m. UTC | #1
Ganapathi Bhat <gbhat@marvell.com> writes:

> Avoid calculating random MAC address in driver. Instead make
> use of 'get_random_mask_addr()' function.
>
> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>

I don't see 1/2 anywhere. Did it get lost?
Ganapathi Bhat Sept. 18, 2017, 2:13 p.m. UTC | #2
Hi Kalle,
> 
> > Avoid calculating random MAC address in driver. Instead make use of
> > 'get_random_mask_addr()' function.
> >
> > Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> 
> I don't see 1/2 anywhere. Did it get lost?
Actually there is no 1/2. What I did is: 'git send-email'; CTRL + C (to correct a typo); and then tried sending it again. I think that created some problem here. Kindly let me know how to proceed. 

Thanks,
Ganapathi
> 
> --
> Kalle Valo
Kalle Valo Sept. 19, 2017, 2:30 p.m. UTC | #3
Ganapathi Bhat <gbhat@marvell.com> writes:

> Hi Kalle,
>> 
>> > Avoid calculating random MAC address in driver. Instead make use of
>> > 'get_random_mask_addr()' function.
>> >
>> > Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
>> 
>> I don't see 1/2 anywhere. Did it get lost?
>
> Actually there is no 1/2. What I did is: 'git send-email'; CTRL + C
> (to correct a typo); and then tried sending it again. I think that
> created some problem here. Kindly let me know how to proceed.

Ok. I'll wait for review comments and if all goes well I'll apply it in
few days.
Brian Norris Sept. 19, 2017, 4:43 p.m. UTC | #4
Hi,

On Tue, Sep 19, 2017 at 05:30:06PM +0300, Kalle Valo wrote:
> Ganapathi Bhat <gbhat@marvell.com> writes:
> 
> > Hi Kalle,
> >> 
> >> > Avoid calculating random MAC address in driver. Instead make use of
> >> > 'get_random_mask_addr()' function.
> >> >
> >> > Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> >> 
> >> I don't see 1/2 anywhere. Did it get lost?
> >
> > Actually there is no 1/2. What I did is: 'git send-email'; CTRL + C

It's dependent on this patch though, which kinda should be '1/2':

[PATCH] mwifiex: avoid storing random_mac in private

> > (to correct a typo); and then tried sending it again. I think that
> > created some problem here. Kindly let me know how to proceed.
> 
> Ok. I'll wait for review comments and if all goes well I'll apply it in
> few days.

FWIW, this looks OK to me:

Reviewed-by: Brian Norris <briannorris@chromium.org>

It's just a bit strange that we have to keep our own on-stack temporary
buffer for this. Maybe this could use an in-place helper too? Or (if
it's really legal for us to modify the cfg80211_scan_request in-place)
why doesn't the upper-layer nl80211 code do the randomization for us?
Many (all?) drivers I see implementing randomization have to do this
anyway; they don't use request->mac_addr directly. (Or I suppose some
firmware could implement the randomization on its own someday...but
would we really trust it?)

Brian
Kalle Valo Sept. 20, 2017, 11:47 a.m. UTC | #5
Brian Norris <briannorris@chromium.org> writes:

> Hi,
>
> On Tue, Sep 19, 2017 at 05:30:06PM +0300, Kalle Valo wrote:
>> Ganapathi Bhat <gbhat@marvell.com> writes:
>> 
>> > Hi Kalle,
>> >> 
>> >> > Avoid calculating random MAC address in driver. Instead make use of
>> >> > 'get_random_mask_addr()' function.
>> >> >
>> >> > Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
>> >> 
>> >> I don't see 1/2 anywhere. Did it get lost?
>> >
>> > Actually there is no 1/2. What I did is: 'git send-email'; CTRL + C
>
> It's dependent on this patch though, which kinda should be '1/2':
>
> [PATCH] mwifiex: avoid storing random_mac in private

Thanks for pointing out, I'll make sure that I commit these in correct
order.

>> > (to correct a typo); and then tried sending it again. I think that
>> > created some problem here. Kindly let me know how to proceed.
>> 
>> Ok. I'll wait for review comments and if all goes well I'll apply it in
>> few days.
>
> FWIW, this looks OK to me:
>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
>
> It's just a bit strange that we have to keep our own on-stack temporary
> buffer for this. Maybe this could use an in-place helper too? Or (if
> it's really legal for us to modify the cfg80211_scan_request in-place)
> why doesn't the upper-layer nl80211 code do the randomization for us?
> Many (all?) drivers I see implementing randomization have to do this
> anyway; they don't use request->mac_addr directly. (Or I suppose some
> firmware could implement the randomization on its own someday...but
> would we really trust it?)

Good questions and I was wondering the same when looking at this patch.
But I wasn't involved in the interface design so I don't really know the
background here.

I'm planning to apply this patch anyway, any improvements can be done as
a followup patch.
Kalle Valo Sept. 20, 2017, 12:49 p.m. UTC | #6
Ganapathi Bhat <gbhat@marvell.com> wrote:

> Avoid calculating random MAC address in driver. Instead make
> use of 'get_random_mask_addr()' function.
> 
> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> Reviewed-by: Brian Norris <briannorris@chromium.org>

Patch applied to wireless-drivers-next.git, thanks.

e9a3846afaa4 mwifiex: use get_random_mask_addr() helper
diff mbox

Patch

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 84c1518..2b293b1 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -2503,6 +2503,7 @@  static int mwifiex_set_ibss_params(struct mwifiex_private *priv,
 	struct ieee80211_channel *chan;
 	struct ieee_types_header *ie;
 	struct mwifiex_user_scan_cfg *user_scan_cfg;
+	u8 mac_addr[ETH_ALEN];
 
 	mwifiex_dbg(priv->adapter, CMD,
 		    "info: received scan request on %s\n", dev->name);
@@ -2529,12 +2530,10 @@  static int mwifiex_set_ibss_params(struct mwifiex_private *priv,
 	priv->scan_request = request;
 
 	if (request->flags & NL80211_SCAN_FLAG_RANDOM_ADDR) {
-		for (i = 0; i < ETH_ALEN; i++) {
-			request->mac_addr[i] &= request->mac_addr_mask[i];
-			request->mac_addr[i] |=
-				get_random_int() & ~(request->mac_addr_mask[i]);
-		}
-		ether_addr_copy(user_scan_cfg->random_mac, request->mac_addr);
+		get_random_mask_addr(mac_addr, request->mac_addr,
+				     request->mac_addr_mask);
+		ether_addr_copy(request->mac_addr, mac_addr);
+		ether_addr_copy(user_scan_cfg->random_mac, mac_addr);
 	}
 
 	user_scan_cfg->num_ssids = request->n_ssids;