| Message ID | 20241029164317.50182-3-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix sparse warnings in dpaa_eth driver | expand |
On Tue, Oct 29, 2024 at 06:43:16PM +0200, Vladimir Oltean wrote:
> Tested on LS1046A.
..and now also tested on T2080 (PowerPC), no apparent regressions.
Le 29/10/2024 à 17:43, Vladimir Oltean a écrit : > 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> Acked-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > .../net/ethernet/freescale/dpaa/dpaa_eth.c | 24 ++++++++++++------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > index ac06b01fe934..e280013afa63 100644 > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > @@ -1820,7 +1820,6 @@ static struct sk_buff *sg_fd_to_skb(const struct dpaa_priv *priv, > struct page *page, *head_page; > struct dpaa_bp *dpaa_bp; > void *vaddr, *sg_vaddr; > - int frag_off, frag_len; > struct sk_buff *skb; > dma_addr_t sg_addr; > int page_offset; > @@ -1863,6 +1862,11 @@ static struct sk_buff *sg_fd_to_skb(const struct dpaa_priv *priv, > * on Tx, if extra headers are added. > */ > WARN_ON(fd_off != priv->rx_headroom); > + /* The offset to data start within the buffer holding > + * the SGT should always be equal to the offset to data > + * start within the first buffer holding the frame. > + */ > + 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 = ((unsigned long)sg_vaddr & > (PAGE_SIZE - 1)) + > (page_address(page) - page_address(head_page)); > - /* page_offset only refers to the beginning of sgt[i]; > - * but the buffer itself may have an internal offset. > + > + /* 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 > * we pass it a tail page, we'll end up with > - * bad page accounting and eventually with segafults. > + * bad page accounting and eventually with segfaults. > */ > - 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} */
diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c index ac06b01fe934..e280013afa63 100644 --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c @@ -1820,7 +1820,6 @@ static struct sk_buff *sg_fd_to_skb(const struct dpaa_priv *priv, struct page *page, *head_page; struct dpaa_bp *dpaa_bp; void *vaddr, *sg_vaddr; - int frag_off, frag_len; struct sk_buff *skb; dma_addr_t sg_addr; int page_offset; @@ -1863,6 +1862,11 @@ static struct sk_buff *sg_fd_to_skb(const struct dpaa_priv *priv, * on Tx, if extra headers are added. */ WARN_ON(fd_off != priv->rx_headroom); + /* The offset to data start within the buffer holding + * the SGT should always be equal to the offset to data + * start within the first buffer holding the frame. + */ + 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 = ((unsigned long)sg_vaddr & (PAGE_SIZE - 1)) + (page_address(page) - page_address(head_page)); - /* page_offset only refers to the beginning of sgt[i]; - * but the buffer itself may have an internal offset. + + /* 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 * we pass it a tail page, we'll end up with - * bad page accounting and eventually with segafults. + * bad page accounting and eventually with segfaults. */ - 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} */
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(-)