Re: remove redundant flag from iwm(4)

[Mark Kettenis]

> Date: Tue, 20 Jun 2017 13:34:57 +0200
> From: Stefan Sperling 
> 
> This fixed diff matches what was intended, and it still works (tested
> switching between networks and suspend/resume).

ok kettenis@

> Index: sys/dev/pci/if_iwm.c
> ===
> RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
> retrieving revision 1.197
> diff -u -p -r1.197 if_iwm.c
> --- sys/dev/pci/if_iwm.c  16 Jun 2017 08:45:34 -  1.197
> +++ sys/dev/pci/if_iwm.c  16 Jun 2017 11:47:09 -
> @@ -6106,9 +6106,6 @@ iwm_init(struct ifnet *ifp)
>   struct ieee80211com *ic = >sc_ic;
>   int err, generation;
>  
> - if (sc->sc_flags & IWM_FLAG_HW_INITED) {
> - return 0;
> - }
>   sc->sc_generation++;
>  
>   err = iwm_init_hw(sc);
> @@ -6135,8 +6132,6 @@ iwm_init(struct ifnet *ifp)
>   return err;
>   } while (ic->ic_state != IEEE80211_S_SCAN);
>  
> - sc->sc_flags |= IWM_FLAG_HW_INITED;
> -
>   return 0;
>  }
>  
> @@ -6214,7 +6209,6 @@ iwm_stop(struct ifnet *ifp, int disable)
>   struct ieee80211com *ic = >sc_ic;
>   struct iwm_node *in = (void *)ic->ic_bss;
>  
> - sc->sc_flags &= ~IWM_FLAG_HW_INITED;
>   sc->sc_generation++;
>   ic->ic_scan_lock = IEEE80211_SCAN_UNLOCKED;
>   ifp->if_flags &= ~IFF_RUNNING;
> @@ -7450,7 +7444,7 @@ iwm_init_task(void *arg1)
>   }
>   s = splnet();
>  
> - if (sc->sc_flags & IWM_FLAG_HW_INITED)
> + if (ifp->if_flags & IFF_RUNNING)
>   iwm_stop(ifp, 0);
>   if ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) == IFF_UP)
>   iwm_init(ifp);
> Index: sys/dev/pci/if_iwmvar.h
> ===
> RCS file: /cvs/src/sys/dev/pci/if_iwmvar.h,v
> retrieving revision 1.28
> diff -u -p -r1.28 if_iwmvar.h
> --- sys/dev/pci/if_iwmvar.h   14 Jun 2017 16:56:04 -  1.28
> +++ sys/dev/pci/if_iwmvar.h   14 Jun 2017 19:17:23 -
> @@ -280,9 +280,8 @@ struct iwm_rx_ring {
>  };
>  
>  #define IWM_FLAG_USE_ICT 0x01
> -#define IWM_FLAG_HW_INITED   0x02
> -#define IWM_FLAG_RFKILL  0x04
> -#define IWM_FLAG_SCANNING0x08
> +#define IWM_FLAG_RFKILL  0x02
> +#define IWM_FLAG_SCANNING0x04
>  
>  struct iwm_ucode_status {
>   uint32_t uc_error_event_table;
> 
> 

Re: remove redundant flag from iwm(4)

[Stefan Sperling]

On Mon, Jun 19, 2017 at 02:10:50PM +0200, Mark Kettenis wrote:
> > Date: Mon, 19 Jun 2017 13:02:58 +0200
> > From: Stefan Sperling 
> > 
> > On Mon, Jun 19, 2017 at 11:57:36AM +0200, Mark Kettenis wrote:
> > > > @@ -7450,7 +7444,7 @@ iwm_init_task(void *arg1)
> > > > }
> > > > s = splnet();
> > > >  
> > > > -   if (sc->sc_flags & IWM_FLAG_HW_INITED)
> > > > +   if (sc->sc_flags & IFF_RUNNING)
> > > > iwm_stop(ifp, 0);
> > > 
> > > This looks wrong to me.
> > 
> > Why?
> 
> Because IFF_RUNNING is a flag for ifp->if_flags, not sc->sc_flags.

Indeed, thanks for catching this mistake!

This fixed diff matches what was intended, and it still works (tested
switching between networks and suspend/resume).

Index: sys/dev/pci/if_iwm.c
===
RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.197
diff -u -p -r1.197 if_iwm.c
--- sys/dev/pci/if_iwm.c16 Jun 2017 08:45:34 -  1.197
+++ sys/dev/pci/if_iwm.c16 Jun 2017 11:47:09 -
@@ -6106,9 +6106,6 @@ iwm_init(struct ifnet *ifp)
struct ieee80211com *ic = >sc_ic;
int err, generation;
 
-   if (sc->sc_flags & IWM_FLAG_HW_INITED) {
-   return 0;
-   }
sc->sc_generation++;
 
err = iwm_init_hw(sc);
@@ -6135,8 +6132,6 @@ iwm_init(struct ifnet *ifp)
return err;
} while (ic->ic_state != IEEE80211_S_SCAN);
 
-   sc->sc_flags |= IWM_FLAG_HW_INITED;
-
return 0;
 }
 
@@ -6214,7 +6209,6 @@ iwm_stop(struct ifnet *ifp, int disable)
struct ieee80211com *ic = >sc_ic;
struct iwm_node *in = (void *)ic->ic_bss;
 
-   sc->sc_flags &= ~IWM_FLAG_HW_INITED;
sc->sc_generation++;
ic->ic_scan_lock = IEEE80211_SCAN_UNLOCKED;
ifp->if_flags &= ~IFF_RUNNING;
@@ -7450,7 +7444,7 @@ iwm_init_task(void *arg1)
}
s = splnet();
 
-   if (sc->sc_flags & IWM_FLAG_HW_INITED)
+   if (ifp->if_flags & IFF_RUNNING)
iwm_stop(ifp, 0);
if ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) == IFF_UP)
iwm_init(ifp);
Index: sys/dev/pci/if_iwmvar.h
===
RCS file: /cvs/src/sys/dev/pci/if_iwmvar.h,v
retrieving revision 1.28
diff -u -p -r1.28 if_iwmvar.h
--- sys/dev/pci/if_iwmvar.h 14 Jun 2017 16:56:04 -  1.28
+++ sys/dev/pci/if_iwmvar.h 14 Jun 2017 19:17:23 -
@@ -280,9 +280,8 @@ struct iwm_rx_ring {
 };
 
 #define IWM_FLAG_USE_ICT   0x01
-#define IWM_FLAG_HW_INITED 0x02
-#define IWM_FLAG_RFKILL0x04
-#define IWM_FLAG_SCANNING  0x08
+#define IWM_FLAG_RFKILL0x02
+#define IWM_FLAG_SCANNING  0x04
 
 struct iwm_ucode_status {
uint32_t uc_error_event_table;

Re: remove redundant flag from iwm(4)

[Mark Kettenis]

> Date: Mon, 19 Jun 2017 13:02:58 +0200
> From: Stefan Sperling 
> 
> On Mon, Jun 19, 2017 at 11:57:36AM +0200, Mark Kettenis wrote:
> > > @@ -7450,7 +7444,7 @@ iwm_init_task(void *arg1)
> > >   }
> > >   s = splnet();
> > >  
> > > - if (sc->sc_flags & IWM_FLAG_HW_INITED)
> > > + if (sc->sc_flags & IFF_RUNNING)
> > >   iwm_stop(ifp, 0);
> > 
> > This looks wrong to me.
> 
> Why?

Because IFF_RUNNING is a flag for ifp->if_flags, not sc->sc_flags.

Re: remove redundant flag from iwm(4)

[Stefan Sperling]

On Mon, Jun 19, 2017 at 11:57:36AM +0200, Mark Kettenis wrote:
> > @@ -7450,7 +7444,7 @@ iwm_init_task(void *arg1)
> > }
> > s = splnet();
> >  
> > -   if (sc->sc_flags & IWM_FLAG_HW_INITED)
> > +   if (sc->sc_flags & IFF_RUNNING)
> > iwm_stop(ifp, 0);
> 
> This looks wrong to me.

Why?

The idea is not to call iwm_stop() if we're resuming.

Re: remove redundant flag from iwm(4)

[Mark Kettenis]

> Date: Fri, 16 Jun 2017 15:39:30 +0200
> From: Stefan Sperling 
> 
> The HW_INITED flag has a grammatically dubious name and is unnecessary
> because it duplicates the purpose of the IFF_RUNNING flag.
> 
> ok?

I don't think so...

> Index: sys/dev/pci/if_iwm.c
> ===
> RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
> retrieving revision 1.197
> diff -u -p -r1.197 if_iwm.c
> --- sys/dev/pci/if_iwm.c  16 Jun 2017 08:45:34 -  1.197
> +++ sys/dev/pci/if_iwm.c  16 Jun 2017 11:47:09 -
> @@ -6106,9 +6106,6 @@ iwm_init(struct ifnet *ifp)
>   struct ieee80211com *ic = >sc_ic;
>   int err, generation;
>  
> - if (sc->sc_flags & IWM_FLAG_HW_INITED) {
> - return 0;
> - }
>   sc->sc_generation++;
>  
>   err = iwm_init_hw(sc);
> @@ -6135,8 +6132,6 @@ iwm_init(struct ifnet *ifp)
>   return err;
>   } while (ic->ic_state != IEEE80211_S_SCAN);
>  
> - sc->sc_flags |= IWM_FLAG_HW_INITED;
> -
>   return 0;
>  }
>  
> @@ -6214,7 +6209,6 @@ iwm_stop(struct ifnet *ifp, int disable)
>   struct ieee80211com *ic = >sc_ic;
>   struct iwm_node *in = (void *)ic->ic_bss;
>  
> - sc->sc_flags &= ~IWM_FLAG_HW_INITED;
>   sc->sc_generation++;
>   ic->ic_scan_lock = IEEE80211_SCAN_UNLOCKED;
>   ifp->if_flags &= ~IFF_RUNNING;
> @@ -7450,7 +7444,7 @@ iwm_init_task(void *arg1)
>   }
>   s = splnet();
>  
> - if (sc->sc_flags & IWM_FLAG_HW_INITED)
> + if (sc->sc_flags & IFF_RUNNING)
>   iwm_stop(ifp, 0);

This looks wrong to me.

>   if ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) == IFF_UP)
>   iwm_init(ifp);
> Index: sys/dev/pci/if_iwmvar.h
> ===
> RCS file: /cvs/src/sys/dev/pci/if_iwmvar.h,v
> retrieving revision 1.28
> diff -u -p -r1.28 if_iwmvar.h
> --- sys/dev/pci/if_iwmvar.h   14 Jun 2017 16:56:04 -  1.28
> +++ sys/dev/pci/if_iwmvar.h   14 Jun 2017 19:17:23 -
> @@ -280,9 +280,8 @@ struct iwm_rx_ring {
>  };
>  
>  #define IWM_FLAG_USE_ICT 0x01
> -#define IWM_FLAG_HW_INITED   0x02
> -#define IWM_FLAG_RFKILL  0x04
> -#define IWM_FLAG_SCANNING0x08
> +#define IWM_FLAG_RFKILL  0x02
> +#define IWM_FLAG_SCANNING0x04
>  
>  struct iwm_ucode_status {
>   uint32_t uc_error_event_table;
> 
> 
> 
> 

remove redundant flag from iwm(4)

[Stefan Sperling]

The HW_INITED flag has a grammatically dubious name and is unnecessary
because it duplicates the purpose of the IFF_RUNNING flag.

ok?

Index: sys/dev/pci/if_iwm.c
===
RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.197
diff -u -p -r1.197 if_iwm.c
--- sys/dev/pci/if_iwm.c16 Jun 2017 08:45:34 -  1.197
+++ sys/dev/pci/if_iwm.c16 Jun 2017 11:47:09 -
@@ -6106,9 +6106,6 @@ iwm_init(struct ifnet *ifp)
struct ieee80211com *ic = >sc_ic;
int err, generation;
 
-   if (sc->sc_flags & IWM_FLAG_HW_INITED) {
-   return 0;
-   }
sc->sc_generation++;
 
err = iwm_init_hw(sc);
@@ -6135,8 +6132,6 @@ iwm_init(struct ifnet *ifp)
return err;
} while (ic->ic_state != IEEE80211_S_SCAN);
 
-   sc->sc_flags |= IWM_FLAG_HW_INITED;
-
return 0;
 }
 
@@ -6214,7 +6209,6 @@ iwm_stop(struct ifnet *ifp, int disable)
struct ieee80211com *ic = >sc_ic;
struct iwm_node *in = (void *)ic->ic_bss;
 
-   sc->sc_flags &= ~IWM_FLAG_HW_INITED;
sc->sc_generation++;
ic->ic_scan_lock = IEEE80211_SCAN_UNLOCKED;
ifp->if_flags &= ~IFF_RUNNING;
@@ -7450,7 +7444,7 @@ iwm_init_task(void *arg1)
}
s = splnet();
 
-   if (sc->sc_flags & IWM_FLAG_HW_INITED)
+   if (sc->sc_flags & IFF_RUNNING)
iwm_stop(ifp, 0);
if ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) == IFF_UP)
iwm_init(ifp);
Index: sys/dev/pci/if_iwmvar.h
===
RCS file: /cvs/src/sys/dev/pci/if_iwmvar.h,v
retrieving revision 1.28
diff -u -p -r1.28 if_iwmvar.h
--- sys/dev/pci/if_iwmvar.h 14 Jun 2017 16:56:04 -  1.28
+++ sys/dev/pci/if_iwmvar.h 14 Jun 2017 19:17:23 -
@@ -280,9 +280,8 @@ struct iwm_rx_ring {
 };
 
 #define IWM_FLAG_USE_ICT   0x01
-#define IWM_FLAG_HW_INITED 0x02
-#define IWM_FLAG_RFKILL0x04
-#define IWM_FLAG_SCANNING  0x08
+#define IWM_FLAG_RFKILL0x02
+#define IWM_FLAG_SCANNING  0x04
 
 struct iwm_ucode_status {
uint32_t uc_error_event_table;