Patchwork [1/2] Fix radio remove works

login
register
mail settings
Submitter Eduardo Abinader
Date March 18, 2014, 1:50 p.m.
Message ID <1395150654-28969-2-git-send-email-eduardo.abinader@openbossa.org>
Download mbox | patch
Permalink /patch/331430/
State Superseded
Headers show

Comments

Eduardo Abinader - March 18, 2014, 1:50 p.m.
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(-)
Otcheretianski, Andrei - March 19, 2014, 8:21 a.m.
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.
Eduardo Abinader - March 19, 2014, 11:25 a.m.
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.
>
>
Otcheretianski, Andrei - March 19, 2014, 12:38 p.m.
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.
Eduardo Abinader - March 19, 2014, 6:30 p.m.
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.
>
>

Patch

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 */