Reading through the commit history, it looks like
there is no special need why we must skip the first 4 bytes
in this trace call:
trace_ath10k_htt_rx_desc(ar, (void*)rx_desc + sizeof(u32),
hw->rx_desc_ops->rx_desc_size - sizeof(u32));
found in the function ath10k_htt_rx_amsdu_pop in the file htt_rx.c
i think the original author
(who is also the one who added rx_desc tracing capabilities
in a0883cf7e75a) just wanted to trace the rx_desc contents,
ignoring the fw_rx_desc_base info field
(which is the part being skipped over).
But the trace_ath10k_htt_rx_desc later added
don't care about skipping it, so it may be good
to uniform this call to the others in the file.
But this would change the output of the trace and
thus it may be a problem for tools that rely on it.
Therefore I propose until further discussion
to just keep it as it is and just fix the pointer arithmetic bug.
Add missing void* cast to rx descriptor pointer in order to
properly skip the initial 4 bytes of the rx descriptor
when passing it to trace_ath10k_htt_rx_desc trace function.
This fixes the pointer arithmetic error detected
by Dan Carpenter's static analysis tool.
Fixes: 6bae9de622d3 ("ath10k: abstract htt_rx_desc structure")
Tested-on: QCA6174 hw3.2 PCI WLAN.RM.4.4.1-00157-QCARMSWPZ-1
Signed-off-by: Francesco Magliocca <[email protected]>
Link: https://lore.kernel.org/ath10k/20220201130900.GD22458@kili/
---
drivers/net/wireless/ath/ath10k/htt_rx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 9ad64ca84beb..e01efcd2ce06 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -429,7 +429,7 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
RX_MSDU_END_INFO0_LAST_MSDU;
/* FIXME: why are we skipping the first part of the rx_desc? */
- trace_ath10k_htt_rx_desc(ar, rx_desc + sizeof(u32),
+ trace_ath10k_htt_rx_desc(ar, (void*)rx_desc + sizeof(u32),
hw->rx_desc_ops->rx_desc_size - sizeof(u32));
if (last_msdu)
--
2.35.1
On 2/21/2022 4:26 AM, Francesco Magliocca wrote:
> Reading through the commit history, it looks like
> there is no special need why we must skip the first 4 bytes
> in this trace call:
>
> trace_ath10k_htt_rx_desc(ar, (void*)rx_desc + sizeof(u32),
> hw->rx_desc_ops->rx_desc_size - sizeof(u32));
>
> found in the function ath10k_htt_rx_amsdu_pop in the file htt_rx.c
>
> i think the original author
> (who is also the one who added rx_desc tracing capabilities
> in a0883cf7e75a) just wanted to trace the rx_desc contents,
> ignoring the fw_rx_desc_base info field
> (which is the part being skipped over).
> But the trace_ath10k_htt_rx_desc later added
> don't care about skipping it, so it may be good
> to uniform this call to the others in the file.
> But this would change the output of the trace and
> thus it may be a problem for tools that rely on it.
> Therefore I propose until further discussion
> to just keep it as it is and just fix the pointer arithmetic bug.
>
> Add missing void* cast to rx descriptor pointer in order to
> properly skip the initial 4 bytes of the rx descriptor
> when passing it to trace_ath10k_htt_rx_desc trace function.
>
> This fixes the pointer arithmetic error detected
> by Dan Carpenter's static analysis tool.
>
> Fixes: 6bae9de622d3 ("ath10k: abstract htt_rx_desc structure")
>
> Tested-on: QCA6174 hw3.2 PCI WLAN.RM.4.4.1-00157-QCARMSWPZ-1
>
> Signed-off-by: Francesco Magliocca <[email protected]>
> Link: https://lore.kernel.org/ath10k/20220201130900.GD22458@kili/
> ---
> drivers/net/wireless/ath/ath10k/htt_rx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
> index 9ad64ca84beb..e01efcd2ce06 100644
> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
> @@ -429,7 +429,7 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
> RX_MSDU_END_INFO0_LAST_MSDU;
>
> /* FIXME: why are we skipping the first part of the rx_desc? */
> - trace_ath10k_htt_rx_desc(ar, rx_desc + sizeof(u32),
> + trace_ath10k_htt_rx_desc(ar, (void*)rx_desc + sizeof(u32),
since void pointer arithmetic is undefined in C99 would it be "better"
to cast as u8 *? I realize that gcc has an extension to support this
[1], but this usage will cause a warning when -Wpointer-arith is used.
[1] https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html
Hi, I picked (void*) to be conformant with the other examples in htt_rx.c
For example at line 1431:
> rxd = HTT_RX_BUF_TO_RX_DESC(hw,
> (void *)msdu->data - hw->rx_desc_ops->rx_desc_size);
But for me it is ok. Maybe we should fix all the occurrences of this kind.
Greetings,
FM
Il giorno mar 22 feb 2022 alle ore 21:52 Jeff Johnson
<[email protected]> ha scritto:
>
> On 2/21/2022 4:26 AM, Francesco Magliocca wrote:
> > Reading through the commit history, it looks like
> > there is no special need why we must skip the first 4 bytes
> > in this trace call:
> >
> > trace_ath10k_htt_rx_desc(ar, (void*)rx_desc + sizeof(u32),
> > hw->rx_desc_ops->rx_desc_size - sizeof(u32));
> >
> > found in the function ath10k_htt_rx_amsdu_pop in the file htt_rx.c
> >
> > i think the original author
> > (who is also the one who added rx_desc tracing capabilities
> > in a0883cf7e75a) just wanted to trace the rx_desc contents,
> > ignoring the fw_rx_desc_base info field
> > (which is the part being skipped over).
> > But the trace_ath10k_htt_rx_desc later added
> > don't care about skipping it, so it may be good
> > to uniform this call to the others in the file.
> > But this would change the output of the trace and
> > thus it may be a problem for tools that rely on it.
> > Therefore I propose until further discussion
> > to just keep it as it is and just fix the pointer arithmetic bug.
> >
> > Add missing void* cast to rx descriptor pointer in order to
> > properly skip the initial 4 bytes of the rx descriptor
> > when passing it to trace_ath10k_htt_rx_desc trace function.
> >
> > This fixes the pointer arithmetic error detected
> > by Dan Carpenter's static analysis tool.
> >
> > Fixes: 6bae9de622d3 ("ath10k: abstract htt_rx_desc structure")
> >
> > Tested-on: QCA6174 hw3.2 PCI WLAN.RM.4.4.1-00157-QCARMSWPZ-1
> >
> > Signed-off-by: Francesco Magliocca <[email protected]>
> > Link: https://lore.kernel.org/ath10k/20220201130900.GD22458@kili/
> > ---
> > drivers/net/wireless/ath/ath10k/htt_rx.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
> > index 9ad64ca84beb..e01efcd2ce06 100644
> > --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
> > +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
> > @@ -429,7 +429,7 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
> > RX_MSDU_END_INFO0_LAST_MSDU;
> >
> > /* FIXME: why are we skipping the first part of the rx_desc? */
> > - trace_ath10k_htt_rx_desc(ar, rx_desc + sizeof(u32),
> > + trace_ath10k_htt_rx_desc(ar, (void*)rx_desc + sizeof(u32),
>
> since void pointer arithmetic is undefined in C99 would it be "better"
> to cast as u8 *? I realize that gcc has an extension to support this
> [1], but this usage will cause a warning when -Wpointer-arith is used.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html
On Thu, Feb 24, 2022 at 09:34:31AM +0200, Kalle Valo wrote:
> Francesco Magliocca <[email protected]> writes:
>
> > Hi, I picked (void*) to be conformant with the other examples in htt_rx.c
> > For example at line 1431:
> >> rxd = HTT_RX_BUF_TO_RX_DESC(hw,
> >> (void *)msdu->data - hw->rx_desc_ops->rx_desc_size);
> >
> > But for me it is ok. Maybe we should fix all the occurrences of this kind.
>
> Yeah, it would be good to fix the void pointer arithmetic in a separate
> patch. I have planning to enable -Wpointer-arith in my ath10k-check and
> ath11k-check scripts, so patches are very welcome.
Void * casts simplify a lot of code. Less noise. More readable.
They're more accurate in a sense because it's not a u8 at all. The
kernel can't compile with other compilers besides GCC and Clang so why
care about that the C standard hasn't caught up?
What does -Wpointer-arith buy us?
regards,
dan carpenter
Francesco Magliocca <[email protected]> wrote:
> Reading through the commit history, it looks like
> there is no special need why we must skip the first 4 bytes
> in this trace call:
>
> trace_ath10k_htt_rx_desc(ar, (void*)rx_desc + sizeof(u32),
> hw->rx_desc_ops->rx_desc_size - sizeof(u32));
>
> found in the function ath10k_htt_rx_amsdu_pop in the file htt_rx.c
>
> i think the original author
> (who is also the one who added rx_desc tracing capabilities
> in a0883cf7e75a) just wanted to trace the rx_desc contents,
> ignoring the fw_rx_desc_base info field
> (which is the part being skipped over).
> But the trace_ath10k_htt_rx_desc later added
> don't care about skipping it, so it may be good
> to uniform this call to the others in the file.
> But this would change the output of the trace and
> thus it may be a problem for tools that rely on it.
> Therefore I propose until further discussion
> to just keep it as it is and just fix the pointer arithmetic bug.
>
> Add missing void* cast to rx descriptor pointer in order to
> properly skip the initial 4 bytes of the rx descriptor
> when passing it to trace_ath10k_htt_rx_desc trace function.
>
> This fixes the pointer arithmetic error detected
> by Dan Carpenter's static analysis tool.
>
> Fixes: 6bae9de622d3 ("ath10k: abstract htt_rx_desc structure")
>
> Tested-on: QCA6174 hw3.2 PCI WLAN.RM.4.4.1-00157-QCARMSWPZ-1
>
> Signed-off-by: Francesco Magliocca <[email protected]>
> Link: https://lore.kernel.org/ath10k/20220201130900.GD22458@kili/
> Signed-off-by: Kalle Valo <[email protected]>
Patch applied to ath-next branch of ath.git, thanks.
49ffac5907a8 ath10k: fix pointer arithmetic error in trace call
--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
(Changing subject, adding Linus and linux-kernel)
Dan Carpenter <[email protected]> writes:
> On Thu, Feb 24, 2022 at 09:34:31AM +0200, Kalle Valo wrote:
>> Francesco Magliocca <[email protected]> writes:
>>
>> > Hi, I picked (void*) to be conformant with the other examples in htt_rx.c
>> > For example at line 1431:
>> >> rxd = HTT_RX_BUF_TO_RX_DESC(hw,
>> >> (void *)msdu->data - hw->rx_desc_ops->rx_desc_size);
>> >
>> > But for me it is ok. Maybe we should fix all the occurrences of this kind.
>>
>> Yeah, it would be good to fix the void pointer arithmetic in a separate
>> patch. I have planning to enable -Wpointer-arith in my ath10k-check and
>> ath11k-check scripts, so patches are very welcome.
>
> Void * casts simplify a lot of code. Less noise. More readable.
> They're more accurate in a sense because it's not a u8 at all. The
> kernel can't compile with other compilers besides GCC and Clang so why
> care about that the C standard hasn't caught up?
>
> What does -Wpointer-arith buy us?
A good question. I have always just thought we should avoid void pointer
arithmetic due to the C standard, but now that you mention it void
pointers can indeed simplify the code. So I'm not so sure anymore.
Any opinions? Is there a kernel wide recommendation for this?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Thu, 2022-02-24 at 11:59 +0200, Kalle Valo wrote:
>
> A good question. I have always just thought we should avoid void pointer
> arithmetic due to the C standard, but now that you mention it void
> pointers can indeed simplify the code. So I'm not so sure anymore.
>
> Any opinions? Is there a kernel wide recommendation for this?
The kernel only enables it with W=3, which I guess nobody uses anyway
... Originally it came from commit 4a5838ad9d2d ("kbuild: Add extra gcc
checks") with a pointer to
http://marc.info/?l=kernel-janitors&m=129802065918147&w=2
(which is offline right now due to an expired certificate ...)
but setting back my clock it seems to point to
https://lore.kernel.org/all/20110218091716.GA4384@bicker/
but the thread kind of revolves around -Wconversion.
FreeBSD does enable -Wpointer-arith which is why we've been trying to
keep iwlwifi clean as a courtesy to them, but for really Linux-only code
I dunno if there's much point. Although of course that applies also to
FreeBSD ...
johannes
Francesco Magliocca <[email protected]> writes:
> Hi, I picked (void*) to be conformant with the other examples in htt_rx.c
> For example at line 1431:
>> rxd = HTT_RX_BUF_TO_RX_DESC(hw,
>> (void *)msdu->data - hw->rx_desc_ops->rx_desc_size);
>
> But for me it is ok. Maybe we should fix all the occurrences of this kind.
Yeah, it would be good to fix the void pointer arithmetic in a separate
patch. I have planning to enable -Wpointer-arith in my ath10k-check and
ath11k-check scripts, so patches are very welcome.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches