| Message ID | 1360581347-26766-7-git-send-email-arend@broadcom.com |
|---|---|
| State | Superseded |
| Headers | show |
On 02/11/2013 12:15 PM, Arend van Spriel wrote: > From: David Spinadel <david.spinadel@intel.com> > > Don't try to assign ifindex for P2P device interface and > check that the ifindex is valid before trying to remove it. > > Don't try to set TX rates for P2P device interface since it's > not a network device. > > Signed-hostap: David Spinadel <david.spinadel@intel.com> > --- > src/drivers/driver_nl80211.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c > index f0306f1..183761a 100644 > --- a/src/drivers/driver_nl80211.c > +++ b/src/drivers/driver_nl80211.c > @@ -6051,6 +6051,13 @@ static int nl80211_create_iface_once(struct wpa_driver_nl80211_data *drv, > return ret; > } > For P2P_DEVICE an additional parameter should be given in the NEW_INTERFACE command to allow wpa_supplicant to determine the P2P device address: NLA_PUT(msg, NL80211_ATTR_MAC, ETH_ALEN, p2p_dev_addr); See wireless-next commit 1c18f1452a772dfe884ed25677bddb3ecaf9c43a Author: Arend van Spriel <arend@broadcom.com> Date: Tue Jan 8 10:17:27 2013 +0100 nl80211: allow user-space to set address for P2P_DEVICE > + if (iftype == NL80211_IFTYPE_P2P_DEVICE) { > + wpa_printf(MSG_DEBUG, > + "nl80211 New P2P device interface %s created", > + ifname); > + return 0; > + } > +
On Mon, 2013-02-11 at 12:53 +0100, Arend van Spriel wrote: > For P2P_DEVICE an additional parameter should be given in the > NEW_INTERFACE command to allow wpa_supplicant to determine the P2P > device address: > > NLA_PUT(msg, NL80211_ATTR_MAC, ETH_ALEN, p2p_dev_addr); That's not really necessary, unless you want to have a single P2P Device interface shared between multiple devices, no? johannes
On 02/11/2013 12:56 PM, Johannes Berg wrote: > On Mon, 2013-02-11 at 12:53 +0100, Arend van Spriel wrote: > >> For P2P_DEVICE an additional parameter should be given in the >> NEW_INTERFACE command to allow wpa_supplicant to determine the P2P >> device address: >> >> NLA_PUT(msg, NL80211_ATTR_MAC, ETH_ALEN, p2p_dev_addr); > > That's not really necessary, unless you want to have a single P2P Device > interface shared between multiple devices, no? Correct, but it seems to make sense to accommodate for it instead of doing another patch later on (although the patch went in for v3.9, right?). Just a comment, I leave it to David what to do with it. Gr. AvS
> > > >> For P2P_DEVICE an additional parameter should be given in the > >> NEW_INTERFACE command to allow wpa_supplicant to determine the > P2P > >> device address: > >> > >> NLA_PUT(msg, NL80211_ATTR_MAC, ETH_ALEN, p2p_dev_addr); > > > > That's not really necessary, unless you want to have a single P2P > > Device interface shared between multiple devices, no? > > Correct, but it seems to make sense to accommodate for it instead of doing > another patch later on (although the patch went in for v3.9, right?). Just a > comment, I leave it to David what to do with it. > I don't think it required, since wpa_supplicant has only one P2P state machine, I don't see a reason to deal with more than one P2P device. David --------------------------------------------------------------------- A member of the Intel Corporation group of companies This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
> > > That's not really necessary, unless you want to have a single P2P > > > Device interface shared between multiple devices, no? > > > > Correct, but it seems to make sense to accommodate for it instead of doing > > another patch later on (although the patch went in for v3.9, right?). Just a > > comment, I leave it to David what to do with it. > > > I don't think it required, since wpa_supplicant has only one P2P state > machine, I don't see a reason to deal with more than one P2P device. I think the idea behind it was that you could have a 2.4/5 GHz device and a 60 GHz device and have them both use the same P2P Device Address so you can use them interchangeably. However I'd actually argue that you should just create the second one with the same address as the first (and even creating a second one in cases like that will need a lot of work) so that you can get the device address assigned by the kernel? OTOH, if the kernel has no address, we'll probably have to allow changing it? This may need some work. johannes
+ Jouni On 02/13/2013 08:41 AM, Johannes Berg wrote: > >>>> That's not really necessary, unless you want to have a single P2P >>>> Device interface shared between multiple devices, no? >>> >>> Correct, but it seems to make sense to accommodate for it instead of doing >>> another patch later on (although the patch went in for v3.9, right?). Just a >>> comment, I leave it to David what to do with it. >>> >> I don't think it required, since wpa_supplicant has only one P2P state >> machine, I don't see a reason to deal with more than one P2P device. > > I think the idea behind it was that you could have a 2.4/5 GHz device > and a 60 GHz device and have them both use the same P2P Device Address > so you can use them interchangeably. That is indeed how I understood the explanation from Jouni (see attached email). I think it is actually about the mac address that ends up in the action frame header. > However I'd actually argue that you should just create the second one > with the same address as the first (and even creating a second one in > cases like that will need a lot of work) so that you can get the device > address assigned by the kernel? OTOH, if the kernel has no address, > we'll probably have to allow changing it? This may need some work. > > johannes > Regards, Arend
> >>>> That's not really necessary, unless you want to have a single P2P > >>>> Device interface shared between multiple devices, no? > >>> > >>> Correct, but it seems to make sense to accommodate for it instead of > >>> doing another patch later on (although the patch went in for v3.9, > >>> right?). Just a comment, I leave it to David what to do with it. > >>> > >> I don't think it required, since wpa_supplicant has only one P2P > >> state machine, I don't see a reason to deal with more than one P2P > device. > > > > I think the idea behind it was that you could have a 2.4/5 GHz device > > and a 60 GHz device and have them both use the same P2P Device Address > > so you can use them interchangeably. > > That is indeed how I understood the explanation from Jouni (see attached > email). I think it is actually about the mac address that ends up in the action > frame header. > Actually it's not going to work now, since the p2p state machine will always call the callbacks on the same interface. And there is a problem when you try to do something on one interface and it's running on other one - the events are raising on a wrong interface and I don't know how it should work. I think it's better to prevent enabling p2p on more than one interface (set p2p_disabled on by default and enable it explicitly if needed in the config file)? David --------------------------------------------------------------------- A member of the Intel Corporation group of companies This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On 02/17/2013 07:42 AM, Spinadel, David wrote: >>>>>> That's not really necessary, unless you want to have a single P2P >>>>>> Device interface shared between multiple devices, no? >>>>> >>>>> Correct, but it seems to make sense to accommodate for it instead of >>>>> doing another patch later on (although the patch went in for v3.9, >>>>> right?). Just a comment, I leave it to David what to do with it. >>>>> >>>> I don't think it required, since wpa_supplicant has only one P2P >>>> state machine, I don't see a reason to deal with more than one P2P >> device. >>> >>> I think the idea behind it was that you could have a 2.4/5 GHz device >>> and a 60 GHz device and have them both use the same P2P Device Address >>> so you can use them interchangeably. >> >> That is indeed how I understood the explanation from Jouni (see attached >> email). I think it is actually about the mac address that ends up in the action >> frame header. >> > Actually it's not going to work now, since the p2p state machine will always call the callbacks on the same interface. And there is a problem when you try to do something on one interface and it's running on other one - the events are raising on a wrong interface and I don't know how it should work. > I think it's better to prevent enabling p2p on more than one interface (set p2p_disabled on by default and enable it explicitly if needed in the config file)? I see. As the use-case seems valid the p2p state machine will need quite some rework or a more abstract interface that takes care of selecting the appropriate interface. I agree it is better to stick to one interface for now and make that work (maybe it already does, but I still need to test your patches ;-) ). Regards, Arend
> -----Original Message----- > From: Arend van Spriel [mailto:arend@broadcom.com] > Sent: Sunday, February 17, 2013 09:47 > To: Spinadel, David > Cc: Johannes Berg; hostap@lists.shmoo.com; Jouni Malinen > Subject: Re: [RFC 06/10] nl80211_driver: add support for P2P device in adding > and removing interface > > On 02/17/2013 07:42 AM, Spinadel, David wrote: > >>>>>> That's not really necessary, unless you want to have a single P2P > >>>>>> Device interface shared between multiple devices, no? > >>>>> > >>>>> Correct, but it seems to make sense to accommodate for it instead > >>>>> of doing another patch later on (although the patch went in for > >>>>> v3.9, right?). Just a comment, I leave it to David what to do with it. > >>>>> > >>>> I don't think it required, since wpa_supplicant has only one P2P > >>>> state machine, I don't see a reason to deal with more than one P2P > >> device. > >>> > >>> I think the idea behind it was that you could have a 2.4/5 GHz > >>> device and a 60 GHz device and have them both use the same P2P > >>> Device Address so you can use them interchangeably. > >> > >> That is indeed how I understood the explanation from Jouni (see > >> attached email). I think it is actually about the mac address that > >> ends up in the action frame header. > >> > > Actually it's not going to work now, since the p2p state machine will always > call the callbacks on the same interface. And there is a problem when you try > to do something on one interface and it's running on other one - the events > are raising on a wrong interface and I don't know how it should work. > > I think it's better to prevent enabling p2p on more than one interface (set > p2p_disabled on by default and enable it explicitly if needed in the config > file)? > > I see. As the use-case seems valid the p2p state machine will need quite > some rework or a more abstract interface that takes care of selecting the > appropriate interface. I agree it is better to stick to one interface for now and > make that work (maybe it already does, but I still need to test your patches ;- > ) ). > Actually there some bugs, I'll resend it later. And there is another patch that disables p2p by default to be sure that we are not enabling more than one p2p interface concurrently. I guess you are not going to work on it today... David --------------------------------------------------------------------- A member of the Intel Corporation group of companies This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On 02/17/2013 08:52 AM, Spinadel, David wrote: >> -----Original Message----- >> From: Arend van Spriel [mailto:arend@broadcom.com] >> Sent: Sunday, February 17, 2013 09:47 >> To: Spinadel, David >> Cc: Johannes Berg; hostap@lists.shmoo.com; Jouni Malinen >> Subject: Re: [RFC 06/10] nl80211_driver: add support for P2P device in adding >> and removing interface >> >> On 02/17/2013 07:42 AM, Spinadel, David wrote: >>>>>>>> That's not really necessary, unless you want to have a single P2P >>>>>>>> Device interface shared between multiple devices, no? >>>>>>> >>>>>>> Correct, but it seems to make sense to accommodate for it instead >>>>>>> of doing another patch later on (although the patch went in for >>>>>>> v3.9, right?). Just a comment, I leave it to David what to do with it. >>>>>>> >>>>>> I don't think it required, since wpa_supplicant has only one P2P >>>>>> state machine, I don't see a reason to deal with more than one P2P >>>> device. >>>>> >>>>> I think the idea behind it was that you could have a 2.4/5 GHz >>>>> device and a 60 GHz device and have them both use the same P2P >>>>> Device Address so you can use them interchangeably. >>>> >>>> That is indeed how I understood the explanation from Jouni (see >>>> attached email). I think it is actually about the mac address that >>>> ends up in the action frame header. >>>> >>> Actually it's not going to work now, since the p2p state machine will always >> call the callbacks on the same interface. And there is a problem when you try >> to do something on one interface and it's running on other one - the events >> are raising on a wrong interface and I don't know how it should work. >>> I think it's better to prevent enabling p2p on more than one interface (set >> p2p_disabled on by default and enable it explicitly if needed in the config >> file)? >> >> I see. As the use-case seems valid the p2p state machine will need quite >> some rework or a more abstract interface that takes care of selecting the >> appropriate interface. I agree it is better to stick to one interface for now and >> make that work (maybe it already does, but I still need to test your patches ;- >> ) ). >> > Actually there some bugs, I'll resend it later. And there is another patch that disables p2p by default to be sure that we are not enabling more than one p2p interface concurrently. > I guess you are not going to work on it today... I am if my 4 year old does not interrupt me :-p No rush though as I have no equipment at home to test with. Thanks, Arend
diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c index f0306f1..183761a 100644 --- a/src/drivers/driver_nl80211.c +++ b/src/drivers/driver_nl80211.c @@ -6051,6 +6051,13 @@ static int nl80211_create_iface_once(struct wpa_driver_nl80211_data *drv, return ret; } + if (iftype == NL80211_IFTYPE_P2P_DEVICE) { + wpa_printf(MSG_DEBUG, + "nl80211 New P2P device interface %s created", + ifname); + return 0; + } + ifidx = if_nametoindex(ifname); wpa_printf(MSG_DEBUG, "nl80211: New interface %s created: ifindex=%d", ifname, ifidx); @@ -6094,7 +6101,8 @@ static int nl80211_create_iface(struct wpa_driver_nl80211_data *drv, wds, handler, arg); } - if (ret >= 0 && is_p2p_interface(iftype)) + if (ret >= 0 && is_p2p_interface(iftype) && + iftype != NL80211_IFTYPE_P2P_DEVICE) nl80211_disable_11b_rates(drv, ret, 1); return ret; @@ -7345,7 +7353,7 @@ done: return ret; } - if (is_p2p_interface(nlmode)) + if (is_p2p_interface(nlmode) && nlmode != NL80211_IFTYPE_P2P_DEVICE) nl80211_disable_11b_rates(drv, drv->ifindex, 1); else if (drv->disabled_11b_rates) nl80211_disable_11b_rates(drv, drv->ifindex, 0);
From: David Spinadel <david.spinadel@intel.com> Don't try to assign ifindex for P2P device interface and check that the ifindex is valid before trying to remove it. Don't try to set TX rates for P2P device interface since it's not a network device. Signed-hostap: David Spinadel <david.spinadel@intel.com> --- src/drivers/driver_nl80211.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)