[PATCH net-next] net: stmmac: Simplify mtl IRQ status checking

[PATCH net-next] net: stmmac: Simplify mtl IRQ status checking

[Kurt Kanzenbach]

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);
 
 		/* PCS link status */
 		if (priv->hw->pcs &&

---
base-commit: 006e89649fa913e285b931f1b8dfd6485d153ca7
change-id: 20240208-stmmac_irq-57682fa778c9

Best regards,
-- 
Kurt Kanzenbach <kurt@linutronix.de>

Re: [PATCH net-next] net: stmmac: Simplify mtl IRQ status checking

[Maciej Fijalkowski]

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>
> 
> 

Re: [PATCH net-next] net: stmmac: Simplify mtl IRQ status checking

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 1767 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH net-next] net: stmmac: Simplify mtl IRQ status checking

[Maciej Fijalkowski]

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

Re: [PATCH net-next] net: stmmac: Simplify mtl IRQ status checking

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 2248 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH net-next] net: stmmac: Simplify mtl IRQ status checking

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 480 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH net-next] net: stmmac: Simplify mtl IRQ status checking

[Jakub Kicinski]

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!

Re: [PATCH net-next] net: stmmac: Simplify mtl IRQ status checking

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html