[PATCH v4 3/4] ath11k: add support for device recovery for QCA6390
Kalle Valo
kvalo at codeaurora.org
Wed Nov 17 00:12:55 PST 2021
Sven Eckelmann <sven at narfation.org> writes:
> On Tuesday, 16 November 2021 05:15:21 CET Wen Gong wrote:
>> Currently ath11k has device recovery logic, it is introduced by this
>> patch "ath11k: Add support for subsystem recovery" which is upstream
>> by
>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=ath11k-bringup&id=3a7b4838b6f6f234239f263ef3dc02e612a083ad.
>
> https://www.kernel.org/doc/html/v5.15/process/submitting-patches.html#describe-your-changes
>
> Please search for "If you want to refer to a specific commit"
> And this commit you referenced is definitely not the upstream commit.
> It was only part of Kalle's repository. The code was only upstreamed
> with commit d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax
> devices").
>
>
> Btw. another thing which I see again and again in these patches:
>
>> --- a/drivers/net/wireless/ath/ath11k/core.h
>> +++ b/drivers/net/wireless/ath/ath11k/core.h
>> @@ -39,6 +39,10 @@
> [...]
>> + bool is_reset;
>
> https://www.kernel.org/doc/html/v5.15/process/coding-style.html#using-bool
>
> "If a structure has many true/false values, consider consolidating them into a
> bitfield with 1 bit members, or using an appropriate fixed width type, such as
> u8."
>
> There are more verbose mails from Linus Torvalds where he points out the
> problems of bools in structs. See for example struct ath11k_skb_rxcb. At
> the moment, it looks like this:
>
> struct ath11k_skb_rxcb {
> dma_addr_t paddr;
> bool is_first_msdu;
> bool is_last_msdu;
> bool is_continuation;
> bool is_mcbc;
> bool is_eapol_tkip;
> struct hal_rx_desc *rx_desc;
> u8 err_rel_src;
> u8 err_code;
> u8 mac_id;
> u8 unmapped;
> u8 is_frag;
> u8 tid;
> u16 peer_id;
> u16 seq_no;
> struct napi_struct *napi;
> };
>
> Compiled for IPQ60xx, it would end up as:
>
> $ pahole ./drivers/net/wireless/ath/ath11k/ath11k.o -C ath11k_skb_rxcb
> struct ath11k_skb_rxcb {
> dma_addr_t paddr; /* 0 8 */
> bool is_first_msdu; /* 8 1 */
> bool is_last_msdu; /* 9 1 */
> bool is_continuation; /* 10 1 */
> bool is_mcbc; /* 11 1 */
> bool is_eapol_tkip; /* 12 1 */
>
> /* XXX 3 bytes hole, try to pack */
>
> struct hal_rx_desc * rx_desc; /* 16 8 */
> u8 err_rel_src; /* 24 1 */
> u8 err_code; /* 25 1 */
> u8 mac_id; /* 26 1 */
> u8 unmapped; /* 27 1 */
> u8 is_frag; /* 28 1 */
> u8 tid; /* 29 1 */
> u16 peer_id; /* 30 2 */
> u16 seq_no; /* 32 2 */
>
> /* XXX 6 bytes hole, try to pack */
>
> struct napi_struct * napi; /* 40 8 */
>
> /* size: 48, cachelines: 1, members: 16 */
> /* sum members: 39, holes: 2, sum holes: 9 */
> /* last cacheline: 48 bytes */
> };
>
> After changing it to u8 ....:1 and reorganizing the structure:
>
> $ pahole drivers/net/wireless/ath/ath11k/ath11k.o -C ath11k_skb_rxcb -R
> struct ath11k_skb_rxcb {
> dma_addr_t paddr; /* 0 8 */
> struct hal_rx_desc * rx_desc; /* 8 8 */
> struct napi_struct * napi; /* 16 8 */
> u8 err_rel_src; /* 24 1 */
> u8 err_code; /* 25 1 */
> u8 mac_id; /* 26 1 */
> u8 unmapped; /* 27 1 */
> u8 is_frag; /* 28 1 */
> u8 tid; /* 29 1 */
> u8 is_first_msdu:1; /* 30: 0 1 */
> u8 is_last_msdu:1; /* 30: 1 1 */
> u8 is_continuation:1; /* 30: 2 1 */
> u8 is_mcbc:1; /* 30: 3 1 */
> u8 is_eapol_tkip:1; /* 30: 4 1 */
>
> /* XXX 3 bits hole, try to pack */
> /* XXX 1 byte hole, try to pack */
>
> u16 peer_id; /* 32 2 */
> u16 seq_no; /* 34 2 */
>
> /* size: 40, cachelines: 1, members: 16 */
> /* sum members: 34, holes: 1, sum holes: 1 */
> /* sum bitfield members: 5 bits, bit holes: 1, sum bit holes: 3 bits */
> /* padding: 4 */
> /* last cacheline: 40 bytes */
> };
>
>
>
> Or ath11k_bus_params:
>
> $ pahole ./drivers/net/wireless/ath/ath11k/ath11k.o -C ath11k_bus_params
> struct ath11k_bus_params {
> bool mhi_support; /* 0 1 */
> bool m3_fw_support; /* 1 1 */
> bool fixed_bdf_addr; /* 2 1 */
> bool fixed_mem_region; /* 3 1 */
> bool static_window_map; /* 4 1 */
>
> /* size: 5, cachelines: 1, members: 5 */
> /* last cacheline: 5 bytes */
> };
>
> After changing it to an "u8 ....:1":
>
> $ pahole ./drivers/net/wireless/ath/ath11k/ath11k.o -C ath11k_bus_params
> struct ath11k_bus_params {
> u8 mhi_support:1; /* 0: 0 1 */
> u8 m3_fw_support:1; /* 0: 1 1 */
> u8 fixed_bdf_addr:1; /* 0: 2 1 */
> u8 fixed_mem_region:1; /* 0: 3 1 */
> u8 static_window_map:1; /* 0: 4 1 */
>
> /* size: 1, cachelines: 1, members: 5 */
> /* bit_padding: 3 bits */
> /* last cacheline: 1 bytes */
> };
>
>
> There are even better examples. ath11k_hw_params will for example take
> currently 200 bytes. With a little reordering and adjusting of bools,
> it can easily be reduced to 152 bytes. But other structures might
> have more impact because they are used more often.
Yeah, I have been worried about this as well and we should fix this. But
instead of u8 I would prefer to use bool like mt76 uses:
struct mt76_queue_entry {
union {
void *buf;
struct sk_buff *skb;
};
union {
struct mt76_txwi_cache *txwi;
struct urb *urb;
int buf_sz;
};
u32 dma_addr[2];
u16 dma_len[2];
u16 wcid;
bool skip_buf0:1;
bool skip_buf1:1;
bool done:1;
};
I didn't even know using bool is legal until I saw it in mt76.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
More information about the ath11k
mailing list