| Message ID | 1613040542-16500-9-git-send-email-stefanc@marvell.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bf270fa3c445faee7c2ca46f862f82f88d7fec3f |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mvpp2: Add TX Flow Control support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 5 this patch: 5 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 104 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 5 this patch: 5 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Thu, Feb 11, 2021 at 12:48:55PM +0200, stefanc@marvell.com wrote: > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > index 761f745..8b4073c 100644 > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > @@ -1133,14 +1133,19 @@ static inline void mvpp2_qvec_interrupt_disable(struct mvpp2_queue_vector *qvec) > static void mvpp2_interrupts_mask(void *arg) > { > struct mvpp2_port *port = arg; > + int cpu = smp_processor_id(); > + u32 thread; > > /* If the thread isn't used, don't do anything */ > - if (smp_processor_id() > port->priv->nthreads) > + if (cpu > port->priv->nthreads) > return; What happened to a patch fixing this? Did I miss it? Was it submitted independently to the net tree? > @@ -1150,20 +1155,25 @@ static void mvpp2_interrupts_mask(void *arg) > static void mvpp2_interrupts_unmask(void *arg) > { > struct mvpp2_port *port = arg; > - u32 val; > + int cpu = smp_processor_id(); > + u32 val, thread; > > /* If the thread isn't used, don't do anything */ > - if (smp_processor_id() > port->priv->nthreads) > + if (cpu > port->priv->nthreads) > return; Ditto. I don't think these need to be fixed in the net tree, but it would still be nice to fix the problem. Please do so, as an initial patch in your series - so we can then backport if it turns out to eventually be necessary. Thanks.
> -----Original Message----- > From: Russell King - ARM Linux admin <linux@armlinux.org.uk> > Sent: Thursday, February 11, 2021 2:50 PM > To: Stefan Chulski <stefanc@marvell.com> > Cc: netdev@vger.kernel.org; thomas.petazzoni@bootlin.com; > davem@davemloft.net; Nadav Haklai <nadavh@marvell.com>; Yan > Markman <ymarkman@marvell.com>; linux-kernel@vger.kernel.org; > kuba@kernel.org; mw@semihalf.com; andrew@lunn.ch; > atenart@kernel.org; devicetree@vger.kernel.org; robh+dt@kernel.org; > sebastian.hesselbarth@gmail.com; gregory.clement@bootlin.com; linux- > arm-kernel@lists.infradead.org > Subject: [EXT] Re: [PATCH v13 net-next 08/15] net: mvpp2: add FCA RXQ non > occupied descriptor threshold > > External Email > > ---------------------------------------------------------------------- > On Thu, Feb 11, 2021 at 12:48:55PM +0200, stefanc@marvell.com wrote: > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > index 761f745..8b4073c 100644 > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > @@ -1133,14 +1133,19 @@ static inline void > > mvpp2_qvec_interrupt_disable(struct mvpp2_queue_vector *qvec) static > > void mvpp2_interrupts_mask(void *arg) { > > struct mvpp2_port *port = arg; > > + int cpu = smp_processor_id(); > > + u32 thread; > > > > /* If the thread isn't used, don't do anything */ > > - if (smp_processor_id() > port->priv->nthreads) > > + if (cpu > port->priv->nthreads) > > return; > > What happened to a patch fixing this? Did I miss it? Was it submitted > independently to the net tree? Some reviewers asked to remove this from the series. I would send it as separate patch to net. Regards.
> Ditto. > > I don't think these need to be fixed in the net tree, but it would still be nice > to fix the problem. Please do so, as an initial patch in your series - so we can > then backport if it turns out to eventually be necessary. > > Thanks. My series already has 15 patches and patchwork not happy about series with over 15 patches. Maybe I can send this as separate patch to net-next(or net) first and base this series on this net-next tree with this patch? Regards, Stefan.
On Thu, Feb 11, 2021 at 01:02:14PM +0000, Stefan Chulski wrote: > > On Thu, Feb 11, 2021 at 12:48:55PM +0200, stefanc@marvell.com wrote: > > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > > b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > > index 761f745..8b4073c 100644 > > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > > @@ -1133,14 +1133,19 @@ static inline void > > > mvpp2_qvec_interrupt_disable(struct mvpp2_queue_vector *qvec) static > > > void mvpp2_interrupts_mask(void *arg) { > > > struct mvpp2_port *port = arg; > > > + int cpu = smp_processor_id(); > > > + u32 thread; > > > > > > /* If the thread isn't used, don't do anything */ > > > - if (smp_processor_id() > port->priv->nthreads) > > > + if (cpu > port->priv->nthreads) > > > return; > > > > What happened to a patch fixing this? Did I miss it? Was it submitted > > independently to the net tree? > > Some reviewers asked to remove this from the series. I would send it as separate patch to net. It is not a regression, and although it is a fix, as you explained when I first raised it, it isn't a condition that can be reached due to: priv->nthreads = min_t(unsigned int, num_present_cpus(), MVPP2_MAX_THREADS); and I don't think we support a dynamic present CPU mask on any platform that is currently supported by this driver. If we did, then it would be possible for the off-by-one issue to be triggered. No matter what, it should happen _before_ this patch set is merged. Trying to do it afterwards guarantees more pain if stable trees decide they want to backport the fix.
On Thu, Feb 11, 2021 at 01:22:35PM +0000, Stefan Chulski wrote: > > Ditto. > > > > I don't think these need to be fixed in the net tree, but it would still be nice > > to fix the problem. Please do so, as an initial patch in your series - so we can > > then backport if it turns out to eventually be necessary. > > > > Thanks. > > My series already has 15 patches and patchwork not happy about series with over 15 patches. > Maybe I can send this as separate patch to net-next(or net) first and base this series on this net-next tree with this patch? In that case, send the fixes as a separate series and get that merged first. It shouldn't take very long to get the fixes merged.
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h index 9239d80..d2cc513c 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h @@ -295,6 +295,8 @@ #define MVPP2_PON_CAUSE_TXP_OCCUP_DESC_ALL_MASK 0x3fc00000 #define MVPP2_PON_CAUSE_MISC_SUM_MASK BIT(31) #define MVPP2_ISR_MISC_CAUSE_REG 0x55b0 +#define MVPP2_ISR_RX_ERR_CAUSE_REG(port) (0x5520 + 4 * (port)) +#define MVPP2_ISR_RX_ERR_CAUSE_NONOCC_MASK 0x00ff /* Buffer Manager registers */ #define MVPP2_BM_POOL_BASE_REG(pool) (0x6000 + ((pool) * 4)) @@ -763,6 +765,7 @@ /* MSS Flow control */ #define FC_QUANTA 0xFFFF #define FC_CLK_DIVIDER 100 +#define MSS_THRESHOLD_STOP 768 /* RX buffer constants */ #define MVPP2_SKB_SHINFO_SIZE \ diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index 761f745..8b4073c 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -1133,14 +1133,19 @@ static inline void mvpp2_qvec_interrupt_disable(struct mvpp2_queue_vector *qvec) static void mvpp2_interrupts_mask(void *arg) { struct mvpp2_port *port = arg; + int cpu = smp_processor_id(); + u32 thread; /* If the thread isn't used, don't do anything */ - if (smp_processor_id() > port->priv->nthreads) + if (cpu > port->priv->nthreads) return; - mvpp2_thread_write(port->priv, - mvpp2_cpu_to_thread(port->priv, smp_processor_id()), + thread = mvpp2_cpu_to_thread(port->priv, cpu); + + mvpp2_thread_write(port->priv, thread, MVPP2_ISR_RX_TX_MASK_REG(port->id), 0); + mvpp2_thread_write(port->priv, thread, + MVPP2_ISR_RX_ERR_CAUSE_REG(port->id), 0); } /* Unmask the current thread's Rx/Tx interrupts. @@ -1150,20 +1155,25 @@ static void mvpp2_interrupts_mask(void *arg) static void mvpp2_interrupts_unmask(void *arg) { struct mvpp2_port *port = arg; - u32 val; + int cpu = smp_processor_id(); + u32 val, thread; /* If the thread isn't used, don't do anything */ - if (smp_processor_id() > port->priv->nthreads) + if (cpu > port->priv->nthreads) return; + thread = mvpp2_cpu_to_thread(port->priv, cpu); + val = MVPP2_CAUSE_MISC_SUM_MASK | MVPP2_CAUSE_RXQ_OCCUP_DESC_ALL_MASK(port->priv->hw_version); if (port->has_tx_irqs) val |= MVPP2_CAUSE_TXQ_OCCUP_DESC_ALL_MASK; - mvpp2_thread_write(port->priv, - mvpp2_cpu_to_thread(port->priv, smp_processor_id()), + mvpp2_thread_write(port->priv, thread, MVPP2_ISR_RX_TX_MASK_REG(port->id), val); + mvpp2_thread_write(port->priv, thread, + MVPP2_ISR_RX_ERR_CAUSE_REG(port->id), + MVPP2_ISR_RX_ERR_CAUSE_NONOCC_MASK); } static void @@ -1188,6 +1198,9 @@ static void mvpp2_interrupts_unmask(void *arg) mvpp2_thread_write(port->priv, v->sw_thread_id, MVPP2_ISR_RX_TX_MASK_REG(port->id), val); + mvpp2_thread_write(port->priv, v->sw_thread_id, + MVPP2_ISR_RX_ERR_CAUSE_REG(port->id), + MVPP2_ISR_RX_ERR_CAUSE_NONOCC_MASK); } } @@ -2393,6 +2406,20 @@ static void mvpp2_txp_max_tx_size_set(struct mvpp2_port *port) } } +/* Set the number of non-occupied descriptors threshold */ +static void mvpp2_set_rxq_free_tresh(struct mvpp2_port *port, + struct mvpp2_rx_queue *rxq) +{ + u32 val; + + mvpp2_write(port->priv, MVPP2_RXQ_NUM_REG, rxq->id); + + val = mvpp2_read(port->priv, MVPP2_RXQ_THRESH_REG); + val &= ~MVPP2_RXQ_NON_OCCUPIED_MASK; + val |= MSS_THRESHOLD_STOP << MVPP2_RXQ_NON_OCCUPIED_OFFSET; + mvpp2_write(port->priv, MVPP2_RXQ_THRESH_REG, val); +} + /* Set the number of packets that will be received before Rx interrupt * will be generated by HW. */ @@ -2648,6 +2675,9 @@ static int mvpp2_rxq_init(struct mvpp2_port *port, mvpp2_rx_pkts_coal_set(port, rxq); mvpp2_rx_time_coal_set(port, rxq); + /* Set the number of non occupied descriptors threshold */ + mvpp2_set_rxq_free_tresh(port, rxq); + /* Add number of descriptors ready for receiving packets */ mvpp2_rxq_status_update(port, rxq->id, 0, rxq->size);