* Re: [PATCH] net: stmmac: mmc_core: Assign, don't add interrupt registers
@ 2024-02-16 17:13 ` Florian Fainelli
0 siblings, 0 replies; 12+ messages in thread
From: Florian Fainelli @ 2024-02-16 17:13 UTC (permalink / raw)
To: Jesper Nilsson, Alexandre Torgue, Jose Abreu, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin
Cc: netdev, linux-stm32, linux-arm-kernel, linux-kernel, kernel
On 2/16/24 07:24, Jesper Nilsson wrote:
> The MMC IPC interrupt status and interrupt mask registers are of
> little use as Ethernet statistics, but incrementing counters
> based on the current interrupt and interrupt mask registers
> makes them worse than useless.
>
> For example, if the interrupt mask is set to 0x08420842,
> the current code will increment by that amount each iteration,
> leading to the following sequence of nonsense:
>
> mmc_rx_ipc_intr_mask: 969816526
> mmc_rx_ipc_intr_mask: 1108361744
>
> Change the increment to a straight assignment to make the
> statistics at least nominally useful.
>
> Signed-off-by: Jesper Nilsson <jesper.nilsson@axis.com>
> ---
> drivers/net/ethernet/stmicro/stmmac/mmc_core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/mmc_core.c b/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
> index 6a7c1d325c46..6051a22b3cec 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
> @@ -280,8 +280,8 @@ static void dwmac_mmc_read(void __iomem *mmcaddr, struct stmmac_counters *mmc)
> mmc->mmc_rx_vlan_frames_gb += readl(mmcaddr + MMC_RX_VLAN_FRAMES_GB);
> mmc->mmc_rx_watchdog_error += readl(mmcaddr + MMC_RX_WATCHDOG_ERROR);
> /* IPC */
> - mmc->mmc_rx_ipc_intr_mask += readl(mmcaddr + MMC_RX_IPC_INTR_MASK);
> - mmc->mmc_rx_ipc_intr += readl(mmcaddr + MMC_RX_IPC_INTR);
> + mmc->mmc_rx_ipc_intr_mask = readl(mmcaddr + MMC_RX_IPC_INTR_MASK);
> + mmc->mmc_rx_ipc_intr = readl(mmcaddr + MMC_RX_IPC_INTR);
So in premise I agree with the patch, that incrementing those is not the
right way to go about them. However these registers are currently
provided as part of the statistics set, but they should instead be
accessed via the register dumping method.
In either case you will get at best a snapshot of those two registers at
any given time and I suppose this can help diagnose a stuck RX
condition, but not much more than that.
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: stmmac: mmc_core: Assign, don't add interrupt registers
2024-02-16 17:13 ` Florian Fainelli
@ 2024-02-16 18:24 ` Serge Semin
-1 siblings, 0 replies; 12+ messages in thread
From: Serge Semin @ 2024-02-16 18:24 UTC (permalink / raw)
To: Florian Fainelli, Jesper Nilsson
Cc: Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin, netdev,
linux-stm32, linux-arm-kernel, linux-kernel, kernel
On Fri, Feb 16, 2024 at 09:13:51AM -0800, Florian Fainelli wrote:
> On 2/16/24 07:24, Jesper Nilsson wrote:
> > The MMC IPC interrupt status and interrupt mask registers are of
> > little use as Ethernet statistics, but incrementing counters
> > based on the current interrupt and interrupt mask registers
> > makes them worse than useless.
> >
> > For example, if the interrupt mask is set to 0x08420842,
> > the current code will increment by that amount each iteration,
> > leading to the following sequence of nonsense:
> >
> > mmc_rx_ipc_intr_mask: 969816526
> > mmc_rx_ipc_intr_mask: 1108361744
> >
> > Change the increment to a straight assignment to make the
> > statistics at least nominally useful.
> >
> > Signed-off-by: Jesper Nilsson <jesper.nilsson@axis.com>
> > ---
> > drivers/net/ethernet/stmicro/stmmac/mmc_core.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/mmc_core.c b/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
> > index 6a7c1d325c46..6051a22b3cec 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
> > @@ -280,8 +280,8 @@ static void dwmac_mmc_read(void __iomem *mmcaddr, struct stmmac_counters *mmc)
> > mmc->mmc_rx_vlan_frames_gb += readl(mmcaddr + MMC_RX_VLAN_FRAMES_GB);
> > mmc->mmc_rx_watchdog_error += readl(mmcaddr + MMC_RX_WATCHDOG_ERROR);
> > /* IPC */
> > - mmc->mmc_rx_ipc_intr_mask += readl(mmcaddr + MMC_RX_IPC_INTR_MASK);
> > - mmc->mmc_rx_ipc_intr += readl(mmcaddr + MMC_RX_IPC_INTR);
> > + mmc->mmc_rx_ipc_intr_mask = readl(mmcaddr + MMC_RX_IPC_INTR_MASK);
> > + mmc->mmc_rx_ipc_intr = readl(mmcaddr + MMC_RX_IPC_INTR);
>
> So in premise I agree with the patch, that incrementing those is not the
> right way to go about them. However these registers are currently provided
> as part of the statistics set, but they should instead be accessed via the
> register dumping method.
>
> In either case you will get at best a snapshot of those two registers at any
> given time and I suppose this can help diagnose a stuck RX condition, but
> not much more than that.
Could you please clarify why do those CSRs state need to be exposed in
the statistics anyway? Who would need such information really?
Wouldn't that be better to just drop the
stmmac_counters::{mmc_rx_ipc_intr_mask,mmc_rx_ipc_intr}
fields? Is it because of the statistics nodes are a kind of kernel
ABI? Even in that case I don't see much reason to support something
that has been absolutely useless so far seeing the nodes currently
returning basically some random values.
-Serge(y)
> --
> Florian
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] net: stmmac: mmc_core: Assign, don't add interrupt registers
@ 2024-02-16 18:24 ` Serge Semin
0 siblings, 0 replies; 12+ messages in thread
From: Serge Semin @ 2024-02-16 18:24 UTC (permalink / raw)
To: Florian Fainelli, Jesper Nilsson
Cc: Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin, netdev,
linux-stm32, linux-arm-kernel, linux-kernel, kernel
On Fri, Feb 16, 2024 at 09:13:51AM -0800, Florian Fainelli wrote:
> On 2/16/24 07:24, Jesper Nilsson wrote:
> > The MMC IPC interrupt status and interrupt mask registers are of
> > little use as Ethernet statistics, but incrementing counters
> > based on the current interrupt and interrupt mask registers
> > makes them worse than useless.
> >
> > For example, if the interrupt mask is set to 0x08420842,
> > the current code will increment by that amount each iteration,
> > leading to the following sequence of nonsense:
> >
> > mmc_rx_ipc_intr_mask: 969816526
> > mmc_rx_ipc_intr_mask: 1108361744
> >
> > Change the increment to a straight assignment to make the
> > statistics at least nominally useful.
> >
> > Signed-off-by: Jesper Nilsson <jesper.nilsson@axis.com>
> > ---
> > drivers/net/ethernet/stmicro/stmmac/mmc_core.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/mmc_core.c b/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
> > index 6a7c1d325c46..6051a22b3cec 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
> > @@ -280,8 +280,8 @@ static void dwmac_mmc_read(void __iomem *mmcaddr, struct stmmac_counters *mmc)
> > mmc->mmc_rx_vlan_frames_gb += readl(mmcaddr + MMC_RX_VLAN_FRAMES_GB);
> > mmc->mmc_rx_watchdog_error += readl(mmcaddr + MMC_RX_WATCHDOG_ERROR);
> > /* IPC */
> > - mmc->mmc_rx_ipc_intr_mask += readl(mmcaddr + MMC_RX_IPC_INTR_MASK);
> > - mmc->mmc_rx_ipc_intr += readl(mmcaddr + MMC_RX_IPC_INTR);
> > + mmc->mmc_rx_ipc_intr_mask = readl(mmcaddr + MMC_RX_IPC_INTR_MASK);
> > + mmc->mmc_rx_ipc_intr = readl(mmcaddr + MMC_RX_IPC_INTR);
>
> So in premise I agree with the patch, that incrementing those is not the
> right way to go about them. However these registers are currently provided
> as part of the statistics set, but they should instead be accessed via the
> register dumping method.
>
> In either case you will get at best a snapshot of those two registers at any
> given time and I suppose this can help diagnose a stuck RX condition, but
> not much more than that.
Could you please clarify why do those CSRs state need to be exposed in
the statistics anyway? Who would need such information really?
Wouldn't that be better to just drop the
stmmac_counters::{mmc_rx_ipc_intr_mask,mmc_rx_ipc_intr}
fields? Is it because of the statistics nodes are a kind of kernel
ABI? Even in that case I don't see much reason to support something
that has been absolutely useless so far seeing the nodes currently
returning basically some random values.
-Serge(y)
> --
> Florian
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: stmmac: mmc_core: Assign, don't add interrupt registers
2024-02-16 17:13 ` Florian Fainelli
@ 2024-02-19 10:40 ` Jesper Nilsson
-1 siblings, 0 replies; 12+ messages in thread
From: Jesper Nilsson @ 2024-02-19 10:40 UTC (permalink / raw)
To: Florian Fainelli
Cc: Jesper Nilsson, Alexandre Torgue, Jose Abreu, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin,
netdev, linux-stm32, linux-arm-kernel, linux-kernel, kernel
On Fri, Feb 16, 2024 at 09:13:51AM -0800, Florian Fainelli wrote:
> So in premise I agree with the patch, that incrementing those is not the
> right way to go about them. However these registers are currently provided
> as part of the statistics set, but they should instead be accessed via the
> register dumping method.
You mean extending the dump register code to dump the MAC Management Counter
registers that are not counters?
From what I understand it's only the Rx and Tx interrupt and interrupt mask
registers that aren't counters. Oh, and the MMC control register itself?
To be honest, I don't think their use can justify the code churn.
> In either case you will get at best a snapshot of those two registers at any
> given time and I suppose this can help diagnose a stuck RX condition, but
> not much more than that.
Yeah, their use is very doubtful. For me, they only introduce more nonsense
data in my logs.
The Rx registers looks to have been added in the first version of the MMC
back in 2011, but the Tx registers never was.
In commit 1c901a46d57 Giuseppe mentions the MMC interrupts as something to
add later (if actually useful).
So Serge's suggestion to drop the entries completely is actually quite attractive.
> Florian
/^JN - Jesper Nilsson
--
Jesper Nilsson -- jesper.nilsson@axis.com
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] net: stmmac: mmc_core: Assign, don't add interrupt registers
@ 2024-02-19 10:40 ` Jesper Nilsson
0 siblings, 0 replies; 12+ messages in thread
From: Jesper Nilsson @ 2024-02-19 10:40 UTC (permalink / raw)
To: Florian Fainelli
Cc: Jesper Nilsson, Alexandre Torgue, Jose Abreu, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin,
netdev, linux-stm32, linux-arm-kernel, linux-kernel, kernel
On Fri, Feb 16, 2024 at 09:13:51AM -0800, Florian Fainelli wrote:
> So in premise I agree with the patch, that incrementing those is not the
> right way to go about them. However these registers are currently provided
> as part of the statistics set, but they should instead be accessed via the
> register dumping method.
You mean extending the dump register code to dump the MAC Management Counter
registers that are not counters?
From what I understand it's only the Rx and Tx interrupt and interrupt mask
registers that aren't counters. Oh, and the MMC control register itself?
To be honest, I don't think their use can justify the code churn.
> In either case you will get at best a snapshot of those two registers at any
> given time and I suppose this can help diagnose a stuck RX condition, but
> not much more than that.
Yeah, their use is very doubtful. For me, they only introduce more nonsense
data in my logs.
The Rx registers looks to have been added in the first version of the MMC
back in 2011, but the Tx registers never was.
In commit 1c901a46d57 Giuseppe mentions the MMC interrupts as something to
add later (if actually useful).
So Serge's suggestion to drop the entries completely is actually quite attractive.
> Florian
/^JN - Jesper Nilsson
--
Jesper Nilsson -- jesper.nilsson@axis.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] net: stmmac: mmc_core: Assign, don't add interrupt registers
2024-02-19 10:40 ` Jesper Nilsson
@ 2024-02-19 20:17 ` Florian Fainelli
-1 siblings, 0 replies; 12+ messages in thread
From: Florian Fainelli @ 2024-02-19 20:17 UTC (permalink / raw)
To: Jesper Nilsson
Cc: Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin, netdev,
linux-stm32, linux-arm-kernel, linux-kernel, kernel
On 2/19/2024 2:40 AM, Jesper Nilsson wrote:
> On Fri, Feb 16, 2024 at 09:13:51AM -0800, Florian Fainelli wrote:
>> So in premise I agree with the patch, that incrementing those is not the
>> right way to go about them. However these registers are currently provided
>> as part of the statistics set, but they should instead be accessed via the
>> register dumping method.
>
> You mean extending the dump register code to dump the MAC Management Counter
> registers that are not counters?
Yes that is what I meant.
> From what I understand it's only the Rx and Tx interrupt and interrupt mask
> registers that aren't counters. Oh, and the MMC control register itself?
>
> To be honest, I don't think their use can justify the code churn.
Fair enough, it really depends whether they are actually useful in
troubleshooting or not.
>
>> In either case you will get at best a snapshot of those two registers at any
>> given time and I suppose this can help diagnose a stuck RX condition, but
>> not much more than that.
>
> Yeah, their use is very doubtful. For me, they only introduce more nonsense
> data in my logs.
>
> The Rx registers looks to have been added in the first version of the MMC
> back in 2011, but the Tx registers never was.
> In commit 1c901a46d57 Giuseppe mentions the MMC interrupts as something to
> add later (if actually useful).
>
> So Serge's suggestion to drop the entries completely is actually quite attractive.
No concerns from me to drop them, not much value in having inaccurate
information.
--
Florian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: stmmac: mmc_core: Assign, don't add interrupt registers
@ 2024-02-19 20:17 ` Florian Fainelli
0 siblings, 0 replies; 12+ messages in thread
From: Florian Fainelli @ 2024-02-19 20:17 UTC (permalink / raw)
To: Jesper Nilsson
Cc: Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin, netdev,
linux-stm32, linux-arm-kernel, linux-kernel, kernel
On 2/19/2024 2:40 AM, Jesper Nilsson wrote:
> On Fri, Feb 16, 2024 at 09:13:51AM -0800, Florian Fainelli wrote:
>> So in premise I agree with the patch, that incrementing those is not the
>> right way to go about them. However these registers are currently provided
>> as part of the statistics set, but they should instead be accessed via the
>> register dumping method.
>
> You mean extending the dump register code to dump the MAC Management Counter
> registers that are not counters?
Yes that is what I meant.
> From what I understand it's only the Rx and Tx interrupt and interrupt mask
> registers that aren't counters. Oh, and the MMC control register itself?
>
> To be honest, I don't think their use can justify the code churn.
Fair enough, it really depends whether they are actually useful in
troubleshooting or not.
>
>> In either case you will get at best a snapshot of those two registers at any
>> given time and I suppose this can help diagnose a stuck RX condition, but
>> not much more than that.
>
> Yeah, their use is very doubtful. For me, they only introduce more nonsense
> data in my logs.
>
> The Rx registers looks to have been added in the first version of the MMC
> back in 2011, but the Tx registers never was.
> In commit 1c901a46d57 Giuseppe mentions the MMC interrupts as something to
> add later (if actually useful).
>
> So Serge's suggestion to drop the entries completely is actually quite attractive.
No concerns from me to drop them, not much value in having inaccurate
information.
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: stmmac: mmc_core: Assign, don't add interrupt registers
2024-02-19 10:40 ` Jesper Nilsson
@ 2024-02-20 10:51 ` Paolo Abeni
-1 siblings, 0 replies; 12+ messages in thread
From: Paolo Abeni @ 2024-02-20 10:51 UTC (permalink / raw)
To: Jesper Nilsson, Florian Fainelli
Cc: Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet,
Jakub Kicinski, Maxime Coquelin, netdev, linux-stm32,
linux-arm-kernel, linux-kernel, kernel
On Mon, 2024-02-19 at 11:40 +0100, Jesper Nilsson wrote:
> On Fri, Feb 16, 2024 at 09:13:51AM -0800, Florian Fainelli wrote:
> > So in premise I agree with the patch, that incrementing those is not the
> > right way to go about them. However these registers are currently provided
> > as part of the statistics set, but they should instead be accessed via the
> > register dumping method.
>
> You mean extending the dump register code to dump the MAC Management Counter
> registers that are not counters?
> > From what I understand it's only the Rx and Tx interrupt and interrupt mask
> registers that aren't counters. Oh, and the MMC control register itself?
>
> To be honest, I don't think their use can justify the code churn.
>
> > In either case you will get at best a snapshot of those two registers at any
> > given time and I suppose this can help diagnose a stuck RX condition, but
> > not much more than that.
>
> Yeah, their use is very doubtful. For me, they only introduce more nonsense
> data in my logs.
>
> The Rx registers looks to have been added in the first version of the MMC
> back in 2011, but the Tx registers never was.
> In commit 1c901a46d57 Giuseppe mentions the MMC interrupts as something to
> add later (if actually useful).
>
> So Serge's suggestion to drop the entries completely is actually quite attractive.
Please, go ahead and drop such entries.
Thank!
Paolo
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: stmmac: mmc_core: Assign, don't add interrupt registers
@ 2024-02-20 10:51 ` Paolo Abeni
0 siblings, 0 replies; 12+ messages in thread
From: Paolo Abeni @ 2024-02-20 10:51 UTC (permalink / raw)
To: Jesper Nilsson, Florian Fainelli
Cc: Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet,
Jakub Kicinski, Maxime Coquelin, netdev, linux-stm32,
linux-arm-kernel, linux-kernel, kernel
On Mon, 2024-02-19 at 11:40 +0100, Jesper Nilsson wrote:
> On Fri, Feb 16, 2024 at 09:13:51AM -0800, Florian Fainelli wrote:
> > So in premise I agree with the patch, that incrementing those is not the
> > right way to go about them. However these registers are currently provided
> > as part of the statistics set, but they should instead be accessed via the
> > register dumping method.
>
> You mean extending the dump register code to dump the MAC Management Counter
> registers that are not counters?
> > From what I understand it's only the Rx and Tx interrupt and interrupt mask
> registers that aren't counters. Oh, and the MMC control register itself?
>
> To be honest, I don't think their use can justify the code churn.
>
> > In either case you will get at best a snapshot of those two registers at any
> > given time and I suppose this can help diagnose a stuck RX condition, but
> > not much more than that.
>
> Yeah, their use is very doubtful. For me, they only introduce more nonsense
> data in my logs.
>
> The Rx registers looks to have been added in the first version of the MMC
> back in 2011, but the Tx registers never was.
> In commit 1c901a46d57 Giuseppe mentions the MMC interrupts as something to
> add later (if actually useful).
>
> So Serge's suggestion to drop the entries completely is actually quite attractive.
Please, go ahead and drop such entries.
Thank!
Paolo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread