diff mbox series

net: ethernet: ti: am65-cpsw-qos: fix non-bql configs

Message ID 20240228140413.1862310-1-arnd@kernel.org
State New
Headers
Series net: ethernet: ti: am65-cpsw-qos: fix non-bql configs |

Commit Message

Arnd Bergmann Feb. 28, 2024, 2:03 p.m. UTC 
  From: Arnd Bergmann <arnd@arndb.de>

It is now possible to disable BQL, but that causes cpsw to break:

drivers/net/ethernet/ti/am65-cpsw-nuss.c:297:28: error: no member named 'dql' in 'struct netdev_queue'
  297 |                    dql_avail(&netif_txq->dql),

Add an #ifdef check for CONFIG_BQL around this usage.

Fixes: ea7f3cfaa588 ("net: bql: allow the config to be disabled")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Dave Taht Feb. 28, 2024, 2:44 p.m. UTC | #1 
but why do you want to disable BQL?

On Wed, Feb 28, 2024 at 9:04 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> It is now possible to disable BQL, but that causes cpsw to break:
>
> drivers/net/ethernet/ti/am65-cpsw-nuss.c:297:28: error: no member named 'dql' in 'struct netdev_queue'
>   297 |                    dql_avail(&netif_txq->dql),
>
> Add an #ifdef check for CONFIG_BQL around this usage.
>
> Fixes: ea7f3cfaa588 ("net: bql: allow the config to be disabled")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index 9d2f4ac783e4..3a3ec9959ee2 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -294,7 +294,11 @@ static void am65_cpsw_nuss_ndo_host_tx_timeout(struct net_device *ndev,
>                    txqueue,
>                    netif_tx_queue_stopped(netif_txq),
>                    jiffies_to_msecs(jiffies - trans_start),
> +#ifdef CONFIG_BQL
>                    dql_avail(&netif_txq->dql),
> +#else
> +                  0,
> +#endif
>                    k3_cppi_desc_pool_avail(tx_chn->desc_pool));
>
>         if (netif_tx_queue_stopped(netif_txq)) {
> --
> 2.39.2
>
>
Arnd Bergmann Feb. 28, 2024, 2:54 p.m. UTC | #2 
On Wed, Feb 28, 2024, at 15:44, Dave Taht wrote:
> but why do you want to disable BQL?

I have no idea, I'm just doing randconfig build tests.

I assume Breno has an answer for that, at least he
sent the patch that triggered the regression, see below.

     Arnd

commit ea7f3cfaa58873bbe271577efa800647e30f18bd
Author: Breno Leitao <leitao@debian.org>
Date:   Thu Feb 15 09:05:07 2024 -0800

    net: bql: allow the config to be disabled
    
    It is impossible to disable BQL individually today, since there is no
    prompt for the Kconfig entry, so, the BQL is always enabled if SYSFS is
    enabled.
    
    Create a prompt entry for BQL, so, it could be enabled or disabled at
    build time independently of SYSFS.
    
    Signed-off-by: Breno Leitao <leitao@debian.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/Kconfig b/net/Kconfig
index 4adc47d0c9c2..3e57ccf0da27 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -331,6 +331,7 @@ config NET_RX_BUSY_POLL
 
 config BQL
        bool
+       prompt "Enable Byte Queue Limits"
        depends on SYSFS
        select DQL
        default y
Breno Leitao Feb. 28, 2024, 2:56 p.m. UTC | #3 
On Wed, Feb 28, 2024 at 03:03:10PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> It is now possible to disable BQL, but that causes cpsw to break:
> 
> drivers/net/ethernet/ti/am65-cpsw-nuss.c:297:28: error: no member named 'dql' in 'struct netdev_queue'
>   297 |                    dql_avail(&netif_txq->dql),
> 
> Add an #ifdef check for CONFIG_BQL around this usage.

I confirmed that all other cases where queue->dql is accessed, it is
inside a #ifdef CONFIG_BQL block, so, seems appropriate doing the same
here.

> Fixes: ea7f3cfaa588 ("net: bql: allow the config to be disabled")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Reviwed-by: Breno Leitao <leitao@debian.org>
Jakub Kicinski Feb. 28, 2024, 3:25 p.m. UTC | #4 
On Wed, 28 Feb 2024 15:03:10 +0100 Arnd Bergmann wrote:
> +#ifdef CONFIG_BQL
>  		   dql_avail(&netif_txq->dql),
> +#else
> +		   0,
> +#endif

Same exact code exists in qdisc_avail_bulklimit()
How about we add static inline in netdevice.h wrapping
this, like we do for other BQL related functionality?
(taking in txq as the argument)
diff mbox series

Patch

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 9d2f4ac783e4..3a3ec9959ee2 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -294,7 +294,11 @@  static void am65_cpsw_nuss_ndo_host_tx_timeout(struct net_device *ndev,
 		   txqueue,
 		   netif_tx_queue_stopped(netif_txq),
 		   jiffies_to_msecs(jiffies - trans_start),
+#ifdef CONFIG_BQL
 		   dql_avail(&netif_txq->dql),
+#else
+		   0,
+#endif
 		   k3_cppi_desc_pool_avail(tx_chn->desc_pool));
 
 	if (netif_tx_queue_stopped(netif_txq)) {