On Mon, Jul 09, 2012 at 02:18:48AM +0300, Lazaros Koromilas wrote:
> On Sun, Jul 08, 2012 at 01:31:43PM -0400, Kenneth R Westerback wrote:
> > On Sun, Jul 08, 2012 at 07:17:21PM +0200, Stefan Sperling wrote:
> > > On Sun, Jul 08, 2012 at 08:00:28PM +0300, Lazaros Koromilas wrote:
> > > > On Sun, Jul 08, 2012 at 10:59:09AM +0200, Stefan Sperling wrote:
> > > > > The linux driver ("iwlegacy") doesn't run this command in async mode.
> > > > > Is there a reason why you're passing 1 for the last param, i.e. not
> > > > > waiting for a command-complete interrupt when sending
> > > > > WPI_CMD_ASSOCIATE?
> > > >
> > > > Not really, no. Fixed that. I added printing because all sync
> > > > command calls are handled this way, but can be removed if it's
> > > > not acceptable.
> > >
> > > I think that printf() is fine.
> > >
> > > > > You don't need all of if_flags, just the IFF_PROMISC bit.
> > > > > Why not add a new flag to sc->sc_flags and use that instead?
> > > >
> > > > You are right, I originally added the extra sc_if_flags in order to XOR
> > > > with if_flags and detect the promisc status change. Does this logic
> > > > seem simpler/better? Also removed the initialization above.
> > >
> > > I don't like this approach because it is adding a new 32bit flags field
> > > to the softc, all for checking a single bit from this flags field,
> > > while the existing sc_flags field has lots of unused bits.
> > >
> > > The xor is cute but usually we just use & to check for flags.
>
> Saw this when studying other if_ drivers and thought so too.
>
> >
> > Or ISSET()!
>
> :)
>
> >
> > .... Ken
> >
> > >
> > > So adding, say, WPI_FLAG_PROMISC to sc_flags and then cross-checking
> > > that with the IFF_PROMISC flag will look nicer IMO.
>
> Diff updated.
Testing this with `ifconfig bridge0 add wpi0' uncovered a fault
in my logic. When the new bridge member is down, ifconfig brings
it up, sets the promiscuous flag and brings it down again. However,
the `wpi_set_promisc' will not work if the interface is not
in associated state. This new diff triggers a reset in this case,
so that promiscuous mode is configured at init. Seems right?
Maybe do the reset unconditionally and get rid of `wpi_set_promisc'?
Suggestions?
Index: if_wpivar.h
===================================================================
RCS file: /cvs/src/sys/dev/pci/if_wpivar.h,v
retrieving revision 1.23
diff -u -p -r1.23 if_wpivar.h
--- if_wpivar.h 7 Sep 2010 16:21:45 -0000 1.23
+++ if_wpivar.h 11 Jul 2012 08:26:50 -0000
@@ -143,6 +143,7 @@ struct wpi_softc {
u_int sc_flags;
#define WPI_FLAG_HAS_5GHZ (1 << 0)
#define WPI_FLAG_BUSY (1 << 1)
+#define WPI_FLAG_PROMISC (1 << 2)
/* Shared area. */
struct wpi_dma_info shared_dma;
Index: if_wpi.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/if_wpi.c,v
retrieving revision 1.110
diff -u -p -r1.110 if_wpi.c
--- if_wpi.c 2 Jun 2011 18:36:53 -0000 1.110
+++ if_wpi.c 11 Jul 2012 08:26:51 -0000
@@ -120,6 +120,7 @@ int wpi_ioctl(struct ifnet *, u_long, c
int wpi_cmd(struct wpi_softc *, int, const void *, int, int);
int wpi_mrr_setup(struct wpi_softc *);
void wpi_updateedca(struct ieee80211com *);
+int wpi_set_promisc(struct wpi_softc *);
void wpi_set_led(struct wpi_softc *, uint8_t, uint8_t, uint8_t);
int wpi_set_timing(struct wpi_softc *, struct ieee80211_node *);
void wpi_power_calibration(struct wpi_softc *);
@@ -2002,7 +2003,18 @@ wpi_ioctl(struct ifnet *ifp, u_long cmd,
/* FALLTHROUGH */
case SIOCSIFFLAGS:
if (ifp->if_flags & IFF_UP) {
- if (!(ifp->if_flags & IFF_RUNNING))
+ if (ifp->if_flags & IFF_RUNNING) {
+ if (((ifp->if_flags & IFF_PROMISC) &&
+ !(sc->sc_flags & WPI_FLAG_PROMISC)) ||
+ (!(ifp->if_flags & IFF_PROMISC) &&
+ (sc->sc_flags & WPI_FLAG_PROMISC))) {
+ if (ic->ic_state == IEEE80211_S_RUN)
+ error = wpi_set_promisc(sc);
+ else
+ error = ENETRESET;
+ sc->sc_flags ^= WPI_FLAG_PROMISC;
+ }
+ } else
error = wpi_init(ifp);
} else {
if (ifp->if_flags & IFF_RUNNING)
@@ -2205,6 +2217,34 @@ wpi_updateedca(struct ieee80211com *ic)
#undef WPI_EXP2
}
+int
+wpi_set_promisc(struct wpi_softc *sc)
+{
+ struct ieee80211com *ic = &sc->sc_ic;
+ struct ifnet *ifp = &ic->ic_if;
+ struct wpi_assoc cmd;
+ int error;
+
+ if (ifp->if_flags & IFF_PROMISC)
+ sc->rxon.filter |= htole32(WPI_FILTER_PROMISC |
+ WPI_FILTER_CTL);
+ else
+ sc->rxon.filter &= ~htole32(WPI_FILTER_PROMISC |
+ WPI_FILTER_CTL);
+
+ memset(&cmd, 0, sizeof cmd);
+ cmd.flags = sc->rxon.flags;
+ cmd.filter = sc->rxon.filter;
+ cmd.ofdm_mask = sc->rxon.ofdm_mask;
+ cmd.cck_mask = sc->rxon.cck_mask;
+ error = wpi_cmd(sc, WPI_CMD_ASSOCIATE, &cmd, sizeof cmd, 0);
+ if (error != 0) {
+ printf("%s: could not set filter\n", sc->sc_dev.dv_xname);
+ return error;
+ }
+ return 0;
+}
+
void
wpi_set_led(struct wpi_softc *sc, uint8_t which, uint8_t off, uint8_t on)
{
@@ -2490,6 +2530,9 @@ wpi_config(struct wpi_softc *sc)
/* Should not get there. */
break;
}
+ if (ifp->if_flags & IFF_PROMISC)
+ sc->rxon.filter |= htole32(WPI_FILTER_PROMISC |
+ WPI_FILTER_CTL);
sc->rxon.cck_mask = 0x0f; /* not yet negotiated */
sc->rxon.ofdm_mask = 0xff; /* not yet negotiated */
DPRINTF(("setting configuration\n"));
