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.
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.
>
> >
> >
> > 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
