| Message ID | 1395150654-28969-2-git-send-email-eduardo.abinader@openbossa.org |
|---|---|
| State | Superseded |
| Headers | show |
Hi,
Checking work->wpa_s doesn't make sense, since it is set in radio_add_work and never unset.
Anyway I think the fix should be more like this:
@@ -3259,6 +3259,7 @@ static void radio_remove_interface(struct wpa_supplicant *wpa_s)
wpa_s->ifname, radio->name);
dl_list_del(&wpa_s->radio_list);
if (!dl_list_empty(&radio->ifaces)) {
+ radio_remove_works(wpa_s, NULL, 0);
wpa_s->radio = NULL;
return; /* Interfaces remain for this radio */
}
Regards,
Andrei
> -----Original Message-----
> From: hostap-bounces@lists.shmoo.com [mailto:hostap-
> bounces@lists.shmoo.com] On Behalf Of Eduardo Abinader
> Sent: Tuesday, March 18, 2014 15:51
> To: hostap@lists.shmoo.com
> Subject: [PATCH 1/2] Fix radio remove works
>
> When radio has been previously removed and pending radio works related
> to an excluded interface remains, new works are not capable of being
> executed. That occurs when a potential P2P client fails to negotiate group
> formation. For some reason, the P2P device is no more capable of issuing
> radio works. Those checks prevent this situation, by allowing removal of
> previous radio works.
>
> Signed-off-by: Eduardo Abinader <eduardo.abinader@openbossa.org>
> ---
> wpa_supplicant/wpa_supplicant.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/wpa_supplicant/wpa_supplicant.c
> b/wpa_supplicant/wpa_supplicant.c index 499dcb3..c74ac20 100644
> --- a/wpa_supplicant/wpa_supplicant.c
> +++ b/wpa_supplicant/wpa_supplicant.c
> @@ -3176,7 +3176,8 @@ void radio_remove_works(struct wpa_supplicant
> *wpa_s,
>
> dl_list_for_each_safe(work, tmp, &radio->work, struct
> wpa_radio_work,
> list) {
> - if (type && os_strcmp(type, work->type) != 0)
> + if (work->wpa_s && work->wpa_s->radio && type &&
> + os_strcmp(type, work->type) != 0)
> continue;
>
> /* skip other ifaces' works */
> --
> 1.8.3.2
>
> _______________________________________________
> HostAP mailing list
> HostAP@lists.shmoo.com
> http://lists.shmoo.com/mailman/listinfo/hostap
---------------------------------------------------------------------
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.
Actually, I did cogitate a very similar solution, but I was wondering if there was some deferred radio work still being removed or cleaned, so that's why I decided to implement in radio_remove_works. The work->wpa_s is just a sanity check for work->wpa_s->radio, actually. Just to be safer. Regards. On Wed, Mar 19, 2014 at 4:21 AM, Otcheretianski, Andrei < andrei.otcheretianski@intel.com> wrote: > Hi, > Checking work->wpa_s doesn't make sense, since it is set in radio_add_work > and never unset. > Anyway I think the fix should be more like this: > > @@ -3259,6 +3259,7 @@ static void radio_remove_interface(struct > wpa_supplicant *wpa_s) > wpa_s->ifname, radio->name); > dl_list_del(&wpa_s->radio_list); > if (!dl_list_empty(&radio->ifaces)) { > + radio_remove_works(wpa_s, NULL, 0); > wpa_s->radio = NULL; > return; /* Interfaces remain for this radio */ > } > > Regards, > Andrei > > > > -----Original Message----- > > From: hostap-bounces@lists.shmoo.com [mailto:hostap- > > bounces@lists.shmoo.com] On Behalf Of Eduardo Abinader > > Sent: Tuesday, March 18, 2014 15:51 > > To: hostap@lists.shmoo.com > > Subject: [PATCH 1/2] Fix radio remove works > > > > When radio has been previously removed and pending radio works related > > to an excluded interface remains, new works are not capable of being > > executed. That occurs when a potential P2P client fails to negotiate > group > > formation. For some reason, the P2P device is no more capable of issuing > > radio works. Those checks prevent this situation, by allowing removal of > > previous radio works. > > > > Signed-off-by: Eduardo Abinader <eduardo.abinader@openbossa.org> > > --- > > wpa_supplicant/wpa_supplicant.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/wpa_supplicant/wpa_supplicant.c > > b/wpa_supplicant/wpa_supplicant.c index 499dcb3..c74ac20 100644 > > --- a/wpa_supplicant/wpa_supplicant.c > > +++ b/wpa_supplicant/wpa_supplicant.c > > @@ -3176,7 +3176,8 @@ void radio_remove_works(struct wpa_supplicant > > *wpa_s, > > > > dl_list_for_each_safe(work, tmp, &radio->work, struct > > wpa_radio_work, > > list) { > > - if (type && os_strcmp(type, work->type) != 0) > > + if (work->wpa_s && work->wpa_s->radio && type && > > + os_strcmp(type, work->type) != 0) > > continue; > > > > /* skip other ifaces' works */ > > -- > > 1.8.3.2 > > > > _______________________________________________ > > HostAP mailing list > > HostAP@lists.shmoo.com > > http://lists.shmoo.com/mailman/listinfo/hostap > --------------------------------------------------------------------- > 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. > >
The only place where wpa_s->radio is set to NULL is in radio_remove_interface.. Since radio_remove_works should remove all the works (including started) and it is called right before setting wpa_s->radio = NULL, I can't see any scenario where work->wpa_s->radio == NULL > From: Eduardo Abinader [mailto:eduardo.abinader@openbossa.org] > Sent: Wednesday, March 19, 2014 13:25 > To: Otcheretianski, Andrei > Subject: Re: [PATCH 1/2] Fix radio remove works > > Actually, I did cogitate a very similar solution, but I was wondering if there was some deferred radio work still being removed or cleaned, so that's why I decided to implement in radio_remove_works. > > The work->wpa_s is just a sanity check for work->wpa_s->radio, actually. Just to be safer. > > > Regards. > > > > On Wed, Mar 19, 2014 at 4:21 AM, Otcheretianski, Andrei <andrei.otcheretianski@intel.com> wrote: > > > Hi, > Checking work->wpa_s doesn't make sense, since it is set in radio_add_work and never unset. > Anyway I think the fix should be more like this: > > @@ -3259,6 +3259,7 @@ static void radio_remove_interface(struct wpa_supplicant *wpa_s) > wpa_s->ifname, radio->name); > dl_list_del(&wpa_s->radio_list); > if (!dl_list_empty(&radio->ifaces)) { > + radio_remove_works(wpa_s, NULL, 0); > wpa_s->radio = NULL; > return; /* Interfaces remain for this radio */ > } > > Regards, > Andrei > > > > > -----Original Message----- > > From: hostap-bounces@lists.shmoo.com [mailto:hostap- > > bounces@lists.shmoo.com] On Behalf Of Eduardo Abinader > > Sent: Tuesday, March 18, 2014 15:51 > > To: hostap@lists.shmoo.com > > Subject: [PATCH 1/2] Fix radio remove works > > > > When radio has been previously removed and pending radio works related > > to an excluded interface remains, new works are not capable of being > > executed. That occurs when a potential P2P client fails to negotiate group > > formation. For some reason, the P2P device is no more capable of issuing > > radio works. Those checks prevent this situation, by allowing removal of > > previous radio works. > > > > Signed-off-by: Eduardo Abinader <eduardo.abinader@openbossa.org> > > --- > > wpa_supplicant/wpa_supplicant.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/wpa_supplicant/wpa_supplicant.c > > b/wpa_supplicant/wpa_supplicant.c index 499dcb3..c74ac20 100644 > > --- a/wpa_supplicant/wpa_supplicant.c > > +++ b/wpa_supplicant/wpa_supplicant.c > > @@ -3176,7 +3176,8 @@ void radio_remove_works(struct wpa_supplicant > > *wpa_s, > > > > dl_list_for_each_safe(work, tmp, &radio->work, struct > > wpa_radio_work, > > list) { > > - if (type && os_strcmp(type, work->type) != 0) > > + if (work->wpa_s && work->wpa_s->radio && type && > > + os_strcmp(type, work->type) != 0) > > continue; > > > > /* skip other ifaces' works */ > > -- > > 1.8.3.2 > > --------------------------------------------------------------------- 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.
All right. Gonna test your suggestion and check if regression is ok. Should I send this patch again or are you gonna do that? Regards. On Wed, Mar 19, 2014 at 8:38 AM, Otcheretianski, Andrei < andrei.otcheretianski@intel.com> wrote: > The only place where wpa_s->radio is set to NULL is in > radio_remove_interface.. Since radio_remove_works should remove all the > works (including started) and it is called right before setting > wpa_s->radio = NULL, I can't see any scenario where work->wpa_s->radio == > NULL > > > From: Eduardo Abinader [mailto:eduardo.abinader@openbossa.org] > > Sent: Wednesday, March 19, 2014 13:25 > > To: Otcheretianski, Andrei > > Subject: Re: [PATCH 1/2] Fix radio remove works > > > > Actually, I did cogitate a very similar solution, but I was wondering if > there was some deferred radio work still being removed or cleaned, so > that's why I decided to implement in radio_remove_works. > > > > The work->wpa_s is just a sanity check for work->wpa_s->radio, actually. > Just to be safer. > > > > > > Regards. > > > > > > > > On Wed, Mar 19, 2014 at 4:21 AM, Otcheretianski, Andrei < > andrei.otcheretianski@intel.com> wrote: > > > > > > Hi, > > Checking work->wpa_s doesn't make sense, since it is set in > radio_add_work and never unset. > > Anyway I think the fix should be more like this: > > > > @@ -3259,6 +3259,7 @@ static void radio_remove_interface(struct > wpa_supplicant *wpa_s) > > wpa_s->ifname, radio->name); > > dl_list_del(&wpa_s->radio_list); > > if (!dl_list_empty(&radio->ifaces)) { > > + radio_remove_works(wpa_s, NULL, 0); > > wpa_s->radio = NULL; > > return; /* Interfaces remain for this > radio */ > > } > > > > Regards, > > Andrei > > > > > > > > > -----Original Message----- > > > From: hostap-bounces@lists.shmoo.com [mailto:hostap- > > > bounces@lists.shmoo.com] On Behalf Of Eduardo Abinader > > > Sent: Tuesday, March 18, 2014 15:51 > > > To: hostap@lists.shmoo.com > > > Subject: [PATCH 1/2] Fix radio remove works > > > > > > When radio has been previously removed and pending radio works related > > > to an excluded interface remains, new works are not capable of being > > > executed. That occurs when a potential P2P client fails to negotiate > group > > > formation. For some reason, the P2P device is no more capable of > issuing > > > radio works. Those checks prevent this situation, by allowing removal > of > > > previous radio works. > > > > > > Signed-off-by: Eduardo Abinader <eduardo.abinader@openbossa.org> > > > --- > > > wpa_supplicant/wpa_supplicant.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/wpa_supplicant/wpa_supplicant.c > > > b/wpa_supplicant/wpa_supplicant.c index 499dcb3..c74ac20 100644 > > > --- a/wpa_supplicant/wpa_supplicant.c > > > +++ b/wpa_supplicant/wpa_supplicant.c > > > @@ -3176,7 +3176,8 @@ void radio_remove_works(struct wpa_supplicant > > > *wpa_s, > > > > > > dl_list_for_each_safe(work, tmp, &radio->work, struct > > > wpa_radio_work, > > > list) { > > > - if (type && os_strcmp(type, work->type) != 0) > > > + if (work->wpa_s && work->wpa_s->radio && type && > > > + os_strcmp(type, work->type) != 0) > > > continue; > > > > > > /* skip other ifaces' works */ > > > -- > > > 1.8.3.2 > > > > --------------------------------------------------------------------- > 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. > >
diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c index 499dcb3..c74ac20 100644 --- a/wpa_supplicant/wpa_supplicant.c +++ b/wpa_supplicant/wpa_supplicant.c @@ -3176,7 +3176,8 @@ void radio_remove_works(struct wpa_supplicant *wpa_s, dl_list_for_each_safe(work, tmp, &radio->work, struct wpa_radio_work, list) { - if (type && os_strcmp(type, work->type) != 0) + if (work->wpa_s && work->wpa_s->radio && type && + os_strcmp(type, work->type) != 0) continue; /* skip other ifaces' works */
When radio has been previously removed and pending radio works related to an excluded interface remains, new works are not capable of being executed. That occurs when a potential P2P client fails to negotiate group formation. For some reason, the P2P device is no more capable of issuing radio works. Those checks prevent this situation, by allowing removal of previous radio works. Signed-off-by: Eduardo Abinader <eduardo.abinader@openbossa.org> --- wpa_supplicant/wpa_supplicant.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)