From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2024-10-29 16:43:32
This is a follow-up of the discussion at:
https://lore.kernel.org/oe-kbuild-all/20241028-sticky-refined-lionfish-b06c0c@leitao/
where I said I would take care of the sparse warnings uncovered by
Breno's COMPILE_TEST change for the dpaa_eth driver.
There was one warning that I decided to treat as an actual bug:
https://lore.kernel.org/netdev/20241029163105.44135-1-vladimir.oltean@nxp.com/
and what remains here are those warnings which I consider harmless.
I would like Christophe to ack the entire series to be taken through
netdev. I find it weird that the qbman driver, whose major API consumer
is netdev, is maintained by a different group. In this case, the buggy
qm_sg_entry_get_off() function is defined in qbman but exclusively
called in netdev.
Vladimir Oltean (3):
soc: fsl_qbman: use be16_to_cpu() in qm_sg_entry_get_off()
net: dpaa_eth: add assertions about SGT entry offsets in
sg_fd_to_skb()
net: dpaa_eth: extract hash using __be32 pointer in rx_default_dqrr()
.../net/ethernet/freescale/dpaa/dpaa_eth.c | 26 ++++++++++++-------
include/soc/fsl/qman.h | 2 +-
2 files changed, 17 insertions(+), 11 deletions(-)
--
2.34.1
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2024-10-29 16:43:35
struct qm_sg_entry :: offset is a 13-bit field, declared as __be16.
When using be32_to_cpu(), a wrong value will be calculated on little
endian systems (Arm), because type promotion from 16-bit to 32-bit,
which is done before the byte swap and always in the CPU native
endianness, changes the value of the scatter/gather list entry offset in
big-endian interpretation (adds two zero bytes in the LSB interpretation).
The result of the byte swap is ANDed with GENMASK(12, 0), so the result
is always zero, because only those bytes added by type promotion remain
after the application of the bit mask.
The impact of the bug is that scatter/gather frames with a non-zero
offset into the buffer are treated by the driver as if they had a zero
offset. This is all in theory, because in practice, qm_sg_entry_get_off()
has a single caller, where the bug is inconsequential, because at that
call site the buffer offset will always be zero, as will be explained in
the subsequent change.
Flagged by sparse:
warning: cast to restricted __be32
warning: cast from restricted __be16
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
include/soc/fsl/qman.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2024-10-29 16:43:37
Multi-buffer frame descriptors (FDs) point to a buffer holding a
scatter/gather table (SGT), which is a finite array of fixed-size
entries, the last of which has qm_sg_entry_is_final(&sgt[i]) == true.
Each SGT entry points to a buffer holding pieces of the frame.
DPAARM.pdf explains in the figure called "Internal and External Margins,
Scatter/Gather Frame Format" that the SGT table is located within its
buffer at the same offset as the frame data start is located within the
first packet buffer.
+------------------------+
Scatter/Gather Buffer | First Buffer | Last Buffer
^ +------------+ ^ +-|---->^ +------------+ +->+------------+
| | | | ICEOF | | | | | |////////////|
| +------------+ v | | | | | |////////////|
BSM | |/ part of //| | |BSM | | | |////////////|
| |/ Internal /| | | | | | |////////////|
| |/ Context //| | | | | | |// Frame ///|
| +------------+ | | | | | ... |/ content //|
| | | | | | | | |////////////|
| | | | | | | | |////////////|
v +------------+ | | v +------------+ |////////////|
| Scatter/ //| sgt[0]--+ | |// Frame ///| |////////////|
| Gather List| ... | |/ content //| +------------+ ^
|////////////| sgt[N]----+ |////////////| | | | BEM
|////////////| |////////////| | | |
+------------+ +------------+ +------------+ v
BSM = Buffer Start Margin, BEM = Buffer End Margin, both are configured
by dpaa_eth_init_rx_port() for the RX FMan port relevant here.
sg_fd_to_skb() runs in the calling context of rx_default_dqrr() -
the NAPI receive callback - which only expects to receive contiguous
(qm_fd_contig) or scatter/gather (qm_fd_sg) frame descriptors.
Everything else is irrelevant codewise.
The processing done by sg_fd_to_skb() is weird because it does not
conform to the expectations laid out by the aforementioned figure.
Namely, it parses the OFFSET field only for SGT entries with i != 0
(codewise, skb != NULL). In those cases, OFFSET should always be 0.
Also, it does not parse the OFFSET field for the sgt[0] case, the only
case where the buffer offset is meaningful in this context. There, it
uses the fd_off, aka the offset to the Scatter/Gather List in the
Scatter/Gather Buffer from the figure. By equivalence, they should both
be equal to the BSM (in turn, equal to priv->rx_headroom).
This can actually be explained due to the bug which we had in
qm_sg_entry_get_off() until the previous change:
- qm_sg_entry_get_off() did not actually _work_ for sgt[0]. It returned
zero even with a non-zero offset, so fd_off had to be used as a fill-in.
- qm_sg_entry_get_off() always returned zero for sgt[i>0], and that
resulted in no user-visible bug, because the buffer offset _was
supposed_ to be zero for those buffers. So remove it from calculations.
Add assertions about the OFFSET field in both cases (first or subsequent
SGT entries) to make it absolutely obvious when something is not well
handled.
Similar logic can be seen in the driver for the architecturally similar
DPAA2, where dpaa2_eth_build_frag_skb() calls dpaa2_sg_get_offset() only
for i == 0. For the rest, there is even a comment stating the same thing:
* Data in subsequent SG entries is stored from the
* beginning of the buffer, so we don't need to add the
* sg_offset.
Tested on LS1046A.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
.../net/ethernet/freescale/dpaa/dpaa_eth.c | 24 ++++++++++++-------
1 file changed, 15 insertions(+), 9 deletions(-)
@@ -1863,6 +1862,11 @@ static struct sk_buff *sg_fd_to_skb(const struct dpaa_priv *priv,*onTx,ifextraheadersareadded.*/WARN_ON(fd_off!=priv->rx_headroom);+/* The offset to data start within the buffer holding+*theSGTshouldalwaysbeequaltotheoffsettodata+*startwithinthefirstbufferholdingtheframe.+*/+WARN_ON_ONCE(fd_off!=qm_sg_entry_get_off(&sgt[i]));skb_reserve(skb,fd_off);skb_put(skb,qm_sg_entry_get_len(&sgt[i]));}else{
@@ -1876,21 +1880,23 @@ static struct sk_buff *sg_fd_to_skb(const struct dpaa_priv *priv,page=virt_to_page(sg_vaddr);head_page=virt_to_head_page(sg_vaddr);-/* Compute offset in (possibly tail) page */+/* Compute offset of sg_vaddr in (possibly tail) page */page_offset=((unsignedlong)sg_vaddr&(PAGE_SIZE-1))+(page_address(page)-page_address(head_page));-/* page_offset only refers to the beginning of sgt[i];-*butthebufferitselfmayhaveaninternaloffset.++/* Non-initial SGT entries should not have a buffer+*offset.*/-frag_off=qm_sg_entry_get_off(&sgt[i])+page_offset;-frag_len=qm_sg_entry_get_len(&sgt[i]);+WARN_ON_ONCE(qm_sg_entry_get_off(&sgt[i]));+/* skb_add_rx_frag() does no checking on the page; if*wepassitatailpage,we'llendupwith-*badpageaccountingandeventuallywithsegafults.+*badpageaccountingandeventuallywithsegfaults.*/-skb_add_rx_frag(skb,i-1,head_page,frag_off,-frag_len,dpaa_bp->size);+skb_add_rx_frag(skb,i-1,head_page,page_offset,+qm_sg_entry_get_len(&sgt[i]),+dpaa_bp->size);}/* Update the pool count for the current {cpu x bpool} */
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2024-10-29 16:43:40
Sparse provides the following output:
warning: cast to restricted __be32
This is a harmless warning due to the fact that we dereference the hash
stored in the FD using an incorrect type annotation. Suppress the
warning by using the correct __be32 type instead of u32. No functional
change.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
On Tue, Oct 29, 2024 at 06:43:17PM +0200, Vladimir Oltean wrote:
Sparse provides the following output:
warning: cast to restricted __be32
This is a harmless warning due to the fact that we dereference the hash
stored in the FD using an incorrect type annotation. Suppress the
warning by using the correct __be32 type instead of u32. No functional
change.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
On Tue, Oct 29, 2024 at 06:43:15PM +0200, Vladimir Oltean wrote:
struct qm_sg_entry :: offset is a 13-bit field, declared as __be16.
When using be32_to_cpu(), a wrong value will be calculated on little
endian systems (Arm), because type promotion from 16-bit to 32-bit,
which is done before the byte swap and always in the CPU native
endianness, changes the value of the scatter/gather list entry offset in
big-endian interpretation (adds two zero bytes in the LSB interpretation).
The result of the byte swap is ANDed with GENMASK(12, 0), so the result
is always zero, because only those bytes added by type promotion remain
after the application of the bit mask.
The impact of the bug is that scatter/gather frames with a non-zero
offset into the buffer are treated by the driver as if they had a zero
offset. This is all in theory, because in practice, qm_sg_entry_get_off()
has a single caller, where the bug is inconsequential, because at that
call site the buffer offset will always be zero, as will be explained in
the subsequent change.
Flagged by sparse:
warning: cast to restricted __be32
warning: cast from restricted __be16
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
-----Original Message-----
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Sent: Tuesday, October 29, 2024 6:43 PM
To: netdev@vger.kernel.org
Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
[off-list ref]; Jakub Kicinski [off-list ref]; Paolo Abeni
[off-list ref]; Breno Leitao [off-list ref]; Madalin Bucur
[off-list ref]; Ioana Ciornei [off-list ref]; Radu-
Andrei Bulie [off-list ref]; Christophe Leroy
[off-list ref]; Sean Anderson [off-list ref];
linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-arm-
kernel@lists.infradead.org
Subject: [PATCH net-next 0/3] Fix sparse warnings in dpaa_eth driver
This is a follow-up of the discussion at:
https://lore.kernel.org/oe-kbuild-all/20241028-sticky-refined-lionfish-
b06c0c@leitao/
where I said I would take care of the sparse warnings uncovered by
Breno's COMPILE_TEST change for the dpaa_eth driver.
There was one warning that I decided to treat as an actual bug:
https://lore.kernel.org/netdev/20241029163105.44135-1-
vladimir.oltean@nxp.com/
and what remains here are those warnings which I consider harmless.
I would like Christophe to ack the entire series to be taken through
netdev. I find it weird that the qbman driver, whose major API consumer
is netdev, is maintained by a different group. In this case, the buggy
qm_sg_entry_get_off() function is defined in qbman but exclusively
called in netdev.
Vladimir Oltean (3):
soc: fsl_qbman: use be16_to_cpu() in qm_sg_entry_get_off()
net: dpaa_eth: add assertions about SGT entry offsets in
sg_fd_to_skb()
net: dpaa_eth: extract hash using __be32 pointer in rx_default_dqrr()
.../net/ethernet/freescale/dpaa/dpaa_eth.c | 26 ++++++++++++-------
include/soc/fsl/qman.h | 2 +-
2 files changed, 17 insertions(+), 11 deletions(-)
--
2.34.1
For the series,
Acked-by: Madalin Bucur <redacted>
Thank you!
Multi-buffer frame descriptors (FDs) point to a buffer holding a
scatter/gather table (SGT), which is a finite array of fixed-size
entries, the last of which has qm_sg_entry_is_final(&sgt[i]) == true.
Each SGT entry points to a buffer holding pieces of the frame.
DPAARM.pdf explains in the figure called "Internal and External Margins,
Scatter/Gather Frame Format" that the SGT table is located within its
buffer at the same offset as the frame data start is located within the
first packet buffer.
+------------------------+
Scatter/Gather Buffer | First Buffer | Last Buffer
^ +------------+ ^ +-|---->^ +------------+ +->+------------+
| | | | ICEOF | | | | | |////////////|
| +------------+ v | | | | | |////////////|
BSM | |/ part of //| | |BSM | | | |////////////|
| |/ Internal /| | | | | | |////////////|
| |/ Context //| | | | | | |// Frame ///|
| +------------+ | | | | | ... |/ content //|
| | | | | | | | |////////////|
| | | | | | | | |////////////|
v +------------+ | | v +------------+ |////////////|
| Scatter/ //| sgt[0]--+ | |// Frame ///| |////////////|
| Gather List| ... | |/ content //| +------------+ ^
|////////////| sgt[N]----+ |////////////| | | | BEM
|////////////| |////////////| | | |
+------------+ +------------+ +------------+ v
BSM = Buffer Start Margin, BEM = Buffer End Margin, both are configured
by dpaa_eth_init_rx_port() for the RX FMan port relevant here.
sg_fd_to_skb() runs in the calling context of rx_default_dqrr() -
the NAPI receive callback - which only expects to receive contiguous
(qm_fd_contig) or scatter/gather (qm_fd_sg) frame descriptors.
Everything else is irrelevant codewise.
The processing done by sg_fd_to_skb() is weird because it does not
conform to the expectations laid out by the aforementioned figure.
Namely, it parses the OFFSET field only for SGT entries with i != 0
(codewise, skb != NULL). In those cases, OFFSET should always be 0.
Also, it does not parse the OFFSET field for the sgt[0] case, the only
case where the buffer offset is meaningful in this context. There, it
uses the fd_off, aka the offset to the Scatter/Gather List in the
Scatter/Gather Buffer from the figure. By equivalence, they should both
be equal to the BSM (in turn, equal to priv->rx_headroom).
This can actually be explained due to the bug which we had in
qm_sg_entry_get_off() until the previous change:
- qm_sg_entry_get_off() did not actually _work_ for sgt[0]. It returned
zero even with a non-zero offset, so fd_off had to be used as a fill-in.
- qm_sg_entry_get_off() always returned zero for sgt[i>0], and that
resulted in no user-visible bug, because the buffer offset _was
supposed_ to be zero for those buffers. So remove it from calculations.
Add assertions about the OFFSET field in both cases (first or subsequent
SGT entries) to make it absolutely obvious when something is not well
handled.
Similar logic can be seen in the driver for the architecturally similar
DPAA2, where dpaa2_eth_build_frag_skb() calls dpaa2_sg_get_offset() only
for i == 0. For the rest, there is even a comment stating the same thing:
* Data in subsequent SG entries is stored from the
* beginning of the buffer, so we don't need to add the
* sg_offset.
Tested on LS1046A.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
@@ -1863,6 +1862,11 @@ static struct sk_buff *sg_fd_to_skb(const struct dpaa_priv *priv,*onTx,ifextraheadersareadded.*/WARN_ON(fd_off!=priv->rx_headroom);+/* The offset to data start within the buffer holding+*theSGTshouldalwaysbeequaltotheoffsettodata+*startwithinthefirstbufferholdingtheframe.+*/+WARN_ON_ONCE(fd_off!=qm_sg_entry_get_off(&sgt[i]));skb_reserve(skb,fd_off);skb_put(skb,qm_sg_entry_get_len(&sgt[i]));}else{
@@ -1876,21 +1880,23 @@ static struct sk_buff *sg_fd_to_skb(const struct dpaa_priv *priv,page=virt_to_page(sg_vaddr);head_page=virt_to_head_page(sg_vaddr);-/* Compute offset in (possibly tail) page */+/* Compute offset of sg_vaddr in (possibly tail) page */page_offset=((unsignedlong)sg_vaddr&(PAGE_SIZE-1))+(page_address(page)-page_address(head_page));-/* page_offset only refers to the beginning of sgt[i];-*butthebufferitselfmayhaveaninternaloffset.++/* Non-initial SGT entries should not have a buffer+*offset.*/-frag_off=qm_sg_entry_get_off(&sgt[i])+page_offset;-frag_len=qm_sg_entry_get_len(&sgt[i]);+WARN_ON_ONCE(qm_sg_entry_get_off(&sgt[i]));+/* skb_add_rx_frag() does no checking on the page; if*wepassitatailpage,we'llendupwith-*badpageaccountingandeventuallywithsegafults.+*badpageaccountingandeventuallywithsegfaults.*/-skb_add_rx_frag(skb,i-1,head_page,frag_off,-frag_len,dpaa_bp->size);+skb_add_rx_frag(skb,i-1,head_page,page_offset,+qm_sg_entry_get_len(&sgt[i]),+dpaa_bp->size);}/* Update the pool count for the current {cpu x bpool} */
Sparse provides the following output:
warning: cast to restricted __be32
This is a harmless warning due to the fact that we dereference the hash
stored in the FD using an incorrect type annotation. Suppress the
warning by using the correct __be32 type instead of u32. No functional
change.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
struct qm_sg_entry :: offset is a 13-bit field, declared as __be16.
When using be32_to_cpu(), a wrong value will be calculated on little
endian systems (Arm), because type promotion from 16-bit to 32-bit,
which is done before the byte swap and always in the CPU native
endianness, changes the value of the scatter/gather list entry offset in
big-endian interpretation (adds two zero bytes in the LSB interpretation).
The result of the byte swap is ANDed with GENMASK(12, 0), so the result
is always zero, because only those bytes added by type promotion remain
after the application of the bit mask.
The impact of the bug is that scatter/gather frames with a non-zero
offset into the buffer are treated by the driver as if they had a zero
offset. This is all in theory, because in practice, qm_sg_entry_get_off()
has a single caller, where the bug is inconsequential, because at that
call site the buffer offset will always be zero, as will be explained in
the subsequent change.
Flagged by sparse:
warning: cast to restricted __be32
warning: cast from restricted __be16
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>