| Message ID | TYZPR01MB55563B3A689D54D18179E5B4C9192@TYZPR01MB5556.apcprd01.prod.exchangelabs.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: ath11k: fix remapped ce accessing issue on 64bit OS | expand |
On 5/1/24 11:14 AM, Ziyang Huang wrote: > On 64bit OS, when ab->mem_ce is lower than or 4G far away from ab->mem, > u32 is not enough to store the offsets, which makes ath11k_ahb_read32() > and ath11k_ahb_write32() access incorrect address and causes Data Abort > Exception. > > Let's use the high bits of offsets to decide where to access, which is > similar as ath11k_pci_get_window_start() done. In the future, we can merge > these functions for unified regs accessing. > > Signed-off-by: Ziyang Huang <hzyitc@outlook.com> > --- > drivers/net/wireless/ath/ath11k/ahb.c | 34 ++++++++++++++++++++------- > drivers/net/wireless/ath/ath11k/hal.c | 17 +++++--------- > drivers/net/wireless/ath/ath11k/hw.c | 14 +++++------ > drivers/net/wireless/ath/ath11k/hw.h | 7 +++++- > 4 files changed, 45 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c > index 7c0a23517949..9e59b4de93a9 100644 > --- a/drivers/net/wireless/ath/ath11k/ahb.c > +++ b/drivers/net/wireless/ath/ath11k/ahb.c > @@ -198,12 +198,30 @@ static const struct ath11k_pci_ops ath11k_ahb_pci_ops_wcn6750 = { > > static inline u32 ath11k_ahb_read32(struct ath11k_base *ab, u32 offset) > { > - return ioread32(ab->mem + offset); > + switch (offset & ATH11K_REG_TYPE_MASK) { > + case ATH11K_REG_TYPE_NORMAL: > + return ioread32(ab->mem + FIELD_GET(ATH11K_REG_OFFSET_MASK, offset)); > + case ATH11K_REG_TYPE_CE: > + return ioread32(ab->mem_ce + FIELD_GET(ATH11K_REG_OFFSET_MASK, offset)); > + default: > + BUG(); Do you really want to crash the system here? A dev_warn() or something similar would log the situation. I suspect this case is never taken, but a system crash is not a good response if it is. > + return 0; > + } > } > > static inline void ath11k_ahb_write32(struct ath11k_base *ab, u32 offset, u32 value) > { > - iowrite32(value, ab->mem + offset); > + switch (offset & ATH11K_REG_TYPE_MASK) { > + case ATH11K_REG_TYPE_NORMAL: > + iowrite32(value, ab->mem + FIELD_GET(ATH11K_REG_OFFSET_MASK, offset)); > + break; > + case ATH11K_REG_TYPE_CE: > + iowrite32(value, ab->mem_ce + FIELD_GET(ATH11K_REG_OFFSET_MASK, offset)); > + break; > + default: > + BUG(); Ditto. Larry
On 5/1/2024 9:14 AM, Ziyang Huang wrote: > On 64bit OS, when ab->mem_ce is lower than or 4G far away from ab->mem, > u32 is not enough to store the offsets, which makes ath11k_ahb_read32() > and ath11k_ahb_write32() access incorrect address and causes Data Abort > Exception. Are you actually observing this issue? Or is this a hypothetical situation? > > Let's use the high bits of offsets to decide where to access, which is > similar as ath11k_pci_get_window_start() done. In the future, we can merge > these functions for unified regs accessing. Performing unnecessary tests and masking for every ioread/write operation will potentially impact performance. What other fixes were considered (i.e. did you consider making all the register addresses u64?) > > Signed-off-by: Ziyang Huang <hzyitc@outlook.com> > --- > drivers/net/wireless/ath/ath11k/ahb.c | 34 ++++++++++++++++++++------- > drivers/net/wireless/ath/ath11k/hal.c | 17 +++++--------- > drivers/net/wireless/ath/ath11k/hw.c | 14 +++++------ > drivers/net/wireless/ath/ath11k/hw.h | 7 +++++- > 4 files changed, 45 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c > index 7c0a23517949..9e59b4de93a9 100644 > --- a/drivers/net/wireless/ath/ath11k/ahb.c > +++ b/drivers/net/wireless/ath/ath11k/ahb.c > @@ -198,12 +198,30 @@ static const struct ath11k_pci_ops ath11k_ahb_pci_ops_wcn6750 = { > > static inline u32 ath11k_ahb_read32(struct ath11k_base *ab, u32 offset) > { > - return ioread32(ab->mem + offset); > + switch (offset & ATH11K_REG_TYPE_MASK) { > + case ATH11K_REG_TYPE_NORMAL: > + return ioread32(ab->mem + FIELD_GET(ATH11K_REG_OFFSET_MASK, offset)); > + case ATH11K_REG_TYPE_CE: > + return ioread32(ab->mem_ce + FIELD_GET(ATH11K_REG_OFFSET_MASK, offset)); > + default: > + BUG(); you can WARN but you can't BUG (and even WARN is being discouraged) > + return 0; > + } > } > > static inline void ath11k_ahb_write32(struct ath11k_base *ab, u32 offset, u32 value) > { > - iowrite32(value, ab->mem + offset); > + switch (offset & ATH11K_REG_TYPE_MASK) { > + case ATH11K_REG_TYPE_NORMAL: > + iowrite32(value, ab->mem + FIELD_GET(ATH11K_REG_OFFSET_MASK, offset)); > + break; > + case ATH11K_REG_TYPE_CE: > + iowrite32(value, ab->mem_ce + FIELD_GET(ATH11K_REG_OFFSET_MASK, offset)); > + break; > + default: > + BUG(); ditto > + break; > + } > } > > static void ath11k_ahb_kill_tasklets(struct ath11k_base *ab) > @@ -275,9 +293,9 @@ static void ath11k_ahb_ce_irq_enable(struct ath11k_base *ab, u16 ce_id) > const struct ce_ie_addr *ce_ie_addr = ab->hw_params.ce_ie_addr; > u32 ie1_reg_addr, ie2_reg_addr, ie3_reg_addr; > > - ie1_reg_addr = ce_ie_addr->ie1_reg_addr + ATH11K_CE_OFFSET(ab); > - ie2_reg_addr = ce_ie_addr->ie2_reg_addr + ATH11K_CE_OFFSET(ab); > - ie3_reg_addr = ce_ie_addr->ie3_reg_addr + ATH11K_CE_OFFSET(ab); > + ie1_reg_addr = ce_ie_addr->ie1_reg_addr; > + ie2_reg_addr = ce_ie_addr->ie2_reg_addr; > + ie3_reg_addr = ce_ie_addr->ie3_reg_addr; > > ce_attr = &ab->hw_params.host_ce_config[ce_id]; > if (ce_attr->src_nentries) > @@ -296,9 +314,9 @@ static void ath11k_ahb_ce_irq_disable(struct ath11k_base *ab, u16 ce_id) > const struct ce_ie_addr *ce_ie_addr = ab->hw_params.ce_ie_addr; > u32 ie1_reg_addr, ie2_reg_addr, ie3_reg_addr; > > - ie1_reg_addr = ce_ie_addr->ie1_reg_addr + ATH11K_CE_OFFSET(ab); > - ie2_reg_addr = ce_ie_addr->ie2_reg_addr + ATH11K_CE_OFFSET(ab); > - ie3_reg_addr = ce_ie_addr->ie3_reg_addr + ATH11K_CE_OFFSET(ab); > + ie1_reg_addr = ce_ie_addr->ie1_reg_addr; > + ie2_reg_addr = ce_ie_addr->ie2_reg_addr; > + ie3_reg_addr = ce_ie_addr->ie3_reg_addr; > > ce_attr = &ab->hw_params.host_ce_config[ce_id]; > if (ce_attr->src_nentries) > diff --git a/drivers/net/wireless/ath/ath11k/hal.c b/drivers/net/wireless/ath/ath11k/hal.c > index f3d04568c221..f9ba2f821108 100644 > --- a/drivers/net/wireless/ath/ath11k/hal.c > +++ b/drivers/net/wireless/ath/ath11k/hal.c > @@ -1233,20 +1233,16 @@ static int ath11k_hal_srng_create_config(struct ath11k_base *ab) > s->reg_start[1] = HAL_SEQ_WCSS_UMAC_TCL_REG + HAL_TCL_STATUS_RING_HP; > > s = &hal->srng_config[HAL_CE_SRC]; > - s->reg_start[0] = HAL_SEQ_WCSS_UMAC_CE0_SRC_REG(ab) + HAL_CE_DST_RING_BASE_LSB + > - ATH11K_CE_OFFSET(ab); > - s->reg_start[1] = HAL_SEQ_WCSS_UMAC_CE0_SRC_REG(ab) + HAL_CE_DST_RING_HP + > - ATH11K_CE_OFFSET(ab); > + s->reg_start[0] = HAL_SEQ_WCSS_UMAC_CE0_SRC_REG(ab) + HAL_CE_DST_RING_BASE_LSB; > + s->reg_start[1] = HAL_SEQ_WCSS_UMAC_CE0_SRC_REG(ab) + HAL_CE_DST_RING_HP; > s->reg_size[0] = HAL_SEQ_WCSS_UMAC_CE1_SRC_REG(ab) - > HAL_SEQ_WCSS_UMAC_CE0_SRC_REG(ab); > s->reg_size[1] = HAL_SEQ_WCSS_UMAC_CE1_SRC_REG(ab) - > HAL_SEQ_WCSS_UMAC_CE0_SRC_REG(ab); > > s = &hal->srng_config[HAL_CE_DST]; > - s->reg_start[0] = HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab) + HAL_CE_DST_RING_BASE_LSB + > - ATH11K_CE_OFFSET(ab); > - s->reg_start[1] = HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab) + HAL_CE_DST_RING_HP + > - ATH11K_CE_OFFSET(ab); > + s->reg_start[0] = HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab) + HAL_CE_DST_RING_BASE_LSB; > + s->reg_start[1] = HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab) + HAL_CE_DST_RING_HP; > s->reg_size[0] = HAL_SEQ_WCSS_UMAC_CE1_DST_REG(ab) - > HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab); > s->reg_size[1] = HAL_SEQ_WCSS_UMAC_CE1_DST_REG(ab) - > @@ -1254,9 +1250,8 @@ static int ath11k_hal_srng_create_config(struct ath11k_base *ab) > > s = &hal->srng_config[HAL_CE_DST_STATUS]; > s->reg_start[0] = HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab) + > - HAL_CE_DST_STATUS_RING_BASE_LSB + ATH11K_CE_OFFSET(ab); > - s->reg_start[1] = HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab) + HAL_CE_DST_STATUS_RING_HP + > - ATH11K_CE_OFFSET(ab); > + HAL_CE_DST_STATUS_RING_BASE_LSB; > + s->reg_start[1] = HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab) + HAL_CE_DST_STATUS_RING_HP; > s->reg_size[0] = HAL_SEQ_WCSS_UMAC_CE1_DST_REG(ab) - > HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab); > s->reg_size[1] = HAL_SEQ_WCSS_UMAC_CE1_DST_REG(ab) - > diff --git a/drivers/net/wireless/ath/ath11k/hw.c b/drivers/net/wireless/ath/ath11k/hw.c > index caa6dc12a790..58f39a7eaa1c 100644 > --- a/drivers/net/wireless/ath/ath11k/hw.c > +++ b/drivers/net/wireless/ath/ath11k/hw.c > @@ -2268,9 +2268,9 @@ const struct ce_ie_addr ath11k_ce_ie_addr_ipq8074 = { > }; > > const struct ce_ie_addr ath11k_ce_ie_addr_ipq5018 = { > - .ie1_reg_addr = CE_HOST_IPQ5018_IE_ADDRESS - HAL_IPQ5018_CE_WFSS_REG_BASE, > - .ie2_reg_addr = CE_HOST_IPQ5018_IE_2_ADDRESS - HAL_IPQ5018_CE_WFSS_REG_BASE, > - .ie3_reg_addr = CE_HOST_IPQ5018_IE_3_ADDRESS - HAL_IPQ5018_CE_WFSS_REG_BASE, > + .ie1_reg_addr = ATH11K_REG_TYPE_CE + CE_HOST_IPQ5018_IE_ADDRESS - HAL_IPQ5018_CE_WFSS_REG_BASE, > + .ie2_reg_addr = ATH11K_REG_TYPE_CE + CE_HOST_IPQ5018_IE_2_ADDRESS - HAL_IPQ5018_CE_WFSS_REG_BASE, > + .ie3_reg_addr = ATH11K_REG_TYPE_CE + CE_HOST_IPQ5018_IE_3_ADDRESS - HAL_IPQ5018_CE_WFSS_REG_BASE, > }; > > const struct ce_remap ath11k_ce_remap_ipq5018 = { > @@ -2801,13 +2801,13 @@ const struct ath11k_hw_regs ipq5018_regs = { > .hal_reo_status_hp = 0x00003070, > > /* WCSS relative address */ > - .hal_seq_wcss_umac_ce0_src_reg = 0x08400000 > + .hal_seq_wcss_umac_ce0_src_reg = ATH11K_REG_TYPE_CE + 0x08400000 > - HAL_IPQ5018_CE_WFSS_REG_BASE, > - .hal_seq_wcss_umac_ce0_dst_reg = 0x08401000 > + .hal_seq_wcss_umac_ce0_dst_reg = ATH11K_REG_TYPE_CE + 0x08401000 > - HAL_IPQ5018_CE_WFSS_REG_BASE, > - .hal_seq_wcss_umac_ce1_src_reg = 0x08402000 > + .hal_seq_wcss_umac_ce1_src_reg = ATH11K_REG_TYPE_CE + 0x08402000 > - HAL_IPQ5018_CE_WFSS_REG_BASE, > - .hal_seq_wcss_umac_ce1_dst_reg = 0x08403000 > + .hal_seq_wcss_umac_ce1_dst_reg = ATH11K_REG_TYPE_CE + 0x08403000 > - HAL_IPQ5018_CE_WFSS_REG_BASE, > > /* WBM Idle address */ > diff --git a/drivers/net/wireless/ath/ath11k/hw.h b/drivers/net/wireless/ath/ath11k/hw.h > index 14ef4eb48f80..44593b38fc85 100644 > --- a/drivers/net/wireless/ath/ath11k/hw.h > +++ b/drivers/net/wireless/ath/ath11k/hw.h > @@ -81,7 +81,12 @@ > #define ATH11K_M3_FILE "m3.bin" > #define ATH11K_REGDB_FILE_NAME "regdb.bin" > > -#define ATH11K_CE_OFFSET(ab) (ab->mem_ce - ab->mem) > +#define ATH11K_REG_TYPE_MASK GENMASK(31, 28) > +#define ATH11K_REG_TYPE(x) FIELD_PREP_CONST(ATH11K_REG_TYPE_MASK, x) > +#define ATH11K_REG_TYPE_NORMAL ATH11K_REG_TYPE(0) > +#define ATH11K_REG_TYPE_DP ATH11K_REG_TYPE(1) > +#define ATH11K_REG_TYPE_CE ATH11K_REG_TYPE(2) > +#define ATH11K_REG_OFFSET_MASK GENMASK(27, 0) > > enum ath11k_hw_rate_cck { > ATH11K_HW_RATE_CCK_LP_11M = 0,
Larry Finger <Larry.Finger@lwfinger.net> writes: > On 5/1/24 11:14 AM, Ziyang Huang wrote: > >> On 64bit OS, when ab->mem_ce is lower than or 4G far away from ab->mem, >> u32 is not enough to store the offsets, which makes ath11k_ahb_read32() >> and ath11k_ahb_write32() access incorrect address and causes Data Abort >> Exception. >> Let's use the high bits of offsets to decide where to access, which >> is >> similar as ath11k_pci_get_window_start() done. In the future, we can merge >> these functions for unified regs accessing. >> Signed-off-by: Ziyang Huang <hzyitc@outlook.com> >> --- >> drivers/net/wireless/ath/ath11k/ahb.c | 34 ++++++++++++++++++++------- >> drivers/net/wireless/ath/ath11k/hal.c | 17 +++++--------- >> drivers/net/wireless/ath/ath11k/hw.c | 14 +++++------ >> drivers/net/wireless/ath/ath11k/hw.h | 7 +++++- >> 4 files changed, 45 insertions(+), 27 deletions(-) >> diff --git a/drivers/net/wireless/ath/ath11k/ahb.c >> b/drivers/net/wireless/ath/ath11k/ahb.c >> index 7c0a23517949..9e59b4de93a9 100644 >> --- a/drivers/net/wireless/ath/ath11k/ahb.c >> +++ b/drivers/net/wireless/ath/ath11k/ahb.c >> @@ -198,12 +198,30 @@ static const struct ath11k_pci_ops ath11k_ahb_pci_ops_wcn6750 = { >> static inline u32 ath11k_ahb_read32(struct ath11k_base *ab, u32 >> offset) >> { >> - return ioread32(ab->mem + offset); >> + switch (offset & ATH11K_REG_TYPE_MASK) { >> + case ATH11K_REG_TYPE_NORMAL: >> + return ioread32(ab->mem + FIELD_GET(ATH11K_REG_OFFSET_MASK, offset)); >> + case ATH11K_REG_TYPE_CE: >> + return ioread32(ab->mem_ce + FIELD_GET(ATH11K_REG_OFFSET_MASK, offset)); >> + default: >> + BUG(); > > Do you really want to crash the system here? A dev_warn() or something > similar would log the situation. I suspect this case is never taken, > but a system crash is not a good response if it is. Correct, BUG() is more or less banned from all wireless code.
Jeff Johnson <quic_jjohnson@quicinc.com> writes: >> static inline u32 ath11k_ahb_read32(struct ath11k_base *ab, u32 offset) >> { >> - return ioread32(ab->mem + offset); >> + switch (offset & ATH11K_REG_TYPE_MASK) { >> + case ATH11K_REG_TYPE_NORMAL: >> + return ioread32(ab->mem + FIELD_GET(ATH11K_REG_OFFSET_MASK, offset)); >> + case ATH11K_REG_TYPE_CE: >> + return ioread32(ab->mem_ce + FIELD_GET(ATH11K_REG_OFFSET_MASK, offset)); >> + default: >> + BUG(); > > you can WARN but you can't BUG (and even WARN is being discouraged) Yeah, even WARN() is risky especially in a function like this. It can cause so much log messages so that the wathdog can trigger and reboot the host.
> On 5/1/2024 9:14 AM, Ziyang Huang wrote: >> On 64bit OS, when ab->mem_ce is lower than or 4G far away from ab->mem, >> u32 is not enough to store the offsets, which makes ath11k_ahb_read32() >> and ath11k_ahb_write32() access incorrect address and causes Data Abort >> Exception. > > Are you actually observing this issue? > Or is this a hypothetical situation? Yes, here is the log: [ 14.896160] ath11k c000000.wifi: ipq5018 hw1.0 [ 14.896210] ath11k c000000.wifi: FW memory mode: 2 [ 14.899576] ath11k c000000.wifi: ath11k_hal_srng_create_config: ab->mem=0xffffffc088000000 [ 14.904290] ath11k c000000.wifi: ath11k_hal_srng_create_config: ab->mem_ce=0xffffffc082800000 [ 14.912593] ath11k c000000.wifi: ath11k_hal_srng_create_config: HAL_SEQ_WCSS_UMAC_CE0_SRC_REG(ab)=0x00000000 [ 14.921247] ath11k c000000.wifi: ath11k_hal_srng_create_config: HAL_CE_DST_RING_BASE_LSB=0x00000000 [ 14.931115] ath11k c000000.wifi: ath11k_hal_srng_create_config: ATH11K_CE_OFFSET(ab)=0xfffffffffa800000 [ 14.939863] ath11k c000000.wifi: ath11k_hal_srng_create_config: s->reg_start[0]=0xfa800000 ... [ 15.155895] ath11k c000000.wifi: chip_id 0x0 chip_family 0x4 board_id 0x10 soc_id 0xffffffff [ 15.155954] ath11k c000000.wifi: fw_version 0x270206d0 fw_build_timestamp 2022-08-04 13:28 fw_build_id WLAN.HK.2.7.0.1-01744-QCAHKSWPL_SILICONZ-1 [ 15.292858] ath11k c000000.wifi: ath11k_hal_srng_src_hw_init: reg_base=0xfa800000 [ 15.292938] ath11k c000000.wifi: ath11k_ahb_write32: ab->mem=0xffffffc088000000 [ 15.299549] ath11k c000000.wifi: ath11k_ahb_write32: offset=0xfa800000 [ 15.306525] ath11k c000000.wifi: ath11k_ahb_write32: addr=0xffffffc182800000 [ 15.313088] Unable to handle kernel paging request at virtual address ffffffc182800000 [ 15.320309] Mem abort info: [ 15.328023] ESR = 0x0000000096000045 [ 15.330691] EC = 0x25: DABT (current EL), IL = 32 bits [ 15.334512] SET = 0, FnV = 0 [ 15.340030] EA = 0, S1PTW = 0 [ 15.342843] FSC = 0x05: level 1 translation fault [ 15.345900] Data abort info: [ 15.350741] ISV = 0, ISS = 0x00000045, ISS2 = 0x00000000 [ 15.353868] CM = 0, WnR = 1, TnD = 0, TagAccess = 0 [ 15.359187] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 15.364286] swapper pgtable: 4k pages, 39-bit VAs, pgdp=0000000041a22000 [ 15.369685] [ffffffc182800000] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000 [ 15.376369] Internal error: Oops: 0000000096000045 [#1] SMP [ 15.384771] Modules linked in: pppoe ppp_async nft_fib_inet nf_flow_table_inet ath11k_pci(O) ath11k_ahb(O) ath11k(O) pppox ppp_generic nft_reject_ipv6 nft_reject_ipv4 nft_reject_inet nft_reject nft_redir nft_quota nft_numgen nft_nat nft_masq nft_log nft_limit nft_hash nft_flow_offload nft_fib_ipv6 nft_fib_ipv4 nft_fib nft_ct nft_chain_nat nf_tables nf_nat nf_flow_table nf_conntrack mac80211(O) cfg80211(O) slhc qrtr_smd qrtr_mhi qrtr qmi_helpers(O) nfnetlink nf_reject_ipv6 nf_reject_ipv4 nf_log_syslog nf_defrag_ipv6 nf_defrag_ipv4 mhi mdio_netlink(O) libcrc32c hwmon crc_ccitt compat(O) sha512_generic sha512_arm64 seqiv sha3_generic jitterentropy_rng drbg michael_mic hmac geniv cmac leds_gpio xhci_plat_hcd xhci_pci xhci_hcd dwc3 dwc3_qcom qca_nss_dp(O) qca_ssdk(O) gpio_button_hotplug(O) ext4 mbcache jbd2 crc32c_generic [ 15.440670] CPU: 1 PID: 127 Comm: kworker/u4:4 Tainted: G O 6.6.28 #0 [ 15.462900] Hardware name: Redmi AX3000 (DT) [ 15.470623] Workqueue: ath11k_qmi_driver_event ath11k_qmi_deinit_service [ath11k] [ 15.474965] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 15.482342] pc : 0xffffffc0796774f8 [ 15.489108] lr : 0xffffffc0796774ec [ 15.492581] sp : ffffffc08256bb60 [ 15.496052] x29: ffffffc08256bb60 x28: ffffff8003ec3200 x27: ffffff8003ec2400 [ 15.499530] x26: ffffff8003ed0000 x25: ffffff8003ec1200 x24: 0000000000000020 [ 15.506647] x23: ffffff8003ec3760 x22: 0000000042c4f000 x21: ffffffc0796b0048 [ 15.513766] x20: ffffff8003ec0000 x19: 00000000fa800000 x18: 00000000000000c7 [ 15.520883] x17: 3030303838306366 x16: 666666666678303d x15: ffffffc081356e20 [ 15.528001] x14: 0000000000000255 x13: 00000000000000c7 x12: 00000000ffffffea [ 15.535119] x11: 00000000ffffefff x10: ffffffc0813aee20 x9 : ffffffc081356dc8 [ 15.542237] x8 : 0000000000017fe8 x7 : c0000000ffffefff x6 : 0000000000057fa8 [ 15.549356] x5 : 0000000000000fff x4 : 0000000000000000 x3 : 0000000000000000 [ 15.556474] x2 : 0000000000000000 x1 : ffffff80036b6c00 x0 : ffffffc182800000 [ 15.563593] Call trace: [ 15.570707] 0xffffffc0796774f8 [ 15.572963] ath11k_hal_srng_setup+0x5a0/0x89c [ath11k] [ 15.576090] ath11k_ce_get_attr_flags+0xb4/0x270 [ath11k] [ 15.581299] ath11k_ce_init_pipes+0x4c/0x19c [ath11k] [ 15.586854] ath11k_core_qmi_firmware_ready+0x3c/0x580 [ath11k] [ 15.591889] ath11k_qmi_deinit_service+0x126c/0x1d80 [ath11k] [ 15.597618] process_one_work+0x158/0x2a8 [ 15.603519] worker_thread+0x2ac/0x48c [ 15.607512] kthread+0xdc/0xe0 [ 15.611156] ret_from_fork+0x10/0x20 [ 15.614203] Code: 940a315e f94e2680 8b130000 d50332bf (b9000016) [ 15.617933] ---[ end trace 0000000000000000 ]--- [ 15.623922] Kernel panic - not syncing: Oops: Fatal exception [ 15.628610] SMP: stopping secondary CPUs [ 15.834290] Kernel Offset: disabled [ 15.834310] CPU features: 0x0,00000000,10000000,0000400b [ 15.836573] Memory Limit: none [ 15.842128] Rebooting in 1 seconds.. >> Let's use the high bits of offsets to decide where to access, which is >> similar as ath11k_pci_get_window_start() done. In the future, we can merge >> these functions for unified regs accessing. > > Performing unnecessary tests and masking for every ioread/write operation will > potentially impact performance. But this is what __ath11k_pcic_write32(), ath11k_pci_window_write32() and ath11k_pci_get_window_start() doing. Here are the codes: static u32 ath11k_pci_get_window_start(struct ath11k_base *ab, u32 offset) { if (!ab->hw_params.static_window_map) return ATH11K_PCI_WINDOW_START; if ((offset ^ HAL_SEQ_WCSS_UMAC_OFFSET) < ATH11K_PCI_WINDOW_RANGE_MASK) /* if offset lies within DP register range, use 3rd window */ return 3 * ATH11K_PCI_WINDOW_START; else if ((offset ^ HAL_SEQ_WCSS_UMAC_CE0_SRC_REG(ab)) < ATH11K_PCI_WINDOW_RANGE_MASK) /* if offset lies within CE register range, use 2nd window */ return 2 * ATH11K_PCI_WINDOW_START; else return ATH11K_PCI_WINDOW_START; } > What other fixes were considered (i.e. did you consider making all the > register addresses u64?) I also want to simplify the PCI register accesses. For dynamic window, ATH11K_PCI_WINDOW_VALUE_MASK is used to decide window. But for static window, we need to use these separated helper functions: ath11k_pci_get_window_start() and ath11k_ahb_get_window_start_wcn6750(). What they do are: 1. decide window index, which can be store in middle bits of offset, just as ATH11K_PCI_WINDOW_VALUE_MASK doing. 2. for dynamic window, ath11k_pci_select_window() is need to switch window. for static window, just directly write/read (index * ATH11K_PCI_WINDOW_START). This is what commit 867f4eeee862 ("wifi: ath11k: Fix register write failure on QCN9074") talk about. But ab->hw_params.static_window_map can be used to identify them. So it doesn't matter. With all of these, we don't need any other remap function and be able to store all remap informations in {ipq,qca,qcn}xxx_regs. > ... > > + default: > > + BUG(); > > you can WARN but you can't BUG (and even WARN is being discouraged) Ok. > ... > > + default: > > + BUG(); > > ditto Ok. > ...
Hi Jeff, On 01.05.2024 19:56, Jeff Johnson wrote: > On 5/1/2024 9:14 AM, Ziyang Huang wrote: >> On 64bit OS, when ab->mem_ce is lower than or 4G far away from ab->mem, >> u32 is not enough to store the offsets, which makes ath11k_ahb_read32() >> and ath11k_ahb_write32() access incorrect address and causes Data Abort >> Exception. > > Are you actually observing this issue? > Or is this a hypothetical situation? Yep. This is the real issue. I faced it on IPQ5018 with 64bits kernel. >> Let's use the high bits of offsets to decide where to access, which is >> similar as ath11k_pci_get_window_start() done. In the future, we can merge >> these functions for unified regs accessing. > > Performing unnecessary tests and masking for every ioread/write operation will > potentially impact performance. > > What other fixes were considered (i.e. did you consider making all the > register addresses u64?) Probably, making argument u64 could also be too much. I/O address space of this chip fits 4GB so u32 should be enough. I have a bit different fix for this bug. It introduces an indirect call for the CE registers access and a dedicated set of accessing functions for chips that has the CE region outside the main I/O area. I am going to publish it in a couple of weeks, when I will come from a trip. The patch still needs some polishing. -- Sergey
diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c index 7c0a23517949..9e59b4de93a9 100644 --- a/drivers/net/wireless/ath/ath11k/ahb.c +++ b/drivers/net/wireless/ath/ath11k/ahb.c @@ -198,12 +198,30 @@ static const struct ath11k_pci_ops ath11k_ahb_pci_ops_wcn6750 = { static inline u32 ath11k_ahb_read32(struct ath11k_base *ab, u32 offset) { - return ioread32(ab->mem + offset); + switch (offset & ATH11K_REG_TYPE_MASK) { + case ATH11K_REG_TYPE_NORMAL: + return ioread32(ab->mem + FIELD_GET(ATH11K_REG_OFFSET_MASK, offset)); + case ATH11K_REG_TYPE_CE: + return ioread32(ab->mem_ce + FIELD_GET(ATH11K_REG_OFFSET_MASK, offset)); + default: + BUG(); + return 0; + } } static inline void ath11k_ahb_write32(struct ath11k_base *ab, u32 offset, u32 value) { - iowrite32(value, ab->mem + offset); + switch (offset & ATH11K_REG_TYPE_MASK) { + case ATH11K_REG_TYPE_NORMAL: + iowrite32(value, ab->mem + FIELD_GET(ATH11K_REG_OFFSET_MASK, offset)); + break; + case ATH11K_REG_TYPE_CE: + iowrite32(value, ab->mem_ce + FIELD_GET(ATH11K_REG_OFFSET_MASK, offset)); + break; + default: + BUG(); + break; + } } static void ath11k_ahb_kill_tasklets(struct ath11k_base *ab) @@ -275,9 +293,9 @@ static void ath11k_ahb_ce_irq_enable(struct ath11k_base *ab, u16 ce_id) const struct ce_ie_addr *ce_ie_addr = ab->hw_params.ce_ie_addr; u32 ie1_reg_addr, ie2_reg_addr, ie3_reg_addr; - ie1_reg_addr = ce_ie_addr->ie1_reg_addr + ATH11K_CE_OFFSET(ab); - ie2_reg_addr = ce_ie_addr->ie2_reg_addr + ATH11K_CE_OFFSET(ab); - ie3_reg_addr = ce_ie_addr->ie3_reg_addr + ATH11K_CE_OFFSET(ab); + ie1_reg_addr = ce_ie_addr->ie1_reg_addr; + ie2_reg_addr = ce_ie_addr->ie2_reg_addr; + ie3_reg_addr = ce_ie_addr->ie3_reg_addr; ce_attr = &ab->hw_params.host_ce_config[ce_id]; if (ce_attr->src_nentries) @@ -296,9 +314,9 @@ static void ath11k_ahb_ce_irq_disable(struct ath11k_base *ab, u16 ce_id) const struct ce_ie_addr *ce_ie_addr = ab->hw_params.ce_ie_addr; u32 ie1_reg_addr, ie2_reg_addr, ie3_reg_addr; - ie1_reg_addr = ce_ie_addr->ie1_reg_addr + ATH11K_CE_OFFSET(ab); - ie2_reg_addr = ce_ie_addr->ie2_reg_addr + ATH11K_CE_OFFSET(ab); - ie3_reg_addr = ce_ie_addr->ie3_reg_addr + ATH11K_CE_OFFSET(ab); + ie1_reg_addr = ce_ie_addr->ie1_reg_addr; + ie2_reg_addr = ce_ie_addr->ie2_reg_addr; + ie3_reg_addr = ce_ie_addr->ie3_reg_addr; ce_attr = &ab->hw_params.host_ce_config[ce_id]; if (ce_attr->src_nentries) diff --git a/drivers/net/wireless/ath/ath11k/hal.c b/drivers/net/wireless/ath/ath11k/hal.c index f3d04568c221..f9ba2f821108 100644 --- a/drivers/net/wireless/ath/ath11k/hal.c +++ b/drivers/net/wireless/ath/ath11k/hal.c @@ -1233,20 +1233,16 @@ static int ath11k_hal_srng_create_config(struct ath11k_base *ab) s->reg_start[1] = HAL_SEQ_WCSS_UMAC_TCL_REG + HAL_TCL_STATUS_RING_HP; s = &hal->srng_config[HAL_CE_SRC]; - s->reg_start[0] = HAL_SEQ_WCSS_UMAC_CE0_SRC_REG(ab) + HAL_CE_DST_RING_BASE_LSB + - ATH11K_CE_OFFSET(ab); - s->reg_start[1] = HAL_SEQ_WCSS_UMAC_CE0_SRC_REG(ab) + HAL_CE_DST_RING_HP + - ATH11K_CE_OFFSET(ab); + s->reg_start[0] = HAL_SEQ_WCSS_UMAC_CE0_SRC_REG(ab) + HAL_CE_DST_RING_BASE_LSB; + s->reg_start[1] = HAL_SEQ_WCSS_UMAC_CE0_SRC_REG(ab) + HAL_CE_DST_RING_HP; s->reg_size[0] = HAL_SEQ_WCSS_UMAC_CE1_SRC_REG(ab) - HAL_SEQ_WCSS_UMAC_CE0_SRC_REG(ab); s->reg_size[1] = HAL_SEQ_WCSS_UMAC_CE1_SRC_REG(ab) - HAL_SEQ_WCSS_UMAC_CE0_SRC_REG(ab); s = &hal->srng_config[HAL_CE_DST]; - s->reg_start[0] = HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab) + HAL_CE_DST_RING_BASE_LSB + - ATH11K_CE_OFFSET(ab); - s->reg_start[1] = HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab) + HAL_CE_DST_RING_HP + - ATH11K_CE_OFFSET(ab); + s->reg_start[0] = HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab) + HAL_CE_DST_RING_BASE_LSB; + s->reg_start[1] = HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab) + HAL_CE_DST_RING_HP; s->reg_size[0] = HAL_SEQ_WCSS_UMAC_CE1_DST_REG(ab) - HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab); s->reg_size[1] = HAL_SEQ_WCSS_UMAC_CE1_DST_REG(ab) - @@ -1254,9 +1250,8 @@ static int ath11k_hal_srng_create_config(struct ath11k_base *ab) s = &hal->srng_config[HAL_CE_DST_STATUS]; s->reg_start[0] = HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab) + - HAL_CE_DST_STATUS_RING_BASE_LSB + ATH11K_CE_OFFSET(ab); - s->reg_start[1] = HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab) + HAL_CE_DST_STATUS_RING_HP + - ATH11K_CE_OFFSET(ab); + HAL_CE_DST_STATUS_RING_BASE_LSB; + s->reg_start[1] = HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab) + HAL_CE_DST_STATUS_RING_HP; s->reg_size[0] = HAL_SEQ_WCSS_UMAC_CE1_DST_REG(ab) - HAL_SEQ_WCSS_UMAC_CE0_DST_REG(ab); s->reg_size[1] = HAL_SEQ_WCSS_UMAC_CE1_DST_REG(ab) - diff --git a/drivers/net/wireless/ath/ath11k/hw.c b/drivers/net/wireless/ath/ath11k/hw.c index caa6dc12a790..58f39a7eaa1c 100644 --- a/drivers/net/wireless/ath/ath11k/hw.c +++ b/drivers/net/wireless/ath/ath11k/hw.c @@ -2268,9 +2268,9 @@ const struct ce_ie_addr ath11k_ce_ie_addr_ipq8074 = { }; const struct ce_ie_addr ath11k_ce_ie_addr_ipq5018 = { - .ie1_reg_addr = CE_HOST_IPQ5018_IE_ADDRESS - HAL_IPQ5018_CE_WFSS_REG_BASE, - .ie2_reg_addr = CE_HOST_IPQ5018_IE_2_ADDRESS - HAL_IPQ5018_CE_WFSS_REG_BASE, - .ie3_reg_addr = CE_HOST_IPQ5018_IE_3_ADDRESS - HAL_IPQ5018_CE_WFSS_REG_BASE, + .ie1_reg_addr = ATH11K_REG_TYPE_CE + CE_HOST_IPQ5018_IE_ADDRESS - HAL_IPQ5018_CE_WFSS_REG_BASE, + .ie2_reg_addr = ATH11K_REG_TYPE_CE + CE_HOST_IPQ5018_IE_2_ADDRESS - HAL_IPQ5018_CE_WFSS_REG_BASE, + .ie3_reg_addr = ATH11K_REG_TYPE_CE + CE_HOST_IPQ5018_IE_3_ADDRESS - HAL_IPQ5018_CE_WFSS_REG_BASE, }; const struct ce_remap ath11k_ce_remap_ipq5018 = { @@ -2801,13 +2801,13 @@ const struct ath11k_hw_regs ipq5018_regs = { .hal_reo_status_hp = 0x00003070, /* WCSS relative address */ - .hal_seq_wcss_umac_ce0_src_reg = 0x08400000 + .hal_seq_wcss_umac_ce0_src_reg = ATH11K_REG_TYPE_CE + 0x08400000 - HAL_IPQ5018_CE_WFSS_REG_BASE, - .hal_seq_wcss_umac_ce0_dst_reg = 0x08401000 + .hal_seq_wcss_umac_ce0_dst_reg = ATH11K_REG_TYPE_CE + 0x08401000 - HAL_IPQ5018_CE_WFSS_REG_BASE, - .hal_seq_wcss_umac_ce1_src_reg = 0x08402000 + .hal_seq_wcss_umac_ce1_src_reg = ATH11K_REG_TYPE_CE + 0x08402000 - HAL_IPQ5018_CE_WFSS_REG_BASE, - .hal_seq_wcss_umac_ce1_dst_reg = 0x08403000 + .hal_seq_wcss_umac_ce1_dst_reg = ATH11K_REG_TYPE_CE + 0x08403000 - HAL_IPQ5018_CE_WFSS_REG_BASE, /* WBM Idle address */ diff --git a/drivers/net/wireless/ath/ath11k/hw.h b/drivers/net/wireless/ath/ath11k/hw.h index 14ef4eb48f80..44593b38fc85 100644 --- a/drivers/net/wireless/ath/ath11k/hw.h +++ b/drivers/net/wireless/ath/ath11k/hw.h @@ -81,7 +81,12 @@ #define ATH11K_M3_FILE "m3.bin" #define ATH11K_REGDB_FILE_NAME "regdb.bin" -#define ATH11K_CE_OFFSET(ab) (ab->mem_ce - ab->mem) +#define ATH11K_REG_TYPE_MASK GENMASK(31, 28) +#define ATH11K_REG_TYPE(x) FIELD_PREP_CONST(ATH11K_REG_TYPE_MASK, x) +#define ATH11K_REG_TYPE_NORMAL ATH11K_REG_TYPE(0) +#define ATH11K_REG_TYPE_DP ATH11K_REG_TYPE(1) +#define ATH11K_REG_TYPE_CE ATH11K_REG_TYPE(2) +#define ATH11K_REG_OFFSET_MASK GENMASK(27, 0) enum ath11k_hw_rate_cck { ATH11K_HW_RATE_CCK_LP_11M = 0,
On 64bit OS, when ab->mem_ce is lower than or 4G far away from ab->mem, u32 is not enough to store the offsets, which makes ath11k_ahb_read32() and ath11k_ahb_write32() access incorrect address and causes Data Abort Exception. Let's use the high bits of offsets to decide where to access, which is similar as ath11k_pci_get_window_start() done. In the future, we can merge these functions for unified regs accessing. Signed-off-by: Ziyang Huang <hzyitc@outlook.com> --- drivers/net/wireless/ath/ath11k/ahb.c | 34 ++++++++++++++++++++------- drivers/net/wireless/ath/ath11k/hal.c | 17 +++++--------- drivers/net/wireless/ath/ath11k/hw.c | 14 +++++------ drivers/net/wireless/ath/ath11k/hw.h | 7 +++++- 4 files changed, 45 insertions(+), 27 deletions(-)