| Message ID | 20241213030006.337695-1-0x1207@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v1] net: stmmac: TSO: Simplify the code flow of DMA descriptor allocations | expand |
On 12/13/24 04:00, Furong Xu wrote: > The DMA AXI address width of DWMAC cores can be configured to > 32-bit/40-bit/48-bit, then the format of DMA transmit descriptors > get a little different between 32-bit and 40-bit/48-bit. > Current driver code checks priv->dma_cap.addr64 to use certain format > with certain configuration. > > This patch converts the format of DMA transmit descriptors on platforms > that the DMA AXI address width is configured to 32-bit (as described by > function comments of stmmac_tso_xmit() in current code) to a more generic > format (see the updated function comments after this patch) which is > actually already used on 40-bit/48-bit platforms to provide better > compatibility and make code flow cleaner. > > Tested and verified on: > DWMAC CORE 5.10a with 32-bit DMA AXI address width > DWXGMAC CORE 3.20a with 40-bit DMA AXI address width > > Signed-off-by: Furong Xu <0x1207@gmail.com> Makes sense to me. Since this could potentially impact multiple versions, it would be great if we could have a little more 3rd parties testing. Thanks, Paolo
On Tue, 17 Dec 2024 10:30:24 +0100, Paolo Abeni <pabeni@redhat.com> wrote: > On 12/13/24 04:00, Furong Xu wrote: > > The DMA AXI address width of DWMAC cores can be configured to > > 32-bit/40-bit/48-bit, then the format of DMA transmit descriptors > > get a little different between 32-bit and 40-bit/48-bit. > > Current driver code checks priv->dma_cap.addr64 to use certain format > > with certain configuration. > > > > This patch converts the format of DMA transmit descriptors on platforms > > that the DMA AXI address width is configured to 32-bit (as described by > > function comments of stmmac_tso_xmit() in current code) to a more generic > > format (see the updated function comments after this patch) which is > > actually already used on 40-bit/48-bit platforms to provide better > > compatibility and make code flow cleaner. > > > > Tested and verified on: > > DWMAC CORE 5.10a with 32-bit DMA AXI address width > > DWXGMAC CORE 3.20a with 40-bit DMA AXI address width > > > > Signed-off-by: Furong Xu <0x1207@gmail.com> > > Makes sense to me. > > Since this could potentially impact multiple versions, it would be great > if we could have a little more 3rd parties testing. Totally agree. Multiple devices with multiple versions of DWMAC core which is configured to 32-bit DMA AXI address width seem to very hard to find and test this patch :( Jon Hunter @ NVIDIA has two versions of DWMAC cores different from mine, Tegra186 Jetson TX2 (DWMAC CORE 4.10) and Tegra194 Jetson AGX Xavier (DWMAC CORE 5.00), but both of them are configured to 40-bit DMA AXI address width, this does not match the case that this patch tries to convert. So I decided not to request him to provide help.
On Fri, 13 Dec 2024 11:00:06 +0800, Furong Xu <0x1207@gmail.com> wrote: > The DMA AXI address width of DWMAC cores can be configured to > 32-bit/40-bit/48-bit, then the format of DMA transmit descriptors > get a little different between 32-bit and 40-bit/48-bit. > Current driver code checks priv->dma_cap.addr64 to use certain format > with certain configuration. > > This patch converts the format of DMA transmit descriptors on platforms > that the DMA AXI address width is configured to 32-bit (as described by > function comments of stmmac_tso_xmit() in current code) to a more generic > format (see the updated function comments after this patch) which is > actually already used on 40-bit/48-bit platforms to provide better > compatibility and make code flow cleaner. > > Tested and verified on: > DWMAC CORE 5.10a with 32-bit DMA AXI address width > DWXGMAC CORE 3.20a with 40-bit DMA AXI address width One more DWMAC core tested and verified: DWMAC CORE 5.00a with 32-bit DMA AXI address width
On Fri, 13 Dec 2024 11:00:06 +0800 Furong Xu wrote: > - if (priv->dma_cap.addr64 <= 32) > - desc->des0 = cpu_to_le32(curr_addr); > - else > - stmmac_set_desc_addr(priv, desc, curr_addr); > - > + stmmac_set_desc_addr(priv, desc, curr_addr); I can't figure out if this is correct or not in a reasonable amount of time. dwmac4 and dwxgmac2 looks pretty obviously okay. But there are also ndesc and enh, which don't seem to map to platform in an obvious way to an outside reviewer. Please provide more context/guidance in the commit message, otherwise this looks like a high risk refactoring for a driver this poorly designed.
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 16b8bcfa8b11..a48b7cf74a29 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -4121,11 +4121,7 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, dma_addr_t des, desc = &tx_q->dma_tx[tx_q->cur_tx]; curr_addr = des + (total_len - tmp_len); - if (priv->dma_cap.addr64 <= 32) - desc->des0 = cpu_to_le32(curr_addr); - else - stmmac_set_desc_addr(priv, desc, curr_addr); - + stmmac_set_desc_addr(priv, desc, curr_addr); buff_size = tmp_len >= TSO_MAX_BUFF_SIZE ? TSO_MAX_BUFF_SIZE : tmp_len; @@ -4171,17 +4167,27 @@ static void stmmac_flush_tx_descriptors(struct stmmac_priv *priv, int queue) * First Descriptor * -------- * | DES0 |---> buffer1 = L2/L3/L4 header - * | DES1 |---> TCP Payload (can continue on next descr...) - * | DES2 |---> buffer 1 and 2 len + * | DES1 |---> can be used as buffer2 for TCP Payload if the DMA AXI address + * | | width is 32-bit, but we never use it. + * | | Also can be used as the most-significant 8-bits or 16-bits of + * | | buffer1 address pointer if the DMA AXI address width is 40-bit + * | | or 48-bit, and we always use it. + * | DES2 |---> buffer1 len * | DES3 |---> must set TSE, TCP hdr len-> [22:19]. TCP payload len [17:0] * -------- + * -------- + * | DES0 |---> buffer1 = TCP Payload (can continue on next descr...) + * | DES1 |---> same as the First Descriptor + * | DES2 |---> buffer1 len + * | DES3 | + * -------- * | * ... * | * -------- - * | DES0 | --| Split TCP Payload on Buffers 1 and 2 - * | DES1 | --| - * | DES2 | --> buffer 1 and 2 len + * | DES0 |---> buffer1 = Split TCP Payload + * | DES1 |---> same as the First Descriptor + * | DES2 |---> buffer1 len * | DES3 | * -------- * @@ -4191,15 +4197,14 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev) { struct dma_desc *desc, *first, *mss_desc = NULL; struct stmmac_priv *priv = netdev_priv(dev); - int tmp_pay_len = 0, first_tx, nfrags; unsigned int first_entry, tx_packets; struct stmmac_txq_stats *txq_stats; struct stmmac_tx_queue *tx_q; u32 pay_len, mss, queue; - dma_addr_t tso_des, des; + int i, first_tx, nfrags; u8 proto_hdr_len, hdr; + dma_addr_t des; bool set_ic; - int i; /* Always insert VLAN tag to SKB payload for TSO frames. * @@ -4284,24 +4289,9 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev) if (dma_mapping_error(priv->device, des)) goto dma_map_err; - if (priv->dma_cap.addr64 <= 32) { - first->des0 = cpu_to_le32(des); - - /* Fill start of payload in buff2 of first descriptor */ - if (pay_len) - first->des1 = cpu_to_le32(des + proto_hdr_len); - - /* If needed take extra descriptors to fill the remaining payload */ - tmp_pay_len = pay_len - TSO_MAX_BUFF_SIZE; - tso_des = des; - } else { - stmmac_set_desc_addr(priv, first, des); - tmp_pay_len = pay_len; - tso_des = des + proto_hdr_len; - pay_len = 0; - } - - stmmac_tso_allocator(priv, tso_des, tmp_pay_len, (nfrags == 0), queue); + stmmac_set_desc_addr(priv, first, des); + stmmac_tso_allocator(priv, des + proto_hdr_len, pay_len, + (nfrags == 0), queue); /* In case two or more DMA transmit descriptors are allocated for this * non-paged SKB data, the DMA buffer address should be saved to @@ -4405,11 +4395,9 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev) } /* Complete the first descriptor before granting the DMA */ - stmmac_prepare_tso_tx_desc(priv, first, 1, - proto_hdr_len, - pay_len, - 1, tx_q->tx_skbuff_dma[first_entry].last_segment, - hdr / 4, (skb->len - proto_hdr_len)); + stmmac_prepare_tso_tx_desc(priv, first, 1, proto_hdr_len, 0, 1, + tx_q->tx_skbuff_dma[first_entry].last_segment, + hdr / 4, (skb->len - proto_hdr_len)); /* If context desc is used to change MSS */ if (mss_desc) {
The DMA AXI address width of DWMAC cores can be configured to 32-bit/40-bit/48-bit, then the format of DMA transmit descriptors get a little different between 32-bit and 40-bit/48-bit. Current driver code checks priv->dma_cap.addr64 to use certain format with certain configuration. This patch converts the format of DMA transmit descriptors on platforms that the DMA AXI address width is configured to 32-bit (as described by function comments of stmmac_tso_xmit() in current code) to a more generic format (see the updated function comments after this patch) which is actually already used on 40-bit/48-bit platforms to provide better compatibility and make code flow cleaner. Tested and verified on: DWMAC CORE 5.10a with 32-bit DMA AXI address width DWXGMAC CORE 3.20a with 40-bit DMA AXI address width Signed-off-by: Furong Xu <0x1207@gmail.com> --- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 60 ++++++++----------- 1 file changed, 24 insertions(+), 36 deletions(-)