[PATCH net-next v7 04/14] net: lan966x: add FDMA LLP register write helper

[Daniel Machon]

> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 (1) · Medium: 1 · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] By moving the FDMA_DCB_LLP/LLP1 writes out of
>   lan966x_fdma_tx_activate() into lan966x_fdma_tx_alloc()…
> - [Low] The patch deletes the comment 'When activating a channel, first
>   is required to write the first DCB address and then to activate it'…
> 
> Pre-existing issues:
> - [High] lan966x_fdma_rx_alloc() (lan966x_fdma.c:135) and
>   lan966x_fdma_tx_alloc() (line 217) discard the return value of…
> 
> --
> 
> net: lan966x: add FDMA LLP register write helper
> 
> This moves the FDMA_DCB_LLP/LLP1 register writes out of
> lan966x_fdma_rx_start() and lan966x_fdma_tx_activate() into the RX/TX
> allocation functions, adding a shared lan966x_fdma_llp_configure() helper
> plus one extra call in the lan966x_fdma_reload() restore path.  The stated
> motivation is that the upcoming PCIe FDMA path programs ATU-translated
> addresses into these registers.
> 
> A few questions below.
> 
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > index 41d4ec7f2f57..b8344fd5e5ad 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > @@ -109,6 +109,13 @@ static int lan966x_fdma_rx_alloc_page_pool(struct lan966x_rx *rx)
> >       return 0;
> >  }
> >
> > +static void lan966x_fdma_llp_configure(struct lan966x *lan966x, u64 addr,
> > +                                    u8 channel_id)
> > +{
> > +     lan_wr(lower_32_bits(addr), lan966x, FDMA_DCB_LLP(channel_id));
> > +     lan_wr(upper_32_bits(addr), lan966x, FDMA_DCB_LLP1(channel_id));
> > +}
> > +
> >  static int lan966x_fdma_rx_alloc(struct lan966x_rx *rx)
> >  {
> >       struct lan966x *lan966x = rx->lan966x;
> > @@ -128,6 +135,8 @@ static int lan966x_fdma_rx_alloc(struct lan966x_rx *rx)
> >       fdma_dcbs_init(fdma, FDMA_DCB_INFO_DATAL(fdma->db_size),
> >                      FDMA_DCB_STATUS_INTR);
> >
> > +     lan966x_fdma_llp_configure(lan966x, fdma->dma, fdma->channel_id);
> > +
> >       return 0;
> >  }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but since the new LLP write now
> sits right after it: should lan966x_fdma_rx_alloc() check the return value
> of fdma_dcbs_init() before arming the channel?
> 
> fdma_dcbs_init() is fallible and stops at the first failing descriptor:
> 
> drivers/net/ethernet/microchip/fdma/fdma_api.c:fdma_dcbs_init() {
>         for (i = 0; i < fdma->n_dcbs; i++) {
>                 err = fdma_dcb_add(fdma, i, info, status);
>                 if (err)
>                         return err;
>         }
> }
> 
> and lan966x_fdma_rx_dataptr_cb() returns -ENOMEM when
> page_pool_dev_alloc_pages() fails, without updating rx->page[dcb][db] or
> *dataptr.  So under memory pressure the chain is left truncated, the
> remaining DB dataptrs stay 0, and rx->page[] keeps its previous contents
> while this function still returns 0 and now also programs the LLP.
> 
> In lan966x_fdma_reload() the success branch then runs, which releases every
> page in old_pages to the old pool and calls page_pool_destroy(page_pool)
> while rx->page[] still references those pages.  A later
> lan966x_fdma_rx_free_pages() at deinit, or on the next MTU change, would
> then call page_pool_put_full_page() on pages already returned to a
> destroyed pool.  In the probe path the same suppressed failure leaves NULL
> entries in rx->page[] that lan966x_fdma_rx_free_pages() dereferences
> unconditionally.
> 
> RX would also stall, since the software dcb_index wraps modulo n_dcbs while
> the hardware only walks the truncated part of the ring.
> 
> Interestingly the PCIe variant added later in this series does check the
> same call, so would it make sense to propagate the error here as well?
> 
> > @@ -137,14 +146,6 @@ static void lan966x_fdma_rx_start(struct lan966x_rx *rx)
> >       struct fdma *fdma = &rx->fdma;
> >       u32 mask;
> >
> > -     /* When activating a channel, first is required to write the first DCB
> > -      * address and then to activate it
> > -      */
> > -     lan_wr(lower_32_bits((u64)fdma->dma), lan966x,
> > -            FDMA_DCB_LLP(fdma->channel_id));
> > -     lan_wr(upper_32_bits((u64)fdma->dma), lan966x,
> > -            FDMA_DCB_LLP1(fdma->channel_id));
> > -
> >       lan_wr(FDMA_CH_CFG_CH_DCB_DB_CNT_SET(fdma->n_dbs) |
> >              FDMA_CH_CFG_CH_INTR_DB_EOF_ONLY_SET(1) |
> >              FDMA_CH_CFG_CH_INJ_PORT_SET(0) |
> 
> [Severity: Low]
> This isn't a bug, but the comment documenting the hardware ordering rule
> ("first is required to write the first DCB address and then to activate
> it") is removed from both lan966x_fdma_rx_start() and
> lan966x_fdma_tx_activate(), and is not restated at
> lan966x_fdma_llp_configure() or at either FDMA_CH_ACTIVATE site.
> 
> The requirement becomes an implicit contract between the allocation and
> activation functions, which is what forces the extra open-coded call in the
> lan966x_fdma_reload() restore path below.  Could the rule be documented at
> the helper, or near the FDMA_CH_ACTIVATE writes?
> 
> All activation paths in the current tree do have a matching LLP write
> before them, so this is only about the lost documentation.
> 
> Also, the commit message says the writes are moved "into the allocation
> functions", but a third call site is added in lan966x_fdma_reload(), which
> is not an allocation function.  Could that be mentioned as well?
> 
> > @@ -215,6 +216,8 @@ static int lan966x_fdma_tx_alloc(struct lan966x_tx *tx)
> >
> >       fdma_dcbs_init(fdma, 0, 0);
> >
> > +     lan966x_fdma_llp_configure(lan966x, fdma->dma, fdma->channel_id);
> > +
> >       return 0;
> >
> >  out:
> 
> [Severity: High]
> This is the same pre-existing unchecked fdma_dcbs_init() described above,
> on the TX side; it is not introduced by this patch, but the new LLP write
> and the unconditional "return 0" now follow it directly.
> 
> > @@ -236,14 +239,6 @@ static void lan966x_fdma_tx_activate(struct lan966x_tx *tx)
> >       struct fdma *fdma = &tx->fdma;
> >       u32 mask;
> >
> > -     /* When activating a channel, first is required to write the first DCB
> > -      * address and then to activate it
> > -      */
> > -     lan_wr(lower_32_bits((u64)fdma->dma), lan966x,
> > -            FDMA_DCB_LLP(fdma->channel_id));
> > -     lan_wr(upper_32_bits((u64)fdma->dma), lan966x,
> > -            FDMA_DCB_LLP1(fdma->channel_id));
> > -
> >       lan_wr(FDMA_CH_CFG_CH_DCB_DB_CNT_SET(fdma->n_dbs) |
> >              FDMA_CH_CFG_CH_INTR_DB_EOF_ONLY_SET(1) |
> >              FDMA_CH_CFG_CH_INJ_PORT_SET(0) |
> 
> [Severity: Medium]
> Can the injection channel now be activated before the LLP has been
> programmed during probe?
> 
> lan966x_probe() sets lan966x->fdma = true before the port loop that calls
> lan966x_probe_port() -> register_netdev(), and only calls
> lan966x_fdma_init() afterwards.  lan966x_port_xmit() dispatches purely on
> that flag:
> 
> drivers/net/ethernet/microchip/lan966x/lan966x_main.c:lan966x_port_xmit() {
>         spin_lock(&lan966x->tx_lock);
>         if (port->lan966x->fdma)
>                 err = lan966x_fdma_xmit(skb, ifh, dev);
>         ...
> }
> 
> lan966x_fdma_init() sets tx.fdma.n_dcbs/n_dbs and the ops before calling
> lan966x_fdma_tx_alloc(), and lan966x_fdma_tx_alloc() does not take
> tx_lock.  So an xmit landing between fdma_dcbs_init() and the new
> lan966x_fdma_llp_configure() would find a valid dcbs_buf and DCB ring and
> go:
> 
> lan966x_fdma_xmit() -> lan966x_fdma_tx_start() -> lan966x_fdma_tx_activate()
> 
> which after this patch writes FDMA_CH_ACTIVATE without programming the
> LLP, so the engine would fetch its first DCB from the post-reset LLP value
> of 0.  tx->activated is latched true there, so the LLP would never be
> programmed for that channel afterwards:
> 
>         if (likely(lan966x->tx.activated)) {
>                 lan966x_fdma_tx_reload(tx);
>         } else {
>                 /* Because it is first time, then just activate */
>                 lan966x->tx.activated = true;
>                 lan966x_fdma_tx_activate(tx);
>         }
> 
> Before this patch tx_activate() wrote the LLP immediately before the
> activate bit, so this ordering was not possible.  Note the same probe
> window already exposes a pre-existing tx->dcbs_buf == NULL dereference in
> lan966x_fdma_get_next_dcb() while n_dcbs is already set, so would
> registering the netdevs after lan966x_fdma_init() be the better fix?

Netdev registration should be done last, yes - and that is the fix that should
go to net (as mentioned in v6). The probe window is real, but not something
introduced by this series.

> 
> > @@ -876,6 +871,9 @@ static int lan966x_fdma_reload(struct lan966x *lan966x, int new_mtu)
> >                                          MEM_TYPE_PAGE_POOL, page_pool);
> >       }
> >
> > +     lan966x_fdma_llp_configure(lan966x, lan966x->rx.fdma.dma,
> > +                                lan966x->rx.fdma.channel_id);
> > +
> >       lan966x_fdma_rx_start(&lan966x->rx);
> >
> >       lan966x_fdma_wakeup_netdev(lan966x);
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918-lan966x-pci-fdma-v7-0-0ecc179c8a2c%40microchip.com