| Message ID | 2C2F1EBA8050E74EA81502D5740B4BD6BBA9D30FC7@SJEXCHCCR02.corp.ad.broadcom.com |
|---|---|
| State | Rejected, archived |
| Headers | show |
On Tue, 2011-11-08 at 03:06 -0800, Neeraj Kumar Garg wrote: > +++ b/src/p2p/p2p_group.c > @@ -658,7 +658,7 @@ u8 p2p_group_presence_req(struct p2p_group *group, > curr_noa_len); > > /* TODO: properly process request and store copy */ > - if (curr_noa_len > 0) > + if (curr_noa_len < 0) > return P2P_SC_FAIL_UNABLE_TO_ACCOMMODATE; > > return P2P_SC_SUCCESS; I thought about this some more and I think neither behaviour is actually correct with nl80211 drivers since they don't pass NoA data up. Unless, of course, powersaving is disabled, the device doesn't support multi-channel, etc. I think this area generally needs a lot of work and this patch/hack isn't really suitable for anything. johannes
Hello Johannes, Thanks for your comments. I agree that this area needs a lot of work but I think it is safe to return P2P_SC_SUCCESS if curr_noa_len > 0. Once nl gives support to these cmds like (get_noa/set_noa), we can always revisit the code once it is properly implemented. May I ask what is the purpose of returning P2P_SC_FAIL_UNABLE_TO_ACCOMMODATE if driver returned noa_len > 0? Regards, -Neeraj -----Original Message----- From: Johannes Berg [mailto:johannes@sipsolutions.net] Sent: Saturday, November 19, 2011 4:41 PM To: Neeraj Kumar Garg Cc: hostap@lists.shmoo.com Subject: Re: [PATCH] P2P return a successful response for p2p presence request if driver has return noa_len greater than 0 On Tue, 2011-11-08 at 03:06 -0800, Neeraj Kumar Garg wrote: > +++ b/src/p2p/p2p_group.c > @@ -658,7 +658,7 @@ u8 p2p_group_presence_req(struct p2p_group *group, > curr_noa_len); > > /* TODO: properly process request and store copy */ > - if (curr_noa_len > 0) > + if (curr_noa_len < 0) > return P2P_SC_FAIL_UNABLE_TO_ACCOMMODATE; > > return P2P_SC_SUCCESS; I thought about this some more and I think neither behaviour is actually correct with nl80211 drivers since they don't pass NoA data up. Unless, of course, powersaving is disabled, the device doesn't support multi-channel, etc. I think this area generally needs a lot of work and this patch/hack isn't really suitable for anything. johannes
Hi Neeraj, > Thanks for your comments. I agree that this area needs a lot of work > but I think it is safe to return P2P_SC_SUCCESS if curr_noa_len > 0. > Once nl gives support to these cmds like (get_noa/set_noa), we can > always revisit the code once it is properly implemented. I disagree, see below. > May I ask what is the purpose of returning > P2P_SC_FAIL_UNABLE_TO_ACCOMMODATE if driver returned noa_len > 0? If the driver returned noa_len > 0, that means that a NoA schedule is in effect. That means that we don't know, without parsing the NoA schedule, whether or not the presence request can be fulfilled. Therefore, we must return UNABLE_TO_ACCOMODATE. With nl80211, unfortunately this logic is broken because the driver doesn't return a NoA schedule, so noa_len is always == 0 (I think?) and we shouldn't be returning SUCCESS even in that case because the driver or device might internally have a NoA schedule in effect. I'm not convinced that get_noa/set_noa is the right answer to this. get_noa is virtually impossible to implement in a race-free manner at least as far as presence requests are confirmed, and set_noa is really only a testing hook. I think the right answer may be to ask the driver for each presence request and return UNABLE_TO_ACCOMODATE if the driver doesn't implement the function. johannes
On Mon, 2011-11-21 at 10:10 +0100, Johannes Berg wrote: > If the driver returned noa_len > 0, that means that a NoA schedule is in > effect. That means that we don't know, without parsing the NoA schedule, > whether or not the presence request can be fulfilled. Therefore, we must > return UNABLE_TO_ACCOMODATE. > > With nl80211, unfortunately this logic is broken because the driver > doesn't return a NoA schedule, so noa_len is always == 0 (I think?) and > we shouldn't be returning SUCCESS even in that case because the driver > or device might internally have a NoA schedule in effect. > > I'm not convinced that get_noa/set_noa is the right answer to this. > get_noa is virtually impossible to implement in a race-free manner at > least as far as presence requests are confirmed, and set_noa is really > only a testing hook. > > I think the right answer may be to ask the driver for each presence > request and return UNABLE_TO_ACCOMODATE if the driver doesn't implement > the function. I also just noticed that the P2P core code (src/p2p) appears to attempt to distinguish between drivers having get_noa support and those that don't by checking p2p->cfg->get_noa, but obvious that is always set since it's the p2p_supplicant's function, not the driver's. So all this seems really really buggy right now. :-( johannes
2011/11/21 Johannes Berg <johannes@sipsolutions.net>: > On Mon, 2011-11-21 at 10:10 +0100, Johannes Berg wrote: > >> If the driver returned noa_len > 0, that means that a NoA schedule is in >> effect. That means that we don't know, without parsing the NoA schedule, >> whether or not the presence request can be fulfilled. Therefore, we must >> return UNABLE_TO_ACCOMODATE. >> >> With nl80211, unfortunately this logic is broken because the driver >> doesn't return a NoA schedule, so noa_len is always == 0 (I think?) and >> we shouldn't be returning SUCCESS even in that case because the driver >> or device might internally have a NoA schedule in effect. >> >> I'm not convinced that get_noa/set_noa is the right answer to this. >> get_noa is virtually impossible to implement in a race-free manner at >> least as far as presence requests are confirmed, and set_noa is really >> only a testing hook. >> >> I think the right answer may be to ask the driver for each presence >> request and return UNABLE_TO_ACCOMODATE if the driver doesn't implement >> the function. > > I also just noticed that the P2P core code (src/p2p) appears to attempt > to distinguish between drivers having get_noa support and those that > don't by checking p2p->cfg->get_noa, but obvious that is always set > since it's the p2p_supplicant's function, not the driver's. > > So all this seems really really buggy right now. :-( > > johannes > Hello Johannes, I think someone should propose architecture for NOA feature. Currently I am not sure what part should be done in the driver and what in supplicant. I am not sure get noa is required/usefull at all. Some my ideas: P2P GO NOA: - Seems we could have set_noa() callback (for certification tests purpose) and possible presence_request handling in the future - maybe we can remove get_noa() callback and instead of that send NOA notification from the driver after driver will set, change NOA params (specially index + start time). We can secure here correct StartTime (add some time) during NOA change to be sure we will update beacon + probe_response correcly - and to be sure all stations will get correct NOA settings. Current scenario we could have: 1) supplicant set_noa() or driver set defaults 2) driver secure correct startTime to be sure we will update beacons/probe_res and all STAs will sync 3) driver send notification to upper layer with NOA values 4) supplicant get this notification and update beacon template + probe_resp 5) supplicant set new beacon. x) we should remember startTime is 4 bytes TSF and change NOA every 70minutes Or driver/firmware should update beacons, check all Probe_Resp frames and will add correct NOA attribute? How this should work in the future? BR Janusz
On Mon, 2011-11-21 at 11:53 +0100, Janusz Dziedzic wrote: > I think someone should propose architecture for NOA feature. Agree, are you volunteering? :-) > Currently I am not sure what part should be done in the driver and > what in supplicant. Me neither. > I am not sure get noa is required/usefull at all. Agree here too. Our device will add NoA in beacons by itself, and we thus add it to probe responses in the driver. > Some my ideas: > P2P GO NOA: > - Seems we could have set_noa() callback (for certification tests > purpose) Agree with this. > and possible presence_request handling in the future Not sure I agree here -- it seems it might be easier to let the driver/device handle figuring out whether or not a given request can be handled and how it should be handled. > - maybe we can remove get_noa() callback and instead of that send NOA > notification from the driver after driver will set, change NOA params > (specially index + start time). We could do that, but I'm not sure it's really the best way to handle it. If there's a presence request, wpa_supplicant should obviously filter it according to its policies (if any), but I'm not convinced the final determination & actually putting it into effect shouldn't be done by the driver. Wouldn't the device also have to be aware of it for OppPS? > Current scenario we could have: > 1) supplicant set_noa() or driver set defaults > 2) driver secure correct startTime to be sure we will update > beacons/probe_res and all STAs will sync > 3) driver send notification to upper layer with NOA values > 4) supplicant get this notification and update beacon template + probe_resp > 5) supplicant set new beacon. > x) we should remember startTime is 4 bytes TSF and change NOA every 70minutes > > Or driver/firmware should update beacons, check all Probe_Resp frames > and will add correct NOA attribute? > How this should work in the future? Right now our firmware will update beacons, and our driver will append NoA to all probe responses. Since we can't turn off the beacon part, we'll definitely need to handle this. I suspect right now our firmware can't actually deal with presence requests properly at all, I believe it'll start a NoA schedule for example when you ask it to scan. So for this case it would be better to reject any presence request. I'm not quite sure what we gain from having the supplicant handle all this. Is this a lot of code if we required drivers handle it? johannes
On Sun, Nov 20, 2011 at 10:41:45PM -0800, Neeraj Kumar Garg wrote: > Thanks for your comments. I agree that this area needs a lot of work but I think it is safe to return P2P_SC_SUCCESS if curr_noa_len > 0. Not really. > May I ask what is the purpose of returning P2P_SC_FAIL_UNABLE_TO_ACCOMMODATE if driver returned noa_len > 0? The goal of the current implementation was to get minimal implementation to pass mandatory certification requirements with a driver (not nl80211-based). In other words, wpa_supplicant was accepting any request if NoA was not used at the GO and rejecting any request if NoA was used. As pointed out in this thread, more complete support needs quite a bit more work and needs to include with architecture design for NoA management in general. In other words, I cannot apply this patch, but I would be fine with a small patch to make this minimal design "work" with nl80211, i.e., make sure we do not accept any request incorrectly if the device happens to be using NoA internally without letting mac80211/wpa_supplicant know.
Hello Jouni, Thanks for the explanation. I see your point now. So below would be my 2 proposals: Proposal A): 1) Upon getting presence request, supplicant will simply call set_noa() with received presence request NOA attribute. Let the driver take care of modifying the NOA attribute in NOA frames depending upon the received presence request. Driver will also disable oppps upon this set_noa call. 2) We need to modify current set_noa function prototype to tell the driver somehow that the attached NOA attributes are from the presence request. Otherwise driver will not have anyway to find out if he has to do the modify NOA schedule based on presence request OR they are new NOA values from the application itself. 3) The disadvantage with this approach is that it doesn't look clean. Or we need to comeup with a new driver function (*handle_presence) or something like that. Proposal B): 1) Upon getting presence request, supplicant will call get_noa(). Supplicant will disable the current NOA schedule completely and will also disable oppps by simply calling set_noa().Also multiple presence requests from different clients will do the same. 2) Once all clients sends a presence request with 0 values (whoevers sent a presence request earlier), we will again enable the previous NOA schedule. 3) Advantage here is it is clean approach and doesn't increase the supplicant complexity to calculate revised NOA schedule. If you want to redesign the power-mgmt stuff later, I think Proposal B) would be a good alternative for near future, rather than not supporting the presence request completely if driver has an NOA schedule going on. Let me know your thoughts on this. I will accordingly send a patch. Regards, -Neeraj -----Original Message----- From: hostap-bounces@lists.shmoo.com [mailto:hostap-bounces@lists.shmoo.com] On Behalf Of Jouni Malinen Sent: Monday, November 21, 2011 7:37 PM To: hostap@lists.shmoo.com Subject: Re: [PATCH] P2P return a successful response for p2p presence request if driver has return noa_len greater than 0 On Sun, Nov 20, 2011 at 10:41:45PM -0800, Neeraj Kumar Garg wrote: > Thanks for your comments. I agree that this area needs a lot of work but I think it is safe to return P2P_SC_SUCCESS if curr_noa_len > 0. Not really. > May I ask what is the purpose of returning P2P_SC_FAIL_UNABLE_TO_ACCOMMODATE if driver returned noa_len > 0? The goal of the current implementation was to get minimal implementation to pass mandatory certification requirements with a driver (not nl80211-based). In other words, wpa_supplicant was accepting any request if NoA was not used at the GO and rejecting any request if NoA was used. As pointed out in this thread, more complete support needs quite a bit more work and needs to include with architecture design for NoA management in general. In other words, I cannot apply this patch, but I would be fine with a small patch to make this minimal design "work" with nl80211, i.e., make sure we do not accept any request incorrectly if the device happens to be using NoA internally without letting mac80211/wpa_supplicant know.
On Mon, Nov 21, 2011 at 12:01:05PM +0100, Johannes Berg wrote: > On Mon, 2011-11-21 at 11:53 +0100, Janusz Dziedzic wrote: > > and possible presence_request handling in the future > > Not sure I agree here -- it seems it might be easier to let the > driver/device handle figuring out whether or not a given request can be > handled and how it should be handled. I would be fine handling the message exchange through wpa_supplicant, but it does need to be the driver/device figuring out whether to agree to change schedule and if so, how. Getting the correct NoA information into Presence Response can be a bit difficult in some cases, but this gets updated with the next Beacon frame anyway, so I'm not too concerned about that. > > 4) supplicant get this notification and update beacon template + probe_resp > > 5) supplicant set new beacon. This sounds unnecessary.. The driver can do this anyway (just add another P2P IE with NoA attribute to the end of the frame). > Right now our firmware will update beacons, and our driver will append > NoA to all probe responses. Since we can't turn off the beacon part, > we'll definitely need to handle this. This seems to be what most vendors have ended up doing (or well firmware/driver, but anyway, somewhere much lower in the stack than wpa_supplicant). > I suspect right now our firmware can't actually deal with presence > requests properly at all, I believe it'll start a NoA schedule for > example when you ask it to scan. So for this case it would be better to > reject any presence request. > > I'm not quite sure what we gain from having the supplicant handle all > this. Is this a lot of code if we required drivers handle it? Agreed. I did a small change based on the thread: reject Presence Request if current NoA cannot be fetched from the driver. That sounds like a safer option and if someone really wants to support Presence Request properly, that can be implemented in the driver/firmware.
On Sun, 2011-12-11 at 17:14 +0200, Jouni Malinen wrote: > > > 4) supplicant get this notification and update beacon template + probe_resp > > > 5) supplicant set new beacon. > > This sounds unnecessary.. The driver can do this anyway (just add > another P2P IE with NoA attribute to the end of the frame). > > > Right now our firmware will update beacons, and our driver will append > > NoA to all probe responses. Since we can't turn off the beacon part, > > we'll definitely need to handle this. > > This seems to be what most vendors have ended up doing (or well > firmware/driver, but anyway, somewhere much lower in the stack than > wpa_supplicant). Right. And given that it's just appending an IE, it's really not a big deal. > > I suspect right now our firmware can't actually deal with presence > > requests properly at all, I believe it'll start a NoA schedule for > > example when you ask it to scan. So for this case it would be better to > > reject any presence request. > > > > I'm not quite sure what we gain from having the supplicant handle all > > this. Is this a lot of code if we required drivers handle it? > > Agreed. I did a small change based on the thread: reject Presence > Request if current NoA cannot be fetched from the driver. That sounds > like a safer option and if someone really wants to support Presence > Request properly, that can be implemented in the driver/firmware. Cool, thanks. I think this change makes sense. The question will be how to implement presence requests, I think it would also be a bit silly to have the driver implement all of it, but I don't think I need a solution at all right now. If somebody wants to do it, it will probably require asking the driver about each request, and if the driver accepts it it would return an updated NoA schedule & keep track of the request to be able to actually honour it, e.g. by scheduling scans around it. johannes
Hello Jouni, Johannes I think my 2 below proposals somehow got un-notices. I think handling presence request should still be done in supplicant. Otherwise driver has to parse all action frames and then if received presence request, need to take an action. I came up with below 2 proposals (A and B): Proposal A): 1) Upon getting presence request, supplicant can simply call set_noa() with received presence request NOA attribute. Let the driver take care of modifying the NOA attribute in NOA frames depending upon the received presence request. Driver will also disable oppps upon this set_noa call. 2) We need to modify current set_noa function prototype to tell the driver somehow that the attached NOA attributes are from the presence request. Otherwise driver will not have anyway to find out if he has to do the modify NOA schedule based on presence request OR they are new NOA values from the application itself. 3) The disadvantage with this approach is that it doesn't look clean. Or we need to comeup with a new driver function (*handle_presence) or something like that. Proposal B): 1) Upon getting presence request, supplicant will call get_noa(). Supplicant will disable the current NOA schedule completely and will also disable oppps by simply calling set_noa(). Also multiple presence requests from different clients will do the same. 2) Once all clients sends a presence request with 0 values (whoevers sent a presence request earlier), we will again enable the previous NOA schedule. 3) Advantage here is it is clean approach and doesn't increase the supplicant complexity to calculate revised NOA schedule. If you want to redesign the power-mgmt stuff later, I think Proposal B) would be a good alternative for near future, rather than not supporting the presence request completely if driver has an NOA schedule going on. Let me know your thoughts on this. I can accordingly send a patch. Regards, -Neeraj -----Original Message----- From: hostap-bounces@lists.shmoo.com [mailto:hostap-bounces@lists.shmoo.com] On Behalf Of Johannes Berg Sent: Sunday, December 11, 2011 8:54 PM To: Jouni Malinen Cc: hostap@lists.shmoo.com Subject: Re: [PATCH] P2P return a successful response for p2p presence request if driver has return noa_len greater than 0 On Sun, 2011-12-11 at 17:14 +0200, Jouni Malinen wrote: > > > 4) supplicant get this notification and update beacon template + probe_resp > > > 5) supplicant set new beacon. > > This sounds unnecessary.. The driver can do this anyway (just add > another P2P IE with NoA attribute to the end of the frame). > > > Right now our firmware will update beacons, and our driver will append > > NoA to all probe responses. Since we can't turn off the beacon part, > > we'll definitely need to handle this. > > This seems to be what most vendors have ended up doing (or well > firmware/driver, but anyway, somewhere much lower in the stack than > wpa_supplicant). Right. And given that it's just appending an IE, it's really not a big deal. > > I suspect right now our firmware can't actually deal with presence > > requests properly at all, I believe it'll start a NoA schedule for > > example when you ask it to scan. So for this case it would be better to > > reject any presence request. > > > > I'm not quite sure what we gain from having the supplicant handle all > > this. Is this a lot of code if we required drivers handle it? > > Agreed. I did a small change based on the thread: reject Presence > Request if current NoA cannot be fetched from the driver. That sounds > like a safer option and if someone really wants to support Presence > Request properly, that can be implemented in the driver/firmware. Cool, thanks. I think this change makes sense. The question will be how to implement presence requests, I think it would also be a bit silly to have the driver implement all of it, but I don't think I need a solution at all right now. If somebody wants to do it, it will probably require asking the driver about each request, and if the driver accepts it it would return an updated NoA schedule & keep track of the request to be able to actually honour it, e.g. by scheduling scans around it. johannes
Hi Neeraj, Maybe instead of proposals, you should describe what you want first. It seems that you want a basic form of presence request handling? > Proposal A): > 1) Upon getting presence request, supplicant can simply call set_noa() > with received presence request NOA attribute. Let the driver take care > of modifying the NOA attribute in NOA frames depending upon the > received presence request. Driver will also disable oppps upon this > set_noa call. > 2) We need to modify current set_noa function prototype to tell the > driver somehow that the attached NOA attributes are from the presence > request. Otherwise driver will not have anyway to find out if he has > to do the modify NOA schedule based on presence request OR they are > new NOA values from the application itself. > 3) The disadvantage with this approach is that it doesn't look clean. > Or we need to comeup with a new driver function (*handle_presence) or > something like that. Using set_noa() is definitely not a good plan. Adding handle_presence(), which can return an error as well, is a better plan. HOWEVER -- for symmetry you will have to also give the driver which station this was so it can *remove* the presence request again when the station requests a new one/none any more, and the supplicant should also tell the driver to remove it before it removes that station from the driver. So I think there are a bunch of corner cases to get right wrt. updating the presence request and removing it. > Proposal B): > 1) Upon getting presence request, supplicant will call get_noa(). > Supplicant will disable the current NOA schedule completely and will > also disable oppps by simply calling set_noa(). Also multiple presence > requests from different clients will do the same. > 2) Once all clients sends a presence request with 0 values (whoevers > sent a presence request earlier), we will again enable the previous > NOA schedule. > 3) Advantage here is it is clean approach and doesn't increase the > supplicant complexity to calculate revised NOA schedule. This doesn't seem right either -- forget set_noa() completely, it's for testing. Anything should, in my opinion, be done through specific APIs. johannes
Hello Johannes, Yes, we need basic form of presence request handling and in my opinion this should be done in supplicant as supplicant already takes care of rest of the action frames.. Now there are some challenges in implementing it in supplicant, and to start with, we can simply call handle_presence function from supplicant. And let the driver handle it. I am fine to implement another function called handle_presence(). Upon handle_presence(with client addr). Then driver can take a call what to do for revised NOA schedule. A simple driver may decide to disable the NOA schedule completely until all the connected clients send a presence request with 0 values. Supplicant should here also make sure that it calls handle_presence function only when it knows that presence request has come from a connected client. Driver need to maintain a list of clients who has sent the presence request. But this will also involve adding one notification from driver to supplicant which is to update the revised NOA schedule in the beacons and probe responses. Or we need not do this, driver can put the modified NOA schedule in the beacons and probe responses. More thoughts of yours? Regards, -Neeraj -----Original Message----- From: Johannes Berg [mailto:johannes@sipsolutions.net] Sent: Monday, December 12, 2011 2:29 PM To: Neeraj Kumar Garg Cc: Jouni Malinen; hostap@lists.shmoo.com Subject: RE: [PATCH] P2P return a successful response for p2p presence request if driver has return noa_len greater than 0 Hi Neeraj, Maybe instead of proposals, you should describe what you want first. It seems that you want a basic form of presence request handling? > Proposal A): > 1) Upon getting presence request, supplicant can simply call set_noa() > with received presence request NOA attribute. Let the driver take care > of modifying the NOA attribute in NOA frames depending upon the > received presence request. Driver will also disable oppps upon this > set_noa call. > 2) We need to modify current set_noa function prototype to tell the > driver somehow that the attached NOA attributes are from the presence > request. Otherwise driver will not have anyway to find out if he has > to do the modify NOA schedule based on presence request OR they are > new NOA values from the application itself. > 3) The disadvantage with this approach is that it doesn't look clean. > Or we need to comeup with a new driver function (*handle_presence) or > something like that. Using set_noa() is definitely not a good plan. Adding handle_presence(), which can return an error as well, is a better plan. HOWEVER -- for symmetry you will have to also give the driver which station this was so it can *remove* the presence request again when the station requests a new one/none any more, and the supplicant should also tell the driver to remove it before it removes that station from the driver. So I think there are a bunch of corner cases to get right wrt. updating the presence request and removing it. > Proposal B): > 1) Upon getting presence request, supplicant will call get_noa(). > Supplicant will disable the current NOA schedule completely and will > also disable oppps by simply calling set_noa(). Also multiple presence > requests from different clients will do the same. > 2) Once all clients sends a presence request with 0 values (whoevers > sent a presence request earlier), we will again enable the previous > NOA schedule. > 3) Advantage here is it is clean approach and doesn't increase the > supplicant complexity to calculate revised NOA schedule. This doesn't seem right either -- forget set_noa() completely, it's for testing. Anything should, in my opinion, be done through specific APIs. johannes
Hi Neeraj, > Yes, we need basic form of presence request handling and in my opinion > this should be done in supplicant as supplicant already takes care of > rest of the action frames.. > Now there are some challenges in implementing it in supplicant, and to > start with, we can simply call handle_presence function from > supplicant. And let the driver handle it. > > I am fine to implement another function called handle_presence(). Upon > handle_presence(with client addr). Then driver can take a call what to > do for revised NOA schedule. A simple driver may decide to disable the > NOA schedule completely until all the connected clients send a > presence request with 0 values. Supplicant should here also make sure > that it calls handle_presence function only when it knows that > presence request has come from a connected client. Driver need to > maintain a list of clients who has sent the presence request. Yes, this sounds reasonable. Not sure how the client disconnect etc. should be handled though (and I'm not sure if a client can "give up" a presence request, maybe by sending a new one?) > But this will also involve adding one notification from driver to > supplicant which is to update the revised NOA schedule in the beacons > and probe responses. Or we need not do this, driver can put the > modified NOA schedule in the beacons and probe responses. I don't think that's necessary -- we've kept this in the driver (device in some cases), and it's trivial to do it there, so I see no reason to let it go through wpa_supplicant. johannes
Johannes, Good point of disconnection. I think we need to call handle_presence from supplicant if a presence request was earlier received from the client and client is disconnecting without clearing the presence request. As per the spec, a client can give up presence request (or clear its presence request) by sending another presence request with 0 NOA attributes. I also feel that adding NOA attribute in the driver/firmware automatically may be a good idea at the moment. Later we can think of notifying supplicant for changes in IEs in beacons and probe-responses. Regards, -Neeraj -----Original Message----- From: Johannes Berg [mailto:johannes@sipsolutions.net] Sent: Tuesday, December 13, 2011 7:06 PM To: Neeraj Kumar Garg Cc: Jouni Malinen; hostap@lists.shmoo.com Subject: RE: [PATCH] P2P return a successful response for p2p presence request if driver has return noa_len greater than 0 Hi Neeraj, > Yes, we need basic form of presence request handling and in my opinion > this should be done in supplicant as supplicant already takes care of > rest of the action frames.. > Now there are some challenges in implementing it in supplicant, and to > start with, we can simply call handle_presence function from > supplicant. And let the driver handle it. > > I am fine to implement another function called handle_presence(). Upon > handle_presence(with client addr). Then driver can take a call what to > do for revised NOA schedule. A simple driver may decide to disable the > NOA schedule completely until all the connected clients send a > presence request with 0 values. Supplicant should here also make sure > that it calls handle_presence function only when it knows that > presence request has come from a connected client. Driver need to > maintain a list of clients who has sent the presence request. Yes, this sounds reasonable. Not sure how the client disconnect etc. should be handled though (and I'm not sure if a client can "give up" a presence request, maybe by sending a new one?) > But this will also involve adding one notification from driver to > supplicant which is to update the revised NOA schedule in the beacons > and probe responses. Or we need not do this, driver can put the > modified NOA schedule in the beacons and probe responses. I don't think that's necessary -- we've kept this in the driver (device in some cases), and it's trivial to do it there, so I see no reason to let it go through wpa_supplicant. johannes
On Sun, Dec 11, 2011 at 11:43:14PM -0800, Neeraj Kumar Garg wrote:
> I think my 2 below proposals somehow got un-notices. I think handling presence request should still be done in supplicant. Otherwise driver has to parse all action frames and then if received presence request, need to take an action.
While I'm not necessarily against actually processing the Action frame
itself in wpa_supplicant, please note that the Presence Request frame is
a P2P Action frame and not P2P Public Action frame. The P2P Action
frames are much more specific to a group operation and I don't see any
problems in drivers handling them since they are more specific to exact
timing and parameters of the group and require synchronization with
low-level operations that are way below wpa_supplicant in the stack.
On Tue, Dec 13, 2011 at 08:31:38PM -0800, Neeraj Kumar Garg wrote: > Good point of disconnection. I think we need to call handle_presence from supplicant if a presence request was earlier received from the client and client is disconnecting without clearing the presence request. I'm assuming that the driver/firmware has to maintain a STA entry anyway and one part of that entry should really be the latest presence request from the STA if the driver claims to supports presence requests in some meaningful way. As such, it should be trivial for the driver/firmware to take care of this clearing automatically without any new call from wpa_supplicant (i.e., just use the existing disconnection mechanism). > As per the spec, a client can give up presence request (or clear its presence request) by sending another presence request with 0 NOA attributes. > I also feel that adding NOA attribute in the driver/firmware automatically may be a good idea at the moment. Later we can think of notifying supplicant for changes in IEs in beacons and probe-responses. I do not see why wpa_supplicant would need to do anything with NoA updates for Beacon or Probe Response frames. If we handle the Presence Request/Response frames in wpa_supplicant, the new (modified or unmodified as it may be) NoA will need to be returned from handle_presence, but other than that, I would expect NoA information to live within driver/firmware.
diff --git a/src/p2p/p2p_group.c b/src/p2p/p2p_group.c old mode 100644 new mode 100755 index 58b24c5..d60f6c0 --- a/src/p2p/p2p_group.c +++ b/src/p2p/p2p_group.c @@ -658,7 +658,7 @@ u8 p2p_group_presence_req(struct p2p_group *group, curr_noa_len); /* TODO: properly process request and store copy */ - if (curr_noa_len > 0) + if (curr_noa_len < 0) return P2P_SC_FAIL_UNABLE_TO_ACCOMMODATE; return P2P_SC_SUCCESS;