Pkshih [off-list ref] writes:

quoted

-----Original Message-----
From: kvalo=codeaurora.org@mg.codeaurora.org
[off-list ref] On
Behalf Of Kalle Valo
Sent: Saturday, October 2, 2021 12:26 AM
To: Pkshih <pkshih@realtek.com>
Cc: linux-wireless@vger.kernel.org
Subject: Re: [PATCH v6 03/24] rtw89: add core and trx files

Ping-Ke Shih [off-list ref] writes:

quoted

Implement main flows that contains register/unregister mac80211 hw with
hardware capability, power on/off sequence, STA state actions, and
TX/RX path.

The chip info is read from efuse while probing PCI, and then it can be
used to induce supported channel, band, bitrate, ht/vht/he capability,
and etc. Then, we register hardware with these capabilities.

When network interface is up, driver does power-on sequence to enable MAC,
BB and RF function blocks. Oppositely, do power-off sequence when
interface is going to down.

To maintain STA state, five callbacks are implemented -- add, assoc,
disassoc, disconnect and remove. In which state, driver tells firmware STA
info via H2C.

TX flow:
When a SKB is going to be transmitted, we must know its type first. If
the type is mgmt or fwcmd made by driver, SKB is queued into corresponding
DMA channel and PCI ring. The other type is data frame that is more
complex, because it needs to establish BA session to have better throughput
with AMPDU and AMSDU.
In order to have better PCI DMA efficiency, we don't kick off DMA every
SKB. With wake TX queue, kick off DMA after a bunch of SKBs are written.
To achieve this, we have two HCI ops -- tx_write and tx_kick_off.

BA establishment work:
For data frames, we start to establish BA session if the STA is associated
with APMDU capability and the TID session isn't established, and then the
BA work is used to ask mac80211 to start AMPDU actions. Driver implements
AMPDU action callbacks to know the session is established, so that we can
set AGG_EN bit in TX descriptor to enable AMPDU.

RX flow:
When a RX SKB is delivered from PCI, rtw89_core_rx() process it depneds on
its type -- WIFI, C2H or PPDU. If type is C2H, it's queued into a C2H
queue, and wake a work to handle the C2H packet. If type is WIFI, it's a
normal RX packet. When mgmt or data frame is received, it is queued
into pending RX SKB queue to wait for corresponding PPDU packet (another
RX packet with PPDU type) to fill its rx_status, like RSSI. And, then
indicate this packet to mac80211. When control frame is received, indicate
it to mac80211 immediately.

Track work:
Use track work to monitor PHY status to know the changes of environment,
and then update RA status or do RFK accordingly.

Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>

[...]

quoted

+static __always_inline void RTW89_SET_TXWD(u8 *txdesc, u32 val,
u8 offset, u32 mask)
+{
+	u32 *txd32 = (u32 *)txdesc;
+
+	le32p_replace_bits((__le32 *)(txd32 + offset), val, mask);
+}

I'm not convinced about this either, please just use inline.

This is because 'mask' argument of le32p_replace_bits() must be constant
only. If I use inline and build this driver with ccflags-y += -Os,
compiler reports errors:

In function 'field_multiplier',
    inlined from 'le32_encode_bits' at ./include/linux/bitfield.h:154:1,
    inlined from 'le32p_replace_bits' at ./include/linux/bitfield.h:154:1,
    inlined from 'RTW89_SET_FWCMD_UA32.constprop' at /work/git-root/rtwlan/rtw89/fw.h:1397:2:
./include/linux/bitfield.h:119:3: error: call to '__bad_mask' declared with attribute error: bad bitfield mask
  119 |   __bad_mask();
      |   ^~~~~~~~~~~~

I check the implement of le32p_replace_bits(), it looks like

static __always_inline void type##p_replace_bits(__##type *p,           \
                                        base val, base field)           \
{                                                                       \
        *p = (*p & ~to(field)) | type##_encode_bits(val, field);        \
}

So, I imitate the function to use __always_inline, and then it works.

Do you think I don't need to consider the case of Os?
But, -Os seems a standard option of Linux kernel.

ifdef CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE
KBUILD_CFLAGS += -O2
else ifdef CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE_O3
KBUILD_CFLAGS += -O3
else ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE
KBUILD_CFLAGS += -Os
endif

Yeah, we need to support -Os.

Arnd, what do you suggest? Is __always_inline good solution for this? I
think we should at least add a comment explaining why it's needed.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches