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.
So adding, say, WPI_FLAG_PROMISC to sc_flags and then cross-checking
that with the IFF_PROMISC flag will look nicer IMO.
>
>
> 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 8 Jul 2012 16:45:14 -0000
> @@ -144,6 +144,8 @@ struct wpi_softc {
> #define WPI_FLAG_HAS_5GHZ (1 << 0)
> #define WPI_FLAG_BUSY (1 << 1)
>
> + int sc_if_flags;
> +
> /* Shared area. */
> struct wpi_dma_info shared_dma;
> struct wpi_shared *shared;
> 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 8 Jul 2012 16:45:15 -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,12 +2003,17 @@ 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 ^ sc->sc_if_flags) &
> + IFF_PROMISC)
> + error = wpi_set_promisc(sc);
> + } else
> error = wpi_init(ifp);
> } else {
> if (ifp->if_flags & IFF_RUNNING)
> wpi_stop(ifp, 1);
> }
> + sc->sc_if_flags = ifp->if_flags;
> break;
>
> case SIOCADDMULTI:
> @@ -2203,6 +2209,34 @@ wpi_updateedca(struct ieee80211com *ic)
> }
> (void)wpi_cmd(sc, WPI_CMD_EDCA_PARAMS, &cmd, sizeof cmd, 1);
> #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
