drivers/net/wireless/ath/ath12k/hal.c | 35 +++++++++++++++------------ drivers/net/wireless/ath/ath12k/hal.h | 8 +++--- 2 files changed, 24 insertions(+), 19 deletions(-)
The SRNG head and tail ring pointers are stored in device memory as
little-endian values. On big-endian systems, direct dereferencing of these
pointers leads to incorrect values being read or written, causing ring
management issues and potentially breaking data flow.
This patch ensures all accesses to SRNG ring pointers use the appropriate
endianness conversions. This affects both read and write paths for source
and destination rings, as well as debug output. The changes guarantee
correct operation on both little- and big-endian architectures.
Signed-off-by: Alexander Wilhelm <alexander.wilhelm@westermo.com>
---
Changes in v2:
- Set '__le32 *' type for 'hp_addr/tp_addr' in both 'dst_ring' and 'src_ring'
---
drivers/net/wireless/ath/ath12k/hal.c | 35 +++++++++++++++------------
drivers/net/wireless/ath/ath12k/hal.h | 8 +++---
2 files changed, 24 insertions(+), 19 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/hal.c b/drivers/net/wireless/ath/ath12k/hal.c
index 6406fcf5d69f..bd4d1de9eb1a 100644
--- a/drivers/net/wireless/ath/ath12k/hal.c
+++ b/drivers/net/wireless/ath/ath12k/hal.c
@@ -2007,7 +2007,7 @@ int ath12k_hal_srng_dst_num_free(struct ath12k_base *ab, struct hal_srng *srng,
tp = srng->u.dst_ring.tp;
if (sync_hw_ptr) {
- hp = *srng->u.dst_ring.hp_addr;
+ hp = le32_to_cpu(*srng->u.dst_ring.hp_addr);
srng->u.dst_ring.cached_hp = hp;
} else {
hp = srng->u.dst_ring.cached_hp;
@@ -2030,7 +2030,7 @@ int ath12k_hal_srng_src_num_free(struct ath12k_base *ab, struct hal_srng *srng,
hp = srng->u.src_ring.hp;
if (sync_hw_ptr) {
- tp = *srng->u.src_ring.tp_addr;
+ tp = le32_to_cpu(*srng->u.src_ring.tp_addr);
srng->u.src_ring.cached_tp = tp;
} else {
tp = srng->u.src_ring.cached_tp;
@@ -2149,9 +2149,9 @@ void ath12k_hal_srng_access_begin(struct ath12k_base *ab, struct hal_srng *srng)
if (srng->ring_dir == HAL_SRNG_DIR_SRC) {
srng->u.src_ring.cached_tp =
- *(volatile u32 *)srng->u.src_ring.tp_addr;
+ le32_to_cpu(*(volatile u32 *)srng->u.src_ring.tp_addr);
} else {
- hp = READ_ONCE(*srng->u.dst_ring.hp_addr);
+ hp = le32_to_cpu(READ_ONCE(*srng->u.dst_ring.hp_addr));
if (hp != srng->u.dst_ring.cached_hp) {
srng->u.dst_ring.cached_hp = hp;
@@ -2175,25 +2175,28 @@ void ath12k_hal_srng_access_end(struct ath12k_base *ab, struct hal_srng *srng)
* hence written to a shared memory location that is read by FW
*/
if (srng->ring_dir == HAL_SRNG_DIR_SRC) {
- srng->u.src_ring.last_tp =
- *(volatile u32 *)srng->u.src_ring.tp_addr;
+ srng->u.src_ring.last_tp = le32_to_cpu(
+ *(volatile u32 *)srng->u.src_ring.tp_addr);
/* Make sure descriptor is written before updating the
* head pointer.
*/
dma_wmb();
- WRITE_ONCE(*srng->u.src_ring.hp_addr, srng->u.src_ring.hp);
+ WRITE_ONCE(*srng->u.src_ring.hp_addr,
+ cpu_to_le32(srng->u.src_ring.hp));
} else {
- srng->u.dst_ring.last_hp = *srng->u.dst_ring.hp_addr;
+ srng->u.dst_ring.last_hp =
+ le32_to_cpu(*srng->u.dst_ring.hp_addr);
/* Make sure descriptor is read before updating the
* tail pointer.
*/
dma_mb();
- WRITE_ONCE(*srng->u.dst_ring.tp_addr, srng->u.dst_ring.tp);
+ WRITE_ONCE(*srng->u.dst_ring.tp_addr,
+ cpu_to_le32(srng->u.dst_ring.tp));
}
} else {
if (srng->ring_dir == HAL_SRNG_DIR_SRC) {
- srng->u.src_ring.last_tp =
- *(volatile u32 *)srng->u.src_ring.tp_addr;
+ srng->u.src_ring.last_tp = le32_to_cpu(
+ *(volatile u32 *)srng->u.src_ring.tp_addr);
/* Assume implementation use an MMIO write accessor
* which has the required wmb() so that the descriptor
* is written before the updating the head pointer.
@@ -2203,7 +2206,8 @@ void ath12k_hal_srng_access_end(struct ath12k_base *ab, struct hal_srng *srng)
(unsigned long)ab->mem,
srng->u.src_ring.hp);
} else {
- srng->u.dst_ring.last_hp = *srng->u.dst_ring.hp_addr;
+ srng->u.dst_ring.last_hp =
+ le32_to_cpu(*srng->u.dst_ring.hp_addr);
/* Make sure descriptor is read before updating the
* tail pointer.
*/
@@ -2547,7 +2551,7 @@ void ath12k_hal_srng_shadow_update_hp_tp(struct ath12k_base *ab,
* HP only when then ring isn't' empty.
*/
if (srng->ring_dir == HAL_SRNG_DIR_SRC &&
- *srng->u.src_ring.tp_addr != srng->u.src_ring.hp)
+ le32_to_cpu(*srng->u.src_ring.tp_addr) != srng->u.src_ring.hp)
ath12k_hal_srng_access_end(ab, srng);
}
@@ -2648,14 +2652,15 @@ void ath12k_hal_dump_srng_stats(struct ath12k_base *ab)
"src srng id %u hp %u, reap_hp %u, cur tp %u, cached tp %u last tp %u napi processed before %ums\n",
srng->ring_id, srng->u.src_ring.hp,
srng->u.src_ring.reap_hp,
- *srng->u.src_ring.tp_addr, srng->u.src_ring.cached_tp,
+ __le32_to_cpu(*srng->u.src_ring.tp_addr),
+ srng->u.src_ring.cached_tp,
srng->u.src_ring.last_tp,
jiffies_to_msecs(jiffies - srng->timestamp));
else if (srng->ring_dir == HAL_SRNG_DIR_DST)
ath12k_err(ab,
"dst srng id %u tp %u, cur hp %u, cached hp %u last hp %u napi processed before %ums\n",
srng->ring_id, srng->u.dst_ring.tp,
- *srng->u.dst_ring.hp_addr,
+ __le32_to_cpu(*srng->u.dst_ring.hp_addr),
srng->u.dst_ring.cached_hp,
srng->u.dst_ring.last_hp,
jiffies_to_msecs(jiffies - srng->timestamp));
diff --git a/drivers/net/wireless/ath/ath12k/hal.h b/drivers/net/wireless/ath/ath12k/hal.h
index efe00e167998..63657d727808 100644
--- a/drivers/net/wireless/ath/ath12k/hal.h
+++ b/drivers/net/wireless/ath/ath12k/hal.h
@@ -709,7 +709,7 @@ struct hal_srng {
u32 tp;
/* Shadow head pointer location to be updated by HW */
- volatile u32 *hp_addr;
+ volatile __le32 *hp_addr;
/* Cached head pointer */
u32 cached_hp;
@@ -718,7 +718,7 @@ struct hal_srng {
* will be a register address and need not be
* accessed through SW structure
*/
- u32 *tp_addr;
+ __le32 *tp_addr;
/* Current SW loop cnt */
u32 loop_cnt;
@@ -738,7 +738,7 @@ struct hal_srng {
u32 reap_hp;
/* Shadow tail pointer location to be updated by HW */
- u32 *tp_addr;
+ __le32 *tp_addr;
/* Cached tail pointer */
u32 cached_tp;
@@ -747,7 +747,7 @@ struct hal_srng {
* will be a register address and need not be accessed
* through SW structure
*/
- u32 *hp_addr;
+ __le32 *hp_addr;
/* Low threshold - in number of ring entries */
u32 low_threshold;
base-commit: be83ff7549057d184b693a85cafc10fbd520f3d7
--
2.43.0
On 11/18/2025 6:17 PM, Alexander Wilhelm wrote:
> The SRNG head and tail ring pointers are stored in device memory as
> little-endian values. On big-endian systems, direct dereferencing of these
> pointers leads to incorrect values being read or written, causing ring
> management issues and potentially breaking data flow.
>
> This patch ensures all accesses to SRNG ring pointers use the appropriate
> endianness conversions. This affects both read and write paths for source
> and destination rings, as well as debug output. The changes guarantee
> correct operation on both little- and big-endian architectures.
>
> Signed-off-by: Alexander Wilhelm <alexander.wilhelm@westermo.com>
> ---
> Changes in v2:
> - Set '__le32 *' type for 'hp_addr/tp_addr' in both 'dst_ring' and 'src_ring'
> ---
> drivers/net/wireless/ath/ath12k/hal.c | 35 +++++++++++++++------------
> drivers/net/wireless/ath/ath12k/hal.h | 8 +++---
> 2 files changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath12k/hal.c b/drivers/net/wireless/ath/ath12k/hal.c
> index 6406fcf5d69f..bd4d1de9eb1a 100644
> --- a/drivers/net/wireless/ath/ath12k/hal.c
> +++ b/drivers/net/wireless/ath/ath12k/hal.c
> @@ -2007,7 +2007,7 @@ int ath12k_hal_srng_dst_num_free(struct ath12k_base *ab, struct hal_srng *srng,
> tp = srng->u.dst_ring.tp;
>
> if (sync_hw_ptr) {
> - hp = *srng->u.dst_ring.hp_addr;
> + hp = le32_to_cpu(*srng->u.dst_ring.hp_addr);
> srng->u.dst_ring.cached_hp = hp;
> } else {
> hp = srng->u.dst_ring.cached_hp;
> @@ -2030,7 +2030,7 @@ int ath12k_hal_srng_src_num_free(struct ath12k_base *ab, struct hal_srng *srng,
> hp = srng->u.src_ring.hp;
>
> if (sync_hw_ptr) {
> - tp = *srng->u.src_ring.tp_addr;
> + tp = le32_to_cpu(*srng->u.src_ring.tp_addr);
> srng->u.src_ring.cached_tp = tp;
> } else {
> tp = srng->u.src_ring.cached_tp;
> @@ -2149,9 +2149,9 @@ void ath12k_hal_srng_access_begin(struct ath12k_base *ab, struct hal_srng *srng)
>
> if (srng->ring_dir == HAL_SRNG_DIR_SRC) {
> srng->u.src_ring.cached_tp =
> - *(volatile u32 *)srng->u.src_ring.tp_addr;
> + le32_to_cpu(*(volatile u32 *)srng->u.src_ring.tp_addr);
s/volatile u32 */volatile __le32 */ ?
> } else {
> - hp = READ_ONCE(*srng->u.dst_ring.hp_addr);
> + hp = le32_to_cpu(READ_ONCE(*srng->u.dst_ring.hp_addr));
>
> if (hp != srng->u.dst_ring.cached_hp) {
> srng->u.dst_ring.cached_hp = hp;
> @@ -2175,25 +2175,28 @@ void ath12k_hal_srng_access_end(struct ath12k_base *ab, struct hal_srng *srng)
> * hence written to a shared memory location that is read by FW
> */
> if (srng->ring_dir == HAL_SRNG_DIR_SRC) {
> - srng->u.src_ring.last_tp =
> - *(volatile u32 *)srng->u.src_ring.tp_addr;
> + srng->u.src_ring.last_tp = le32_to_cpu(
> + *(volatile u32 *)srng->u.src_ring.tp_addr);
s/volatile u32 */volatile __le32 */ ?
> /* Make sure descriptor is written before updating the
> * head pointer.
> */
> dma_wmb();
> - WRITE_ONCE(*srng->u.src_ring.hp_addr, srng->u.src_ring.hp);
> + WRITE_ONCE(*srng->u.src_ring.hp_addr,
> + cpu_to_le32(srng->u.src_ring.hp));
> } else {
> - srng->u.dst_ring.last_hp = *srng->u.dst_ring.hp_addr;
> + srng->u.dst_ring.last_hp =
> + le32_to_cpu(*srng->u.dst_ring.hp_addr);
> /* Make sure descriptor is read before updating the
> * tail pointer.
> */
> dma_mb();
> - WRITE_ONCE(*srng->u.dst_ring.tp_addr, srng->u.dst_ring.tp);
> + WRITE_ONCE(*srng->u.dst_ring.tp_addr,
> + cpu_to_le32(srng->u.dst_ring.tp));
> }
> } else {
> if (srng->ring_dir == HAL_SRNG_DIR_SRC) {
> - srng->u.src_ring.last_tp =
> - *(volatile u32 *)srng->u.src_ring.tp_addr;
> + srng->u.src_ring.last_tp = le32_to_cpu(
> + *(volatile u32 *)srng->u.src_ring.tp_addr);
s/volatile u32 */volatile __le32 */ ?
> /* Assume implementation use an MMIO write accessor
> * which has the required wmb() so that the descriptor
> * is written before the updating the head pointer.
> @@ -2203,7 +2206,8 @@ void ath12k_hal_srng_access_end(struct ath12k_base *ab, struct hal_srng *srng)
> (unsigned long)ab->mem,
> srng->u.src_ring.hp);
> } else {
> - srng->u.dst_ring.last_hp = *srng->u.dst_ring.hp_addr;
> + srng->u.dst_ring.last_hp =
> + le32_to_cpu(*srng->u.dst_ring.hp_addr);
> /* Make sure descriptor is read before updating the
> * tail pointer.
> */
> @@ -2547,7 +2551,7 @@ void ath12k_hal_srng_shadow_update_hp_tp(struct ath12k_base *ab,
> * HP only when then ring isn't' empty.
> */
> if (srng->ring_dir == HAL_SRNG_DIR_SRC &&
> - *srng->u.src_ring.tp_addr != srng->u.src_ring.hp)
> + le32_to_cpu(*srng->u.src_ring.tp_addr) != srng->u.src_ring.hp)
> ath12k_hal_srng_access_end(ab, srng);
> }
>
> @@ -2648,14 +2652,15 @@ void ath12k_hal_dump_srng_stats(struct ath12k_base *ab)
> "src srng id %u hp %u, reap_hp %u, cur tp %u, cached tp %u last tp %u napi processed before %ums\n",
> srng->ring_id, srng->u.src_ring.hp,
> srng->u.src_ring.reap_hp,
> - *srng->u.src_ring.tp_addr, srng->u.src_ring.cached_tp,
> + __le32_to_cpu(*srng->u.src_ring.tp_addr),
> + srng->u.src_ring.cached_tp,
> srng->u.src_ring.last_tp,
> jiffies_to_msecs(jiffies - srng->timestamp));
> else if (srng->ring_dir == HAL_SRNG_DIR_DST)
> ath12k_err(ab,
> "dst srng id %u tp %u, cur hp %u, cached hp %u last hp %u napi processed before %ums\n",
> srng->ring_id, srng->u.dst_ring.tp,
> - *srng->u.dst_ring.hp_addr,
> + __le32_to_cpu(*srng->u.dst_ring.hp_addr),
still my v1 comment does not get addressed:
why __le32_to_cpu() only in logging, while le32_to_cpu() elsewhere?
> srng->u.dst_ring.cached_hp,
> srng->u.dst_ring.last_hp,
> jiffies_to_msecs(jiffies - srng->timestamp));
On Tue, Nov 18, 2025 at 06:43:44PM +0800, Baochen Qiang wrote:
>
>
> On 11/18/2025 6:17 PM, Alexander Wilhelm wrote:
> > The SRNG head and tail ring pointers are stored in device memory as
> > little-endian values. On big-endian systems, direct dereferencing of these
> > pointers leads to incorrect values being read or written, causing ring
> > management issues and potentially breaking data flow.
> >
> > This patch ensures all accesses to SRNG ring pointers use the appropriate
> > endianness conversions. This affects both read and write paths for source
> > and destination rings, as well as debug output. The changes guarantee
> > correct operation on both little- and big-endian architectures.
> >
> > Signed-off-by: Alexander Wilhelm <alexander.wilhelm@westermo.com>
> > ---
> > Changes in v2:
> > - Set '__le32 *' type for 'hp_addr/tp_addr' in both 'dst_ring' and 'src_ring'
> > ---
> > drivers/net/wireless/ath/ath12k/hal.c | 35 +++++++++++++++------------
> > drivers/net/wireless/ath/ath12k/hal.h | 8 +++---
> > 2 files changed, 24 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath12k/hal.c b/drivers/net/wireless/ath/ath12k/hal.c
> > index 6406fcf5d69f..bd4d1de9eb1a 100644
> > --- a/drivers/net/wireless/ath/ath12k/hal.c
> > +++ b/drivers/net/wireless/ath/ath12k/hal.c
> > @@ -2007,7 +2007,7 @@ int ath12k_hal_srng_dst_num_free(struct ath12k_base *ab, struct hal_srng *srng,
> > tp = srng->u.dst_ring.tp;
> >
> > if (sync_hw_ptr) {
> > - hp = *srng->u.dst_ring.hp_addr;
> > + hp = le32_to_cpu(*srng->u.dst_ring.hp_addr);
> > srng->u.dst_ring.cached_hp = hp;
> > } else {
> > hp = srng->u.dst_ring.cached_hp;
> > @@ -2030,7 +2030,7 @@ int ath12k_hal_srng_src_num_free(struct ath12k_base *ab, struct hal_srng *srng,
> > hp = srng->u.src_ring.hp;
> >
> > if (sync_hw_ptr) {
> > - tp = *srng->u.src_ring.tp_addr;
> > + tp = le32_to_cpu(*srng->u.src_ring.tp_addr);
> > srng->u.src_ring.cached_tp = tp;
> > } else {
> > tp = srng->u.src_ring.cached_tp;
> > @@ -2149,9 +2149,9 @@ void ath12k_hal_srng_access_begin(struct ath12k_base *ab, struct hal_srng *srng)
> >
> > if (srng->ring_dir == HAL_SRNG_DIR_SRC) {
> > srng->u.src_ring.cached_tp =
> > - *(volatile u32 *)srng->u.src_ring.tp_addr;
> > + le32_to_cpu(*(volatile u32 *)srng->u.src_ring.tp_addr);
>
> s/volatile u32 */volatile __le32 */ ?
I got it. I'll fix it in v3.
> > } else {
> > - hp = READ_ONCE(*srng->u.dst_ring.hp_addr);
> > + hp = le32_to_cpu(READ_ONCE(*srng->u.dst_ring.hp_addr));
> >
> > if (hp != srng->u.dst_ring.cached_hp) {
> > srng->u.dst_ring.cached_hp = hp;
> > @@ -2175,25 +2175,28 @@ void ath12k_hal_srng_access_end(struct ath12k_base *ab, struct hal_srng *srng)
> > * hence written to a shared memory location that is read by FW
> > */
> > if (srng->ring_dir == HAL_SRNG_DIR_SRC) {
> > - srng->u.src_ring.last_tp =
> > - *(volatile u32 *)srng->u.src_ring.tp_addr;
> > + srng->u.src_ring.last_tp = le32_to_cpu(
> > + *(volatile u32 *)srng->u.src_ring.tp_addr);
>
> s/volatile u32 */volatile __le32 */ ?
Same as above, sure!
> > /* Make sure descriptor is written before updating the
> > * head pointer.
> > */
> > dma_wmb();
> > - WRITE_ONCE(*srng->u.src_ring.hp_addr, srng->u.src_ring.hp);
> > + WRITE_ONCE(*srng->u.src_ring.hp_addr,
> > + cpu_to_le32(srng->u.src_ring.hp));
> > } else {
> > - srng->u.dst_ring.last_hp = *srng->u.dst_ring.hp_addr;
> > + srng->u.dst_ring.last_hp =
> > + le32_to_cpu(*srng->u.dst_ring.hp_addr);
> > /* Make sure descriptor is read before updating the
> > * tail pointer.
> > */
> > dma_mb();
> > - WRITE_ONCE(*srng->u.dst_ring.tp_addr, srng->u.dst_ring.tp);
> > + WRITE_ONCE(*srng->u.dst_ring.tp_addr,
> > + cpu_to_le32(srng->u.dst_ring.tp));
> > }
> > } else {
> > if (srng->ring_dir == HAL_SRNG_DIR_SRC) {
> > - srng->u.src_ring.last_tp =
> > - *(volatile u32 *)srng->u.src_ring.tp_addr;
> > + srng->u.src_ring.last_tp = le32_to_cpu(
> > + *(volatile u32 *)srng->u.src_ring.tp_addr);
Same as above, sure!
> > /* Assume implementation use an MMIO write accessor
> > * which has the required wmb() so that the descriptor
> > * is written before the updating the head pointer.
> > @@ -2203,7 +2206,8 @@ void ath12k_hal_srng_access_end(struct ath12k_base *ab, struct hal_srng *srng)
> > (unsigned long)ab->mem,
> > srng->u.src_ring.hp);
> > } else {
> > - srng->u.dst_ring.last_hp = *srng->u.dst_ring.hp_addr;
> > + srng->u.dst_ring.last_hp =
> > + le32_to_cpu(*srng->u.dst_ring.hp_addr);
> > /* Make sure descriptor is read before updating the
> > * tail pointer.
> > */
> > @@ -2547,7 +2551,7 @@ void ath12k_hal_srng_shadow_update_hp_tp(struct ath12k_base *ab,
> > * HP only when then ring isn't' empty.
> > */
> > if (srng->ring_dir == HAL_SRNG_DIR_SRC &&
> > - *srng->u.src_ring.tp_addr != srng->u.src_ring.hp)
> > + le32_to_cpu(*srng->u.src_ring.tp_addr) != srng->u.src_ring.hp)
> > ath12k_hal_srng_access_end(ab, srng);
> > }
> >
> > @@ -2648,14 +2652,15 @@ void ath12k_hal_dump_srng_stats(struct ath12k_base *ab)
> > "src srng id %u hp %u, reap_hp %u, cur tp %u, cached tp %u last tp %u napi processed before %ums\n",
> > srng->ring_id, srng->u.src_ring.hp,
> > srng->u.src_ring.reap_hp,
> > - *srng->u.src_ring.tp_addr, srng->u.src_ring.cached_tp,
> > + __le32_to_cpu(*srng->u.src_ring.tp_addr),
> > + srng->u.src_ring.cached_tp,
> > srng->u.src_ring.last_tp,
> > jiffies_to_msecs(jiffies - srng->timestamp));
> > else if (srng->ring_dir == HAL_SRNG_DIR_DST)
> > ath12k_err(ab,
> > "dst srng id %u tp %u, cur hp %u, cached hp %u last hp %u napi processed before %ums\n",
> > srng->ring_id, srng->u.dst_ring.tp,
> > - *srng->u.dst_ring.hp_addr,
> > + __le32_to_cpu(*srng->u.dst_ring.hp_addr),
>
> still my v1 comment does not get addressed:
>
>
> why __le32_to_cpu() only in logging, while le32_to_cpu() elsewhere?
Sorry, I was confused with the previous issue. Yes, I saw this form on an
upstream patch where '__le32_to_cpu()' was used instead of 'le32_to_cpu()'.
Which one should I prefer? I'll unify that in v3.
Best regards
Alexander Wilhelm
On 11/18/2025 6:49 PM, Alexander Wilhelm wrote:
> On Tue, Nov 18, 2025 at 06:43:44PM +0800, Baochen Qiang wrote:
>>
>>
>> On 11/18/2025 6:17 PM, Alexander Wilhelm wrote:
>>> The SRNG head and tail ring pointers are stored in device memory as
>>> little-endian values. On big-endian systems, direct dereferencing of these
>>> pointers leads to incorrect values being read or written, causing ring
>>> management issues and potentially breaking data flow.
>>>
>>> This patch ensures all accesses to SRNG ring pointers use the appropriate
>>> endianness conversions. This affects both read and write paths for source
>>> and destination rings, as well as debug output. The changes guarantee
>>> correct operation on both little- and big-endian architectures.
>>>
>>> Signed-off-by: Alexander Wilhelm <alexander.wilhelm@westermo.com>
>>> ---
>>> Changes in v2:
>>> - Set '__le32 *' type for 'hp_addr/tp_addr' in both 'dst_ring' and 'src_ring'
>>> ---
>>> drivers/net/wireless/ath/ath12k/hal.c | 35 +++++++++++++++------------
>>> drivers/net/wireless/ath/ath12k/hal.h | 8 +++---
>>> 2 files changed, 24 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath12k/hal.c b/drivers/net/wireless/ath/ath12k/hal.c
>>> index 6406fcf5d69f..bd4d1de9eb1a 100644
>>> --- a/drivers/net/wireless/ath/ath12k/hal.c
>>> +++ b/drivers/net/wireless/ath/ath12k/hal.c
>>> @@ -2007,7 +2007,7 @@ int ath12k_hal_srng_dst_num_free(struct ath12k_base *ab, struct hal_srng *srng,
>>> tp = srng->u.dst_ring.tp;
>>>
>>> if (sync_hw_ptr) {
>>> - hp = *srng->u.dst_ring.hp_addr;
>>> + hp = le32_to_cpu(*srng->u.dst_ring.hp_addr);
>>> srng->u.dst_ring.cached_hp = hp;
>>> } else {
>>> hp = srng->u.dst_ring.cached_hp;
>>> @@ -2030,7 +2030,7 @@ int ath12k_hal_srng_src_num_free(struct ath12k_base *ab, struct hal_srng *srng,
>>> hp = srng->u.src_ring.hp;
>>>
>>> if (sync_hw_ptr) {
>>> - tp = *srng->u.src_ring.tp_addr;
>>> + tp = le32_to_cpu(*srng->u.src_ring.tp_addr);
>>> srng->u.src_ring.cached_tp = tp;
>>> } else {
>>> tp = srng->u.src_ring.cached_tp;
>>> @@ -2149,9 +2149,9 @@ void ath12k_hal_srng_access_begin(struct ath12k_base *ab, struct hal_srng *srng)
>>>
>>> if (srng->ring_dir == HAL_SRNG_DIR_SRC) {
>>> srng->u.src_ring.cached_tp =
>>> - *(volatile u32 *)srng->u.src_ring.tp_addr;
>>> + le32_to_cpu(*(volatile u32 *)srng->u.src_ring.tp_addr);
>>
>> s/volatile u32 */volatile __le32 */ ?
>
> I got it. I'll fix it in v3.
>
>>> } else {
>>> - hp = READ_ONCE(*srng->u.dst_ring.hp_addr);
>>> + hp = le32_to_cpu(READ_ONCE(*srng->u.dst_ring.hp_addr));
>>>
>>> if (hp != srng->u.dst_ring.cached_hp) {
>>> srng->u.dst_ring.cached_hp = hp;
>>> @@ -2175,25 +2175,28 @@ void ath12k_hal_srng_access_end(struct ath12k_base *ab, struct hal_srng *srng)
>>> * hence written to a shared memory location that is read by FW
>>> */
>>> if (srng->ring_dir == HAL_SRNG_DIR_SRC) {
>>> - srng->u.src_ring.last_tp =
>>> - *(volatile u32 *)srng->u.src_ring.tp_addr;
>>> + srng->u.src_ring.last_tp = le32_to_cpu(
>>> + *(volatile u32 *)srng->u.src_ring.tp_addr);
>>
>> s/volatile u32 */volatile __le32 */ ?
>
> Same as above, sure!
>
>>> /* Make sure descriptor is written before updating the
>>> * head pointer.
>>> */
>>> dma_wmb();
>>> - WRITE_ONCE(*srng->u.src_ring.hp_addr, srng->u.src_ring.hp);
>>> + WRITE_ONCE(*srng->u.src_ring.hp_addr,
>>> + cpu_to_le32(srng->u.src_ring.hp));
>>> } else {
>>> - srng->u.dst_ring.last_hp = *srng->u.dst_ring.hp_addr;
>>> + srng->u.dst_ring.last_hp =
>>> + le32_to_cpu(*srng->u.dst_ring.hp_addr);
>>> /* Make sure descriptor is read before updating the
>>> * tail pointer.
>>> */
>>> dma_mb();
>>> - WRITE_ONCE(*srng->u.dst_ring.tp_addr, srng->u.dst_ring.tp);
>>> + WRITE_ONCE(*srng->u.dst_ring.tp_addr,
>>> + cpu_to_le32(srng->u.dst_ring.tp));
>>> }
>>> } else {
>>> if (srng->ring_dir == HAL_SRNG_DIR_SRC) {
>>> - srng->u.src_ring.last_tp =
>>> - *(volatile u32 *)srng->u.src_ring.tp_addr;
>>> + srng->u.src_ring.last_tp = le32_to_cpu(
>>> + *(volatile u32 *)srng->u.src_ring.tp_addr);
>
> Same as above, sure!
>
>>> /* Assume implementation use an MMIO write accessor
>>> * which has the required wmb() so that the descriptor
>>> * is written before the updating the head pointer.
>>> @@ -2203,7 +2206,8 @@ void ath12k_hal_srng_access_end(struct ath12k_base *ab, struct hal_srng *srng)
>>> (unsigned long)ab->mem,
>>> srng->u.src_ring.hp);
>>> } else {
>>> - srng->u.dst_ring.last_hp = *srng->u.dst_ring.hp_addr;
>>> + srng->u.dst_ring.last_hp =
>>> + le32_to_cpu(*srng->u.dst_ring.hp_addr);
>>> /* Make sure descriptor is read before updating the
>>> * tail pointer.
>>> */
>>> @@ -2547,7 +2551,7 @@ void ath12k_hal_srng_shadow_update_hp_tp(struct ath12k_base *ab,
>>> * HP only when then ring isn't' empty.
>>> */
>>> if (srng->ring_dir == HAL_SRNG_DIR_SRC &&
>>> - *srng->u.src_ring.tp_addr != srng->u.src_ring.hp)
>>> + le32_to_cpu(*srng->u.src_ring.tp_addr) != srng->u.src_ring.hp)
>>> ath12k_hal_srng_access_end(ab, srng);
>>> }
>>>
>>> @@ -2648,14 +2652,15 @@ void ath12k_hal_dump_srng_stats(struct ath12k_base *ab)
>>> "src srng id %u hp %u, reap_hp %u, cur tp %u, cached tp %u last tp %u napi processed before %ums\n",
>>> srng->ring_id, srng->u.src_ring.hp,
>>> srng->u.src_ring.reap_hp,
>>> - *srng->u.src_ring.tp_addr, srng->u.src_ring.cached_tp,
>>> + __le32_to_cpu(*srng->u.src_ring.tp_addr),
>>> + srng->u.src_ring.cached_tp,
>>> srng->u.src_ring.last_tp,
>>> jiffies_to_msecs(jiffies - srng->timestamp));
>>> else if (srng->ring_dir == HAL_SRNG_DIR_DST)
>>> ath12k_err(ab,
>>> "dst srng id %u tp %u, cur hp %u, cached hp %u last hp %u napi processed before %ums\n",
>>> srng->ring_id, srng->u.dst_ring.tp,
>>> - *srng->u.dst_ring.hp_addr,
>>> + __le32_to_cpu(*srng->u.dst_ring.hp_addr),
>>
>> still my v1 comment does not get addressed:
>>
>>
>> why __le32_to_cpu() only in logging, while le32_to_cpu() elsewhere?
>
> Sorry, I was confused with the previous issue. Yes, I saw this form on an
> upstream patch where '__le32_to_cpu()' was used instead of 'le32_to_cpu()'.
> Which one should I prefer? I'll unify that in v3.
I am not sure here -- Copilot tells me that le32_to_cpu() does argument type check while
__le32_to_cpu() does not, in that case we should use the latter one because we are sure
the type is safe.
However actual code shows they are actually the same:
include/linux/byteorder/generic.h:
#define le32_to_cpu __le32_to_cpu
Let's see if others can give some insights.
>
>
> Best regards
> Alexander Wilhelm
© 2016 - 2025 Red Hat, Inc.