diff mbox

nl80211: Don't call linux_iface_up() for a dedicated P2P Device

Message ID 1451219771-10978-1-git-send-email-ilan.peer@intel.com
State Accepted
Headers show

Commit Message

Peer, Ilan Dec. 27, 2015, 12:36 p.m. UTC
As a dedicated P2P Device interface does not have a network
interface associated with it, trying to call linux_iface_up()
on it would always fail so this call can be skipped for
such an interface.

Getting interface nlmode can be done only after bss->wdev_id is
set, so move this call to wpa_driver_nl80211_finish_drv_init(),
and do it only in case the nlmode != NL80211_IFTYPE_P2P_DEVICE.

Signed-off-by: Ilan Peer <ilan.peer@intel.com>
---
 src/drivers/driver_nl80211.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Jouni Malinen Dec. 28, 2015, 2:04 p.m. UTC | #1
On Sun, Dec 27, 2015 at 02:36:11PM +0200, Ilan Peer wrote:
> As a dedicated P2P Device interface does not have a network
> interface associated with it, trying to call linux_iface_up()
> on it would always fail so this call can be skipped for
> such an interface.
> 
> Getting interface nlmode can be done only after bss->wdev_id is
> set, so move this call to wpa_driver_nl80211_finish_drv_init(),
> and do it only in case the nlmode != NL80211_IFTYPE_P2P_DEVICE.

Does this need to be done after the wpa_driver_nl80211_capa() call here?

> @@ -2243,6 +2240,11 @@ wpa_driver_nl80211_finish_drv_init(struct wpa_driver_nl80211_data *drv,
>  	if (wpa_driver_nl80211_capa(drv))
>  		return -1;
>  
> +	if (first &&
> +	    nl80211_get_ifmode(bss) != NL80211_IFTYPE_P2P_DEVICE &&
> +	    linux_iface_up(drv->global->ioctl_sock, bss->ifname) > 0)
> +		drv->start_iface_up = 1;

The sequence used here would be different for fetching driver
capabilities compared to what was used previously (i.e., the interface
would now be down while it was up previously). I'd rather avoid such a
change here (i.e., reorder these two calls with
wpa_driver_nl80211_finish_drv_init()) unless this nl80211_get_ifmode()
part is not expected to work here without the driver capabilities
fetched. If this specific sequence is needed, I'd like to get better
understand on whether this could break any existing use cases (i.e.,
whether there is a driver that cannot return correct capabilities
without the interface being UP first). If I understood correctly, this
new patch does not really fix anything other than maybe making the debug
log cleaner (and well, if it does fix something, the commit log should
be modified to say so).
Peer, Ilan Dec. 28, 2015, 2:47 p.m. UTC | #2
> -----Original Message-----
> From: Jouni Malinen [mailto:j@w1.fi]
> Sent: Monday, December 28, 2015 16:05
> To: Peer, Ilan
> Cc: hostap@lists.infradead.org
> Subject: Re: [PATCH] nl80211: Don't call linux_iface_up() for a dedicated P2P
> Device
> 
> On Sun, Dec 27, 2015 at 02:36:11PM +0200, Ilan Peer wrote:
> > As a dedicated P2P Device interface does not have a network interface
> > associated with it, trying to call linux_iface_up() on it would always
> > fail so this call can be skipped for such an interface.
> >
> > Getting interface nlmode can be done only after bss->wdev_id is set,
> > so move this call to wpa_driver_nl80211_finish_drv_init(),
> > and do it only in case the nlmode != NL80211_IFTYPE_P2P_DEVICE.
> 
> Does this need to be done after the wpa_driver_nl80211_capa() call here?

No. The important thing is to have wdev_id set, so it can be used in nl80211_cmd_msg(). 

> 
> > @@ -2243,6 +2240,11 @@ wpa_driver_nl80211_finish_drv_init(struct
> wpa_driver_nl80211_data *drv,
> >  	if (wpa_driver_nl80211_capa(drv))
> >  		return -1;
> >
> > +	if (first &&
> > +	    nl80211_get_ifmode(bss) != NL80211_IFTYPE_P2P_DEVICE &&
> > +	    linux_iface_up(drv->global->ioctl_sock, bss->ifname) > 0)
> > +		drv->start_iface_up = 1;
> 
> The sequence used here would be different for fetching driver capabilities
> compared to what was used previously (i.e., the interface would now be down
> while it was up previously). I'd rather avoid such a change here (i.e., reorder
> these two calls with
> wpa_driver_nl80211_finish_drv_init()) unless this nl80211_get_ifmode() part
> is not expected to work here without the driver capabilities fetched. If this

This should still work as expected as AFAIU the only calls command that are used before it this 
flow are ones that do not require the interface to be up. But as stated above this call can be moved
before querying the capabilities.

> specific sequence is needed, I'd like to get better understand on whether this
> could break any existing use cases (i.e., whether there is a driver that cannot
> return correct capabilities without the interface being UP first). If I
> understood correctly, this new patch does not really fix anything other than
> maybe making the debug log cleaner (and well, if it does fix something, the
> commit log should be modified to say so).

This is only log cleanup. 

Should I send a fixed patch that move the lines before querying the capabilities? 

Thanks,

Ilan.
Jouni Malinen Dec. 28, 2015, 3:06 p.m. UTC | #3
On Mon, Dec 28, 2015 at 02:47:55PM +0000, Peer, Ilan wrote:
> > From: Jouni Malinen [mailto:j@w1.fi]
> > Does this need to be done after the wpa_driver_nl80211_capa() call here?
> 
> No. The important thing is to have wdev_id set, so it can be used in nl80211_cmd_msg(). 

OK, thanks.

> Should I send a fixed patch that move the lines before querying the capabilities? 

No need, I'll move it when applying the current patch.
Jouni Malinen Dec. 28, 2015, 4:56 p.m. UTC | #4
On Sun, Dec 27, 2015 at 02:36:11PM +0200, Ilan Peer wrote:
> As a dedicated P2P Device interface does not have a network
> interface associated with it, trying to call linux_iface_up()
> on it would always fail so this call can be skipped for
> such an interface.
> 
> Getting interface nlmode can be done only after bss->wdev_id is
> set, so move this call to wpa_driver_nl80211_finish_drv_init(),
> and do it only in case the nlmode != NL80211_IFTYPE_P2P_DEVICE.

Thanks, applied.
diff mbox

Patch

diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
index ed5e4a8..b3d56b2 100644
--- a/src/drivers/driver_nl80211.c
+++ b/src/drivers/driver_nl80211.c
@@ -1735,9 +1735,6 @@  static void * wpa_driver_nl80211_drv_init(void *ctx, const char *ifname,
 	if (nl80211_init_bss(bss))
 		goto failed;
 
-	if (linux_iface_up(drv->global->ioctl_sock, ifname) > 0)
-		drv->start_iface_up = 1;
-
 	if (wpa_driver_nl80211_finish_drv_init(drv, set_addr, 1, driver_params))
 		goto failed;
 
@@ -2243,6 +2240,11 @@  wpa_driver_nl80211_finish_drv_init(struct wpa_driver_nl80211_data *drv,
 	if (wpa_driver_nl80211_capa(drv))
 		return -1;
 
+	if (first &&
+	    nl80211_get_ifmode(bss) != NL80211_IFTYPE_P2P_DEVICE &&
+	    linux_iface_up(drv->global->ioctl_sock, bss->ifname) > 0)
+		drv->start_iface_up = 1;
+
 	if (driver_params && nl80211_set_param(bss, driver_params) < 0)
 		return -1;