Hi Lennert,
Are you bringing your laptop? Want to do a little direct review next
week? If I can go home, I probably won't bring a laptop though.
> > > +static inline u16 mwl8k_qos_setbit_qlen(u16 qos, u8 len)
> >
> > Why do you need to work with the QoS header?
>
> The firmware expects that we send it a 4-address header without a
> QoS field, and any QoS info (if present) should be passed via the TX
> descriptor, which is why we need these. Does that make sense?
So the QoS info in the TX descriptor doesn't match the layout in the
frame and you need to translate it? I guess that makes sense.
> Actually, the firmware always runs in little endian. I have yet to
> test it on a big-endian host but I've done a pass over the driver to
> make sure that all firmware struct fields >8 bits are annotated
> as __le and that we use cpu_to_leXX/leXX_to_cpu in the right places,
> and I think it's okay now. (And I think we got rid of the bitfields
> before the first submission.)
Sounds good :) If you've got pcie hardware but no big endian machine, I
can test it for you if you want.
> > > + /*
> > > + * Copy up/down the 802.11 header; the firmware requires
> > > + * we present a 2-byte payload length followed by a
> > > + * 4-address header (w/o QoS), followed (optionally) by
> > > + * any WEP/ExtIV header (but only filled in for CCMP).
> > > + */
> > > + if (hdrlen < sizeof(struct mwl8k_dma_data)) {
> >
> > That comparison seems slightly odd to me
>
> I double-checked it, but it seems correct, even if it'll always
> evaluate as true for now. In the theoretical case we get a 4-address
> header here (which can't happen for now) with a QoS field,
> hdrlen == sizeof(struct mwl8k_dma_data) and we don't need to
> skb_push(), since we'll just push the header 2 bytes forward (over
> the QoS field, moving that to the descriptor) and prepend a 16bit
> length field to the packet and end up with a payload of the same size.
Right, I probably just got confused. Don't even remember.
> > > + /* Clear addr4 */
> > > + memset(tr->wh.addr4, 0, IEEE80211_ADDR_LEN);
> >
> > ?
>
> As per above, the firmware always expects a 4-address header, but we
> won't actually end up with a 4-address header here currently, so we
> can just unconditionally write zeroes here. This might need to be
> revisited in the future.
Yeah, if you want to support 4addr (WDS).
> > > +/*
> > > + * Scan a list of BSSIDs to process for finalize join.
> > > + * Allows for extension to process multiple BSSIDs.
> > > + */
> >
> > Can you explain why you need to do this much BSSID stuff? Maybe we
> > can extend mac80211 a little instead?
>
> There is a firmware command FINALIZE_JOIN which expects a copy of
> the beacon used for association. From the beacon it uses the BSSID
> and TSF timer (to program the PS timers), and possibly a couple of
> other fields (I'd have to check), which is why we have
> priv->capture_bssid and priv->beacon_skb. Ideas? I guess we could
> have mac80211 pass us a copy of the beacon it used for association,
> but I don't think any other card needs this...
Might make sense to do that with some of the new cfg80211 assoc stuff
that might have that anyway, but it seems ok to keep for now then. I
guess mac80211 kinda passes all the information that the firmware will
parse out of the beacon in an already parsed form.
> We just ended up always queueing the work, since it's more consistent
> that way, and you'll have to wait in line anyway since you don't want
> to end up breaking up already-queued command sequences that the
> firmware expects to be sequential. So it seemed simpler to just use
> the existing mechanism.
Ok, except, where does the requirement to use a workqueue come from at
all? Can we remove that entirely maybe? Or is it the filter flags stuff
(where we can't change atomicity requirement)?
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
