| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1f5b71d6-872f-486d-9b06-7b5e407f8482@tuxon.dev>
Date: Mon, 24 Mar 2025 10:20:10 +0200
From: Claudiu Beznea <claudiu.beznea@...on.dev>
To: Théo Lebrun <theo.lebrun@...tlin.com>,
Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Nicolas Ferre <nicolas.ferre@...rochip.com>,
Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt
<palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
Alexandre Ghiti <alex@...ti.fr>, Samuel Holland <samuel.holland@...ive.com>,
Richard Cochran <richardcochran@...il.com>,
Russell King <linux@...linux.org.uk>,
Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>,
Gregory CLEMENT <gregory.clement@...tlin.com>
Cc: netdev@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
linux-mips@...r.kernel.org, Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
Tawfik Bayouk <tawfik.bayouk@...ileye.com>
Subject: Re: [PATCH net-next 08/13] net: macb: introduce DMA descriptor
helpers (is 64bit? is PTP?)
I think the "(is 64bit? is PTP?)" from title could be dropped.
On 21.03.2025 21:09, Théo Lebrun wrote:
> Introduce macb_dma_is_64b() and macb_dma_is_ptp() helper functions.
> Many codepaths are made simpler by dropping conditional compilation.
>
> This implies three changes:
> - Always compile related structure definitions inside <macb.h>.
> - Make the field hw_dma_cap in struct macb always present.
> - MACB_EXT_DESC can be dropped as it is useless now.
>
> The common case is:
>
> #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> struct macb_dma_desc_64 *desc_64;
> if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
> desc_64 = macb_64b_desc(bp, desc);
> // ...
> }
> #endif
>
> And replaced by:
>
> struct macb_dma_desc_64 *desc_64;
> if (macb_dma_is_64b(bp)) {
> desc_64 = macb_64b_desc(bp, desc);
> // ...
> }
>
> Signed-off-by: Théo Lebrun <theo.lebrun@...tlin.com>
> ---
> drivers/net/ethernet/cadence/macb.h | 8 ---
> drivers/net/ethernet/cadence/macb_main.c | 110 +++++++++++--------------------
> 2 files changed, 38 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 5bf7e7ff70490cdb068bfdbe7cfd5bb8e1db7f86..26e0af44a45926c782cf0f72184332ab3605a178 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -15,10 +15,6 @@
> #include <linux/phy/phy.h>
> #include <linux/workqueue.h>
>
> -#if defined(CONFIG_ARCH_DMA_ADDR_T_64BIT) || defined(CONFIG_MACB_USE_HWSTAMP)
> -#define MACB_EXT_DESC
> -#endif
> -
> #define MACB_GREGS_NBR 16
> #define MACB_GREGS_VERSION 2
> #define MACB_MAX_QUEUES 8
> @@ -824,7 +820,6 @@ struct macb_dma_desc {
> u32 ctrl;
> };
>
> -#ifdef MACB_EXT_DESC
> #define HW_DMA_CAP_32B 0
> #define HW_DMA_CAP_64B (1 << 0)
> #define HW_DMA_CAP_PTP (1 << 1)
> @@ -839,7 +834,6 @@ struct macb_dma_desc_ptp {
> u32 ts_1;
> u32 ts_2;
> };
> -#endif
>
> /* DMA descriptor bitfields */
> #define MACB_RX_USED_OFFSET 0
> @@ -1319,9 +1313,7 @@ struct macb {
>
> struct phy *sgmii_phy; /* for ZynqMP SGMII mode */
>
> -#ifdef MACB_EXT_DESC
> uint8_t hw_dma_cap;
> -#endif
> spinlock_t tsu_clk_lock; /* gem tsu clock locking */
> unsigned int tsu_rate;
> struct ptp_clock *ptp_clock;
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index b32363ba1ec3be0fc42866c8585f0b465d178220..ad154cfe29106f642b32922fd4a03ca63112f4a7 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -98,6 +98,18 @@ struct sifive_fu540_macb_mgmt {
>
> #define MACB_MDIO_TIMEOUT 1000000 /* in usecs */
>
> +static bool macb_dma_is_64b(struct macb *bp)
> +{
> + return IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT) &&
> + bp->hw_dma_cap & HW_DMA_CAP_64B;
> +}
> +
> +static bool macb_dma_is_ptp(struct macb *bp)
> +{
> + return IS_ENABLED(CONFIG_MACB_USE_HWSTAMP) &&
> + bp->hw_dma_cap & HW_DMA_CAP_PTP;
> +}
> +
> /* DMA buffer descriptor might be different size
> * depends on hardware configuration:
> *
> @@ -127,56 +139,31 @@ struct sifive_fu540_macb_mgmt {
> */
> static unsigned int macb_dma_desc_get_size(struct macb *bp)
> {
> -#ifdef MACB_EXT_DESC
> - unsigned int desc_size;
> + unsigned int desc_size = sizeof(struct macb_dma_desc);
> +
> + if (macb_dma_is_64b(bp))
> + desc_size += sizeof(struct macb_dma_desc_64);
> + if (macb_dma_is_ptp(bp))
> + desc_size += sizeof(struct macb_dma_desc_ptp);
>
> - switch (bp->hw_dma_cap) {
> - case HW_DMA_CAP_64B:
> - desc_size = sizeof(struct macb_dma_desc)
> - + sizeof(struct macb_dma_desc_64);
> - break;
> - case HW_DMA_CAP_PTP:
> - desc_size = sizeof(struct macb_dma_desc)
> - + sizeof(struct macb_dma_desc_ptp);
> - break;
> - case HW_DMA_CAP_64B_PTP:
> - desc_size = sizeof(struct macb_dma_desc)
> - + sizeof(struct macb_dma_desc_64)
> - + sizeof(struct macb_dma_desc_ptp);
> - break;
> - default:
> - desc_size = sizeof(struct macb_dma_desc);
> - }
> return desc_size;
> -#endif
> - return sizeof(struct macb_dma_desc);
> }
>
> static unsigned int macb_adj_dma_desc_idx(struct macb *bp, unsigned int desc_idx)
> {
> -#ifdef MACB_EXT_DESC
> - switch (bp->hw_dma_cap) {
> - case HW_DMA_CAP_64B:
> - case HW_DMA_CAP_PTP:
> - desc_idx <<= 1;
> - break;
> - case HW_DMA_CAP_64B_PTP:
> - desc_idx *= 3;
> - break;
> - default:
> - break;
> - }
> -#endif
> - return desc_idx;
> + if (macb_dma_is_64b(bp) && macb_dma_is_ptp(bp))
> + return desc_idx * 3;
> + else if (macb_dma_is_64b(bp) || macb_dma_is_ptp(bp))
> + return desc_idx << 1;
> + else
> + return desc_idx;
> }
>
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> static struct macb_dma_desc_64 *macb_64b_desc(struct macb *bp, struct macb_dma_desc *desc)
> {
> return (struct macb_dma_desc_64 *)((void *)desc
> + sizeof(struct macb_dma_desc));
> }
> -#endif
>
> /* Ring buffer accessors */
> static unsigned int macb_tx_ring_wrap(struct macb *bp, unsigned int index)
> @@ -500,17 +487,13 @@ static void macb_init_buffers(struct macb *bp)
>
> for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> queue_writel(queue, RBQP, lower_32_bits(queue->rx_ring_dma));
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> - if (bp->hw_dma_cap & HW_DMA_CAP_64B)
> + if (macb_dma_is_64b(bp))
> queue_writel(queue, RBQPH,
> upper_32_bits(queue->rx_ring_dma));
> -#endif
> queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> - if (bp->hw_dma_cap & HW_DMA_CAP_64B)
> + if (macb_dma_is_64b(bp))
> queue_writel(queue, TBQPH,
> upper_32_bits(queue->tx_ring_dma));
> -#endif
> }
> }
>
> @@ -1038,10 +1021,9 @@ static void macb_tx_unmap(struct macb *bp, struct macb_tx_skb *tx_skb, int budge
>
> static void macb_set_addr(struct macb *bp, struct macb_dma_desc *desc, dma_addr_t addr)
> {
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> struct macb_dma_desc_64 *desc_64;
This can be moved
>
> - if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
> + if (macb_dma_is_64b(bp)) {
here.
> desc_64 = macb_64b_desc(bp, desc);
> desc_64->addrh = upper_32_bits(addr);
> /* The low bits of RX address contain the RX_USED bit, clearing
> @@ -1050,26 +1032,22 @@ static void macb_set_addr(struct macb *bp, struct macb_dma_desc *desc, dma_addr_
> */
> dma_wmb();
> }
> -#endif
> +
> desc->addr = lower_32_bits(addr);
> }
>
> static dma_addr_t macb_get_addr(struct macb *bp, struct macb_dma_desc *desc)
> {
> - dma_addr_t addr = 0;
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> struct macb_dma_desc_64 *desc_64;
Same for this one.
> + dma_addr_t addr = 0;
>
> - if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
> + if (macb_dma_is_64b(bp)) {
> desc_64 = macb_64b_desc(bp, desc);
> addr = ((u64)(desc_64->addrh) << 32);
> }
> -#endif
> addr |= MACB_BF(RX_WADDR, MACB_BFEXT(RX_WADDR, desc->addr));
> -#ifdef CONFIG_MACB_USE_HWSTAMP
> - if (bp->hw_dma_cap & HW_DMA_CAP_PTP)
> + if (macb_dma_is_ptp(bp))
> addr &= ~GEM_BIT(DMA_RXVALID);
> -#endif
> return addr;
> }
>
> @@ -1176,10 +1154,8 @@ static void macb_tx_error_task(struct work_struct *work)
>
> /* Reinitialize the TX desc queue */
> queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> - if (bp->hw_dma_cap & HW_DMA_CAP_64B)
> + if (macb_dma_is_64b(bp))
> queue_writel(queue, TBQPH, upper_32_bits(queue->tx_ring_dma));
> -#endif
> /* Make TX ring reflect state of hardware */
> queue->tx_head = 0;
> queue->tx_tail = 0;
> @@ -2349,11 +2325,9 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
> return ret;
> }
>
> -#ifdef CONFIG_MACB_USE_HWSTAMP
> - if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> - (bp->hw_dma_cap & HW_DMA_CAP_PTP))
> + if (macb_dma_is_ptp(bp) &&
> + (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
> skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> -#endif
>
> is_lso = (skb_shinfo(skb)->gso_size != 0);
>
> @@ -2813,14 +2787,10 @@ static void macb_configure_dma(struct macb *bp)
> dmacfg &= ~GEM_BIT(TXCOEN);
>
> dmacfg &= ~GEM_BIT(ADDR64);
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> - if (bp->hw_dma_cap & HW_DMA_CAP_64B)
> + if (macb_dma_is_64b(bp))
> dmacfg |= GEM_BIT(ADDR64);
> -#endif
> -#ifdef CONFIG_MACB_USE_HWSTAMP
> - if (bp->hw_dma_cap & HW_DMA_CAP_PTP)
> + if (macb_dma_is_ptp(bp))
> dmacfg |= GEM_BIT(RXEXT) | GEM_BIT(TXEXT);
> -#endif
> netdev_dbg(bp->dev, "Cadence configure DMA with 0x%08x\n",
> dmacfg);
> gem_writel(bp, DMACFG, dmacfg);
> @@ -4326,12 +4296,10 @@ static int macb_init(struct platform_device *pdev)
> queue->TBQP = GEM_TBQP(hw_q - 1);
> queue->RBQP = GEM_RBQP(hw_q - 1);
> queue->RBQS = GEM_RBQS(hw_q - 1);
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> - if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
> + if (macb_dma_is_64b(bp)) {
> queue->TBQPH = GEM_TBQPH(hw_q - 1);
> queue->RBQPH = GEM_RBQPH(hw_q - 1);
> }
> -#endif
> } else {
> /* queue0 uses legacy registers */
> queue->ISR = MACB_ISR;
> @@ -4340,12 +4308,10 @@ static int macb_init(struct platform_device *pdev)
> queue->IMR = MACB_IMR;
> queue->TBQP = MACB_TBQP;
> queue->RBQP = MACB_RBQP;
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> - if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
> + if (macb_dma_is_64b(bp)) {
> queue->TBQPH = MACB_TBQPH;
> queue->RBQPH = MACB_RBQPH;
> }
> -#endif
> }
>
> /* get irq: here we use the linux queue index, not the hardware
>
Powered by blists - more mailing lists