Johannes Berg <johannes@xxxxxxxxxxxxxxxx> writes:
> On Mon, 2015-08-31 at 19:42 -0400, Jes Sorensen wrote:
>>
>> Anything related to doing things correctly wrt to the mac80211 API
>> would be awesome.
>
> :)
>
>> static int rtl8xxxu_add_interface(struct ieee80211_hw *hw,
>> struct ieee80211_vif *vif)
>> {
>> struct rtl8xxxu_priv *priv = hw->priv;
>> int ret;
>> u8 val8;
>>
>> switch (vif->type) {
>> case NL80211_IFTYPE_STATION:
>> rtl8723a_stop_tx_beacon(priv);
>
> This seems odd - you shouldn't have any kind of operation before
> add_interface(), so why stop anything?
> (You also don't even support beaconing yet anyway)
The code mimics what the vendor driver does - part of the bringup of the
chip sets a bunch of parameters from an register value array. Rather
than guessing what their default is, I feel it's safer to just shut it
down here and stick with that.
>
>> /*
>> * This is a bit of a hack - the lower bits of the cipher
>> * suite selector happens to match the cipher index in the
>> * CAM
>> */
>
> That's probably simply intentional - the cipher suite selector is built
> as a OUI : cipher number ...
>
> However - I don't see any code actually using key->cipher here? You
> should move the comment into rtl8xxxu_cam_write() I guess.
I did assume it was intentional, but still a bit of a hack in my book.
I'll move the comment, as you suggested.
>> static int
>> rtl8xxxu_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>> enum ieee80211_ampdu_mlme_action action,
>> struct ieee80211_sta *sta, u16 tid, u16 *ssn, u8 buf_size)
>> {
>> struct rtl8xxxu_priv *priv = hw->priv;
>> struct device *dev = &priv->udev->dev;
>> u8 ampdu_factor, ampdu_density;
>>
>> switch (action) {
> [...]
>
> I'd be extremely surprised if any of this worked - perhaps you're not
> advertising A-MPDU support (yet)?
>
> You're not calling ieee80211_start_tx_ba_cb_irqsafe() or
> ieee80211_stop_tx_ba_cb_irqsafe(), so this can't really work. The
> session might be set up but will never actually start.
>
> That said, the code also looks incomplete. Perhaps better to just
> remove it entirely for now?
I started working on this, but I guess I didn't finish it. One thing I
haven't figured out yet is why I see AMPDU_RX_START events, but I never
see AMPDU_TX_START events show in the log.
>> sta_priv->short_preamble = false;
>
> I don't think there's any need to track it, especially since you only
> use it in TX where you have the TX info data about it.
Oh, I hadn't spotted that information in struct tx_info - in that case I
ought to be able to get rid of this one.
>
>> };
>
> stray semicolons at the end of functions in a few places :)
Gone :)
> I guess you can get rid of sta_add/remove entirely. sta_state() is the
> better hook to implement, but you don't really need any of the three.
I added it because I was looking at using it for AMPDU, then got stock,
but didn't feel like removing it in case I would need it later.
I need to figure out what goes on with the AMPDU side of things - the
problem writing a driver like this without any spec sheets is that it
isn't quite clear to me, how much of this the firmware takes care of
transparently.
>> static void rtl8xxxu_sw_scan_start(struct ieee80211_hw *hw,
>> struct ieee80211_vif *vif, const u8 *mac)
>> {
>> }
>
> No need for this ... You'll get stop even without the start, but
> actually I think your start is perhaps missing setting
> BEACON_DISABLE_TSF_UPDATE (which stop clears)
I'll have a look at this.
Thanks for the comments!
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
