Sven Eckelmann <sven@xxxxxxxxxxxxx> 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
