| Message ID | 20240208-stmmac_irq-v1-1-8bab236026d4@linutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6256fbfd651c8b3653b7f1cfe08e635404e4b51d |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: stmmac: Simplify mtl IRQ status checking | expand |
On Thu, Feb 08, 2024 at 11:35:25AM +0100, Kurt Kanzenbach wrote: > Commit 8a7cb245cf28 ("net: stmmac: Do not enable RX FIFO overflow > interrupts") disabled the RX FIFO overflow interrupts. However, it left the > status variable around, but never checks it. > > As stmmac_host_mtl_irq_status() returns only 0 now, the code can be > simplified. > > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 04d817dc5899..10ce2f272b62 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -6036,10 +6036,8 @@ static void stmmac_common_interrupt(struct stmmac_priv *priv) > priv->tx_path_in_lpi_mode = false; > } > > - for (queue = 0; queue < queues_count; queue++) { > - status = stmmac_host_mtl_irq_status(priv, priv->hw, > - queue); > - } > + for (queue = 0; queue < queues_count; queue++) > + stmmac_host_mtl_irq_status(priv, priv->hw, queue); Hey Kurt, looks to me that all of the current callbacks just return 0 so why not make them return void instead? > > /* PCS link status */ > if (priv->hw->pcs && > > --- > base-commit: 006e89649fa913e285b931f1b8dfd6485d153ca7 > change-id: 20240208-stmmac_irq-57682fa778c9 > > Best regards, > -- > Kurt Kanzenbach <kurt@linutronix.de> > >
On Thu Feb 08 2024, Maciej Fijalkowski wrote: > On Thu, Feb 08, 2024 at 11:35:25AM +0100, Kurt Kanzenbach wrote: >> Commit 8a7cb245cf28 ("net: stmmac: Do not enable RX FIFO overflow >> interrupts") disabled the RX FIFO overflow interrupts. However, it left the >> status variable around, but never checks it. >> >> As stmmac_host_mtl_irq_status() returns only 0 now, the code can be >> simplified. >> >> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> >> --- >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> index 04d817dc5899..10ce2f272b62 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> @@ -6036,10 +6036,8 @@ static void stmmac_common_interrupt(struct stmmac_priv *priv) >> priv->tx_path_in_lpi_mode = false; >> } >> >> - for (queue = 0; queue < queues_count; queue++) { >> - status = stmmac_host_mtl_irq_status(priv, priv->hw, >> - queue); >> - } >> + for (queue = 0; queue < queues_count; queue++) >> + stmmac_host_mtl_irq_status(priv, priv->hw, queue); > > Hey Kurt, > > looks to me that all of the current callbacks just return 0 so why not > make them return void instead? Well, there are two callbacks of this in dwmac4 and dwxgmac2. Both of them still have the code for handling the overflow interrupt (and then returning != 0). However, as of commit 8a7cb245cf28 the interrupt shouldn't fire. So yes, it could be changed to void along with some code removal. But, maybe i'm missing something. Thanks, Kurt
On Thu, Feb 08, 2024 at 03:32:30PM +0100, Kurt Kanzenbach wrote: > On Thu Feb 08 2024, Maciej Fijalkowski wrote: > > On Thu, Feb 08, 2024 at 11:35:25AM +0100, Kurt Kanzenbach wrote: > >> Commit 8a7cb245cf28 ("net: stmmac: Do not enable RX FIFO overflow > >> interrupts") disabled the RX FIFO overflow interrupts. However, it left the > >> status variable around, but never checks it. > >> > >> As stmmac_host_mtl_irq_status() returns only 0 now, the code can be > >> simplified. > >> > >> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> > >> --- > >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++---- > >> 1 file changed, 2 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >> index 04d817dc5899..10ce2f272b62 100644 > >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >> @@ -6036,10 +6036,8 @@ static void stmmac_common_interrupt(struct stmmac_priv *priv) > >> priv->tx_path_in_lpi_mode = false; > >> } > >> > >> - for (queue = 0; queue < queues_count; queue++) { > >> - status = stmmac_host_mtl_irq_status(priv, priv->hw, > >> - queue); > >> - } > >> + for (queue = 0; queue < queues_count; queue++) > >> + stmmac_host_mtl_irq_status(priv, priv->hw, queue); > > > > Hey Kurt, > > > > looks to me that all of the current callbacks just return 0 so why not > > make them return void instead? > > Well, there are two callbacks of this in dwmac4 and dwxgmac2. Both of > them still have the code for handling the overflow interrupt (and then > returning != 0). However, as of commit 8a7cb245cf28 the interrupt > shouldn't fire. So yes, it could be changed to void along with some > code removal. But, maybe i'm missing something. Hmm, ok, my 'quick' glance over the code was too quick :) I missed overflow encoding to ret within callbacks, sorry. But it seems that even though they can return nonzero values they would be ignored, correct? > > Thanks, > Kurt
On Thu Feb 08 2024, Maciej Fijalkowski wrote: > On Thu, Feb 08, 2024 at 03:32:30PM +0100, Kurt Kanzenbach wrote: >> On Thu Feb 08 2024, Maciej Fijalkowski wrote: >> > On Thu, Feb 08, 2024 at 11:35:25AM +0100, Kurt Kanzenbach wrote: >> >> Commit 8a7cb245cf28 ("net: stmmac: Do not enable RX FIFO overflow >> >> interrupts") disabled the RX FIFO overflow interrupts. However, it left the >> >> status variable around, but never checks it. >> >> >> >> As stmmac_host_mtl_irq_status() returns only 0 now, the code can be >> >> simplified. >> >> >> >> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> >> >> --- >> >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++---- >> >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> >> index 04d817dc5899..10ce2f272b62 100644 >> >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> >> @@ -6036,10 +6036,8 @@ static void stmmac_common_interrupt(struct stmmac_priv *priv) >> >> priv->tx_path_in_lpi_mode = false; >> >> } >> >> >> >> - for (queue = 0; queue < queues_count; queue++) { >> >> - status = stmmac_host_mtl_irq_status(priv, priv->hw, >> >> - queue); >> >> - } >> >> + for (queue = 0; queue < queues_count; queue++) >> >> + stmmac_host_mtl_irq_status(priv, priv->hw, queue); >> > >> > Hey Kurt, >> > >> > looks to me that all of the current callbacks just return 0 so why not >> > make them return void instead? >> >> Well, there are two callbacks of this in dwmac4 and dwxgmac2. Both of >> them still have the code for handling the overflow interrupt (and then >> returning != 0). However, as of commit 8a7cb245cf28 the interrupt >> shouldn't fire. So yes, it could be changed to void along with some >> code removal. But, maybe i'm missing something. > > Hmm, ok, my 'quick' glance over the code was too quick :) I missed > overflow encoding to ret within callbacks, sorry. But it seems that even > though they can return nonzero values they would be ignored, correct? Yeah, they are ignored. Thanks, Kurt
On Thu Feb 08 2024, Kurt Kanzenbach wrote: > Commit 8a7cb245cf28 ("net: stmmac: Do not enable RX FIFO overflow > interrupts") disabled the RX FIFO overflow interrupts. However, it left the > status variable around, but never checks it. > > As stmmac_host_mtl_irq_status() returns only 0 now, the code can be > simplified. > > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> Hi Jakub, why did this got marked as Changes Requested. What changes have to be made? Thanks, Kurt
On Mon, 12 Feb 2024 13:17:37 +0100 Kurt Kanzenbach wrote: > On Thu Feb 08 2024, Kurt Kanzenbach wrote: > > Commit 8a7cb245cf28 ("net: stmmac: Do not enable RX FIFO overflow > > interrupts") disabled the RX FIFO overflow interrupts. However, it left the > > status variable around, but never checks it. > > > > As stmmac_host_mtl_irq_status() returns only 0 now, the code can be > > simplified. > > > > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> > > why did this got marked as Changes Requested. What changes have to be > made? Sorry, restored!
Hello: This patch was applied to netdev/net-next.git (main) by Paolo Abeni <pabeni@redhat.com>: On Thu, 08 Feb 2024 11:35:25 +0100 you wrote: > Commit 8a7cb245cf28 ("net: stmmac: Do not enable RX FIFO overflow > interrupts") disabled the RX FIFO overflow interrupts. However, it left the > status variable around, but never checks it. > > As stmmac_host_mtl_irq_status() returns only 0 now, the code can be > simplified. > > [...] Here is the summary with links: - [net-next] net: stmmac: Simplify mtl IRQ status checking https://git.kernel.org/netdev/net-next/c/6256fbfd651c You are awesome, thank you!
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 04d817dc5899..10ce2f272b62 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -6036,10 +6036,8 @@ static void stmmac_common_interrupt(struct stmmac_priv *priv) priv->tx_path_in_lpi_mode = false; } - for (queue = 0; queue < queues_count; queue++) { - status = stmmac_host_mtl_irq_status(priv, priv->hw, - queue); - } + for (queue = 0; queue < queues_count; queue++) + stmmac_host_mtl_irq_status(priv, priv->hw, queue); /* PCS link status */ if (priv->hw->pcs &&
Commit 8a7cb245cf28 ("net: stmmac: Do not enable RX FIFO overflow interrupts") disabled the RX FIFO overflow interrupts. However, it left the status variable around, but never checks it. As stmmac_host_mtl_irq_status() returns only 0 now, the code can be simplified. Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) --- base-commit: 006e89649fa913e285b931f1b8dfd6485d153ca7 change-id: 20240208-stmmac_irq-57682fa778c9 Best regards,