diff mbox series

[v1,net-next] net: dsa: qca: ar9331: add ethtool stats support

Message ID 20201115073533.1366-1-o.rempel@pengutronix.de (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [v1,net-next] net: dsa: qca: ar9331: add ethtool stats support | expand

Checks

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/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: 0 this patch: 0
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, 137 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Oleksij Rempel Nov. 15, 2020, 7:35 a.m. UTC
Add stats support for the ar9331 switch.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/qca/ar9331.c | 113 +++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

Comments

Andrew Lunn Nov. 15, 2020, 3:47 p.m. UTC | #1
On Sun, Nov 15, 2020 at 08:35:33AM +0100, Oleksij Rempel wrote:
> Add stats support for the ar9331 switch.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Jakub Kicinski Nov. 16, 2020, 9:34 p.m. UTC | #2
On Sun, 15 Nov 2020 08:35:33 +0100 Oleksij Rempel wrote:
> +static const struct ar9331_mib_desc ar9331_mib[] = {
> +	MIB_DESC(1, 0x00, "RxBroad"),
> +	MIB_DESC(1, 0x04, "RxPause"),
> +	MIB_DESC(1, 0x08, "RxMulti"),
> +	MIB_DESC(1, 0x0c, "RxFcsErr"),
> +	MIB_DESC(1, 0x10, "RxAlignErr"),
> +	MIB_DESC(1, 0x14, "RxRunt"),
> +	MIB_DESC(1, 0x18, "RxFragment"),
> +	MIB_DESC(1, 0x1c, "Rx64Byte"),
> +	MIB_DESC(1, 0x20, "Rx128Byte"),
> +	MIB_DESC(1, 0x24, "Rx256Byte"),
> +	MIB_DESC(1, 0x28, "Rx512Byte"),
> +	MIB_DESC(1, 0x2c, "Rx1024Byte"),
> +	MIB_DESC(1, 0x30, "Rx1518Byte"),
> +	MIB_DESC(1, 0x34, "RxMaxByte"),
> +	MIB_DESC(1, 0x38, "RxTooLong"),
> +	MIB_DESC(2, 0x3c, "RxGoodByte"),
> +	MIB_DESC(2, 0x44, "RxBadByte"),
> +	MIB_DESC(1, 0x4c, "RxOverFlow"),
> +	MIB_DESC(1, 0x50, "Filtered"),
> +	MIB_DESC(1, 0x54, "TxBroad"),
> +	MIB_DESC(1, 0x58, "TxPause"),
> +	MIB_DESC(1, 0x5c, "TxMulti"),
> +	MIB_DESC(1, 0x60, "TxUnderRun"),
> +	MIB_DESC(1, 0x64, "Tx64Byte"),
> +	MIB_DESC(1, 0x68, "Tx128Byte"),
> +	MIB_DESC(1, 0x6c, "Tx256Byte"),
> +	MIB_DESC(1, 0x70, "Tx512Byte"),
> +	MIB_DESC(1, 0x74, "Tx1024Byte"),
> +	MIB_DESC(1, 0x78, "Tx1518Byte"),
> +	MIB_DESC(1, 0x7c, "TxMaxByte"),
> +	MIB_DESC(1, 0x80, "TxOverSize"),
> +	MIB_DESC(2, 0x84, "TxByte"),
> +	MIB_DESC(1, 0x8c, "TxCollision"),
> +	MIB_DESC(1, 0x90, "TxAbortCol"),
> +	MIB_DESC(1, 0x94, "TxMultiCol"),
> +	MIB_DESC(1, 0x98, "TxSingleCol"),
> +	MIB_DESC(1, 0x9c, "TxExcDefer"),
> +	MIB_DESC(1, 0xa0, "TxDefer"),
> +	MIB_DESC(1, 0xa4, "TxLateCol"),
> +};

You must expose relevant statistics via the normal get_stats64 NDO
before you start dumping free form stuff in ethtool -S.
Vladimir Oltean Nov. 16, 2020, 10:21 p.m. UTC | #3
On Mon, Nov 16, 2020 at 01:34:53PM -0800, Jakub Kicinski wrote:
> You must expose relevant statistics via the normal get_stats64 NDO
> before you start dumping free form stuff in ethtool -S.

Completely agree on the point, Jakub, but to be honest we don't give him
that possibility within the DSA framework today, see .ndo_get_stats64 in
net/dsa/slave.c which returns the generic dev_get_tstats64 implementation,
and not something that hooks into the hardware counters, or into the
driver at all, for that matter.

But it's good that you raise the point, I was thinking too that we
should do better in terms of keeping the software counters in sync with
the hardware. But what would be a good reference for keeping statistics
on an offloaded interface? Is it ok to just populate the netdev counters
based on the hardware statistics? And what about the statistics gathered
today in software, could we return them maybe via something like ifstat
--extended=cpu_hits?
Jakub Kicinski Nov. 16, 2020, 10:35 p.m. UTC | #4
On Tue, 17 Nov 2020 00:21:46 +0200 Vladimir Oltean wrote:
> On Mon, Nov 16, 2020 at 01:34:53PM -0800, Jakub Kicinski wrote:
> > You must expose relevant statistics via the normal get_stats64 NDO
> > before you start dumping free form stuff in ethtool -S.  
> 
> Completely agree on the point, Jakub, but to be honest we don't give him
> that possibility within the DSA framework today, see .ndo_get_stats64 in
> net/dsa/slave.c which returns the generic dev_get_tstats64 implementation,
> and not something that hooks into the hardware counters, or into the
> driver at all, for that matter.

Simple matter of coding, right? I don't see a problem.

Also I only mentioned .ndo_get_stats64, but now we also have stats in
ethtool->get_pause_stats.

> But it's good that you raise the point, I was thinking too that we
> should do better in terms of keeping the software counters in sync with
> the hardware. But what would be a good reference for keeping statistics
> on an offloaded interface? Is it ok to just populate the netdev counters
> based on the hardware statistics?

IIRC the stats on the interface should be a sum of forwarded in software
and in hardware. Which in practice means interface HW stats are okay,
given eventually both forwarding types end up in the HW interface
(/MAC block).

> And what about the statistics gathered
> today in software, could we return them maybe via something like ifstat
> --extended=cpu_hits?

Yup, exactly, that's what --extended=cpu_hits is for.
Vladimir Oltean Nov. 16, 2020, 11 p.m. UTC | #5
On Mon, Nov 16, 2020 at 02:35:44PM -0800, Jakub Kicinski wrote:
> On Tue, 17 Nov 2020 00:21:46 +0200 Vladimir Oltean wrote:
> > On Mon, Nov 16, 2020 at 01:34:53PM -0800, Jakub Kicinski wrote:
> > > You must expose relevant statistics via the normal get_stats64 NDO
> > > before you start dumping free form stuff in ethtool -S.
> >
> > Completely agree on the point, Jakub, but to be honest we don't give him
> > that possibility within the DSA framework today, see .ndo_get_stats64 in
> > net/dsa/slave.c which returns the generic dev_get_tstats64 implementation,
> > and not something that hooks into the hardware counters, or into the
> > driver at all, for that matter.
>
> Simple matter of coding, right? I don't see a problem.
>
> Also I only mentioned .ndo_get_stats64, but now we also have stats in
> ethtool->get_pause_stats.

Yes, sure we can do that. The pause stats and packet counter ops would
need to be exposed to the drivers by DSA first, though. Not sure if this
is something you expect Oleksij to do or if we could pick that up separately
afterwards.

> > But it's good that you raise the point, I was thinking too that we
> > should do better in terms of keeping the software counters in sync with
> > the hardware. But what would be a good reference for keeping statistics
> > on an offloaded interface? Is it ok to just populate the netdev counters
> > based on the hardware statistics?
>
> IIRC the stats on the interface should be a sum of forwarded in software
> and in hardware. Which in practice means interface HW stats are okay,
> given eventually both forwarding types end up in the HW interface
> (/MAC block).

A sum? Wouldn't that count the packets sent/received by the stack twice?
Jakub Kicinski Nov. 16, 2020, 11:13 p.m. UTC | #6
On Tue, 17 Nov 2020 01:00:53 +0200 Vladimir Oltean wrote:
> On Mon, Nov 16, 2020 at 02:35:44PM -0800, Jakub Kicinski wrote:
> > On Tue, 17 Nov 2020 00:21:46 +0200 Vladimir Oltean wrote:  
> > > On Mon, Nov 16, 2020 at 01:34:53PM -0800, Jakub Kicinski wrote:  
> > > > You must expose relevant statistics via the normal get_stats64 NDO
> > > > before you start dumping free form stuff in ethtool -S.  
> > >
> > > Completely agree on the point, Jakub, but to be honest we don't give him
> > > that possibility within the DSA framework today, see .ndo_get_stats64 in
> > > net/dsa/slave.c which returns the generic dev_get_tstats64 implementation,
> > > and not something that hooks into the hardware counters, or into the
> > > driver at all, for that matter.  
> >
> > Simple matter of coding, right? I don't see a problem.
> >
> > Also I only mentioned .ndo_get_stats64, but now we also have stats in
> > ethtool->get_pause_stats.  
> 
> Yes, sure we can do that. The pause stats and packet counter ops would
> need to be exposed to the drivers by DSA first, though. Not sure if this
> is something you expect Oleksij to do or if we could pick that up separately
> afterwards.

Well, I feel like unless we draw the line nobody will have 
the incentive to do the work.

I don't mind if it's Oleksij or anyone else doing the plumbing work,
but the task itself seems rather trivial.

> > > But it's good that you raise the point, I was thinking too that we
> > > should do better in terms of keeping the software counters in sync with
> > > the hardware. But what would be a good reference for keeping statistics
> > > on an offloaded interface? Is it ok to just populate the netdev counters
> > > based on the hardware statistics?  
> >
> > IIRC the stats on the interface should be a sum of forwarded in software
> > and in hardware. Which in practice means interface HW stats are okay,
> > given eventually both forwarding types end up in the HW interface
> > (/MAC block).  
> 
> A sum? Wouldn't that count the packets sent/received by the stack twice?

Note that I said _forwarded_. Frames are either forwarded by the HW or
SW (former never hit the CPU, while the latter do hit the CPU or
originate from it).
Vladimir Oltean Nov. 16, 2020, 11:27 p.m. UTC | #7
On Mon, Nov 16, 2020 at 03:13:47PM -0800, Jakub Kicinski wrote:
> On Tue, 17 Nov 2020 01:00:53 +0200 Vladimir Oltean wrote:
> > On Mon, Nov 16, 2020 at 02:35:44PM -0800, Jakub Kicinski wrote:
> > > On Tue, 17 Nov 2020 00:21:46 +0200 Vladimir Oltean wrote:
> > > > On Mon, Nov 16, 2020 at 01:34:53PM -0800, Jakub Kicinski wrote:
> > > > > You must expose relevant statistics via the normal get_stats64 NDO
> > > > > before you start dumping free form stuff in ethtool -S.
> > > >
> > > > Completely agree on the point, Jakub, but to be honest we don't give him
> > > > that possibility within the DSA framework today, see .ndo_get_stats64 in
> > > > net/dsa/slave.c which returns the generic dev_get_tstats64 implementation,
> > > > and not something that hooks into the hardware counters, or into the
> > > > driver at all, for that matter.
> > >
> > > Simple matter of coding, right? I don't see a problem.
> > >
> > > Also I only mentioned .ndo_get_stats64, but now we also have stats in
> > > ethtool->get_pause_stats.
> >
> > Yes, sure we can do that. The pause stats and packet counter ops would
> > need to be exposed to the drivers by DSA first, though. Not sure if this
> > is something you expect Oleksij to do or if we could pick that up separately
> > afterwards.
>
> Well, I feel like unless we draw the line nobody will have
> the incentive to do the work.
>
> I don't mind if it's Oleksij or anyone else doing the plumbing work,
> but the task itself seems rather trivial.

So then I'll let Oleksij show his availability.

> > > > But it's good that you raise the point, I was thinking too that we
> > > > should do better in terms of keeping the software counters in sync with
> > > > the hardware. But what would be a good reference for keeping statistics
> > > > on an offloaded interface? Is it ok to just populate the netdev counters
> > > > based on the hardware statistics?
> > >
> > > IIRC the stats on the interface should be a sum of forwarded in software
> > > and in hardware. Which in practice means interface HW stats are okay,
> > > given eventually both forwarding types end up in the HW interface
> > > (/MAC block).
> >
> > A sum? Wouldn't that count the packets sent/received by the stack twice?
>
> Note that I said _forwarded_. Frames are either forwarded by the HW or
> SW (former never hit the CPU, while the latter do hit the CPU or
> originate from it).

Ah, you were just thinking out loud, I really did not understand what
you meant by the separation between "forwarded in software" and
"forwarded in hardware".
Yes, the hardware typically only gives us MAC-level counters anyway.
Another way to look at it is that the number of packets forwarded in
hardware from a given port are equal to the total number of RX packets
on that MAC minus the packets seen by the CPU coming from that port.
So all in all, it's the MAC-level counters we should expose in
.ndo_get_stats64, I'm glad you agree. As for the error packets, I
suppose that would be a driver-specific aggregate.

What about RMON/RFC2819 style etherStatsPkts65to127Octets? We have a
number of switches supporting that style of counters, including the one
that Oleksij is adding support for, apparently (but not all switches
though). I suppose your M.O. is that anything standardizable is welcome
to be standardized via rtnetlink?

Andrew, Florian, any opinions here?
Florian Fainelli Nov. 16, 2020, 11:30 p.m. UTC | #8
On 11/16/20 3:27 PM, Vladimir Oltean wrote:
> On Mon, Nov 16, 2020 at 03:13:47PM -0800, Jakub Kicinski wrote:
>> On Tue, 17 Nov 2020 01:00:53 +0200 Vladimir Oltean wrote:
>>> On Mon, Nov 16, 2020 at 02:35:44PM -0800, Jakub Kicinski wrote:
>>>> On Tue, 17 Nov 2020 00:21:46 +0200 Vladimir Oltean wrote:
>>>>> On Mon, Nov 16, 2020 at 01:34:53PM -0800, Jakub Kicinski wrote:
>>>>>> You must expose relevant statistics via the normal get_stats64 NDO
>>>>>> before you start dumping free form stuff in ethtool -S.
>>>>>
>>>>> Completely agree on the point, Jakub, but to be honest we don't give him
>>>>> that possibility within the DSA framework today, see .ndo_get_stats64 in
>>>>> net/dsa/slave.c which returns the generic dev_get_tstats64 implementation,
>>>>> and not something that hooks into the hardware counters, or into the
>>>>> driver at all, for that matter.
>>>>
>>>> Simple matter of coding, right? I don't see a problem.
>>>>
>>>> Also I only mentioned .ndo_get_stats64, but now we also have stats in
>>>> ethtool->get_pause_stats.
>>>
>>> Yes, sure we can do that. The pause stats and packet counter ops would
>>> need to be exposed to the drivers by DSA first, though. Not sure if this
>>> is something you expect Oleksij to do or if we could pick that up separately
>>> afterwards.
>>
>> Well, I feel like unless we draw the line nobody will have
>> the incentive to do the work.
>>
>> I don't mind if it's Oleksij or anyone else doing the plumbing work,
>> but the task itself seems rather trivial.
> 
> So then I'll let Oleksij show his availability.
> 
>>>>> But it's good that you raise the point, I was thinking too that we
>>>>> should do better in terms of keeping the software counters in sync with
>>>>> the hardware. But what would be a good reference for keeping statistics
>>>>> on an offloaded interface? Is it ok to just populate the netdev counters
>>>>> based on the hardware statistics?
>>>>
>>>> IIRC the stats on the interface should be a sum of forwarded in software
>>>> and in hardware. Which in practice means interface HW stats are okay,
>>>> given eventually both forwarding types end up in the HW interface
>>>> (/MAC block).
>>>
>>> A sum? Wouldn't that count the packets sent/received by the stack twice?
>>
>> Note that I said _forwarded_. Frames are either forwarded by the HW or
>> SW (former never hit the CPU, while the latter do hit the CPU or
>> originate from it).
> 
> Ah, you were just thinking out loud, I really did not understand what
> you meant by the separation between "forwarded in software" and
> "forwarded in hardware".
> Yes, the hardware typically only gives us MAC-level counters anyway.
> Another way to look at it is that the number of packets forwarded in
> hardware from a given port are equal to the total number of RX packets
> on that MAC minus the packets seen by the CPU coming from that port.
> So all in all, it's the MAC-level counters we should expose in
> .ndo_get_stats64, I'm glad you agree. As for the error packets, I
> suppose that would be a driver-specific aggregate.
> 
> What about RMON/RFC2819 style etherStatsPkts65to127Octets? We have a
> number of switches supporting that style of counters, including the one
> that Oleksij is adding support for, apparently (but not all switches
> though). I suppose your M.O. is that anything standardizable is welcome
> to be standardized via rtnetlink?
> 
> Andrew, Florian, any opinions here?
> 

Settling on RMON/RFC2819 statistics would work for me, and hopefully is
not too hard to get the various drivers converted to. With respect to
Oleksij's patch, I would tend to accept it so we actually have more
visibility into what standardized counters are available across switch
drivers.
Jakub Kicinski Nov. 16, 2020, 11:56 p.m. UTC | #9
On Tue, 17 Nov 2020 01:27:31 +0200 Vladimir Oltean wrote:
> > Note that I said _forwarded_. Frames are either forwarded by the HW or
> > SW (former never hit the CPU, while the latter do hit the CPU or
> > originate from it).  
> 
> Ah, you were just thinking out loud, I really did not understand what
> you meant by the separation between "forwarded in software" and
> "forwarded in hardware".
> Yes, the hardware typically only gives us MAC-level counters anyway.
> Another way to look at it is that the number of packets forwarded in
> hardware from a given port are equal to the total number of RX packets
> on that MAC minus the packets seen by the CPU coming from that port.
> So all in all, it's the MAC-level counters we should expose in
> .ndo_get_stats64, I'm glad you agree. As for the error packets, I
> suppose that would be a driver-specific aggregate.

Yup, sorry about the confusion, I was only working on those stats
with SDN/OvS/tc hardware, which explains the slight difference in
terminology.
Jakub Kicinski Nov. 17, 2020, 12:02 a.m. UTC | #10
On Mon, 16 Nov 2020 15:30:39 -0800 Florian Fainelli wrote:
> > What about RMON/RFC2819 style etherStatsPkts65to127Octets? We have a
> > number of switches supporting that style of counters, including the one
> > that Oleksij is adding support for, apparently (but not all switches
> > though). I suppose your M.O. is that anything standardizable is welcome
> > to be standardized via rtnetlink?
> > 
> > Andrew, Florian, any opinions here?
> 
> Settling on RMON/RFC2819 statistics would work for me, and hopefully is
> not too hard to get the various drivers converted to.

That would be grate! For RMON stats you may have slightly more legwork
to do on ethtool side, since IIRC the plumbing for stats over netlink
is (intentionally?) not there until we settle on how to handle
standardize-able counters.

I've only done pause stats 'cause those and FEC are the primary HW
stats my employer cares about :) I'm sure people would actually find
use for the RMON stats once they get standardized tho!

> With respect to Oleksij's patch, I would tend to accept it so we
> actually have more visibility into what standardized counters are
> available across switch drivers.

For a while now we have been pushing back on stats which have a proper
interface to be added to ethtool -S. So I'd expect the list of stats
exposed via ethtool will end up being shorter than in this patch.
Vladimir Oltean Nov. 17, 2020, 12:10 a.m. UTC | #11
On Mon, Nov 16, 2020 at 04:02:13PM -0800, Jakub Kicinski wrote:
> For a while now we have been pushing back on stats which have a proper
> interface to be added to ethtool -S. So I'd expect the list of stats
> exposed via ethtool will end up being shorter than in this patch.

Hmm, not sure if that's ever going to be the case. Even with drivers
that are going to expose standardized forms of counters, I'm not sure
it's going to be nice to remove them from ethtool -S. Testing teams all
over the world have scripts that grep for those. Unfortunately I think
ethtool -S will always remain a dumping ground of hell, and the place
where you search for a counter based on its name from the hardware block
guide as opposed to its standardized name/function. And that might mean
there's no reason to not accept Oleksij's patch right away. Even if he
might volunteer to actually follow up with a patch where he exposes the
.ndo_get_stats64 from DSA towards drivers, as well as implements
.ndo_has_offload_stats and .ndo_get_offload_stats within DSA, that will
most likely be done as separate patches to this one, and not change in
any way how this patch looks.
Jakub Kicinski Nov. 17, 2020, 12:28 a.m. UTC | #12
On Tue, 17 Nov 2020 02:10:05 +0200 Vladimir Oltean wrote:
> On Mon, Nov 16, 2020 at 04:02:13PM -0800, Jakub Kicinski wrote:
> > For a while now we have been pushing back on stats which have a proper
> > interface to be added to ethtool -S. So I'd expect the list of stats
> > exposed via ethtool will end up being shorter than in this patch.  
> 
> Hmm, not sure if that's ever going to be the case. Even with drivers
> that are going to expose standardized forms of counters, I'm not sure
> it's going to be nice to remove them from ethtool -S.

Not remove, but also not accept adding them to new drivers.

> Testing teams all
> over the world have scripts that grep for those. Unfortunately I think
> ethtool -S will always remain a dumping ground of hell, and the place
> where you search for a counter based on its name from the hardware block
> guide as opposed to its standardized name/function. And that might mean
> there's no reason to not accept Oleksij's patch right away. Even if he
> might volunteer to actually follow up with a patch where he exposes the
> .ndo_get_stats64 from DSA towards drivers, as well as implements
> .ndo_has_offload_stats and .ndo_get_offload_stats within DSA, that will
> most likely be done as separate patches to this one, and not change in
> any way how this patch looks.
Oleksij Rempel Nov. 20, 2020, 12:59 p.m. UTC | #13
On Mon, Nov 16, 2020 at 04:28:44PM -0800, Jakub Kicinski wrote:
> On Tue, 17 Nov 2020 02:10:05 +0200 Vladimir Oltean wrote:
> > On Mon, Nov 16, 2020 at 04:02:13PM -0800, Jakub Kicinski wrote:
> > > For a while now we have been pushing back on stats which have a proper
> > > interface to be added to ethtool -S. So I'd expect the list of stats
> > > exposed via ethtool will end up being shorter than in this patch.  
> > 
> > Hmm, not sure if that's ever going to be the case. Even with drivers
> > that are going to expose standardized forms of counters, I'm not sure
> > it's going to be nice to remove them from ethtool -S.
> 
> Not remove, but also not accept adding them to new drivers.
> 
> > Testing teams all
> > over the world have scripts that grep for those. Unfortunately I think
> > ethtool -S will always remain a dumping ground of hell, and the place
> > where you search for a counter based on its name from the hardware block
> > guide as opposed to its standardized name/function. And that might mean
> > there's no reason to not accept Oleksij's patch right away. Even if he
> > might volunteer to actually follow up with a patch where he exposes the
> > .ndo_get_stats64 from DSA towards drivers, as well as implements
> > .ndo_has_offload_stats and .ndo_get_offload_stats within DSA, that will
> > most likely be done as separate patches to this one, and not change in
> > any way how this patch looks.

Ok, so what is the plan for me? Implement .ndo_get_stats64 before this
one?

Regards,
Oleksij
Jakub Kicinski Nov. 20, 2020, 3:33 p.m. UTC | #14
On Fri, 20 Nov 2020 13:59:07 +0100 Oleksij Rempel wrote:
> On Mon, Nov 16, 2020 at 04:28:44PM -0800, Jakub Kicinski wrote:
> > On Tue, 17 Nov 2020 02:10:05 +0200 Vladimir Oltean wrote:  
> > > On Mon, Nov 16, 2020 at 04:02:13PM -0800, Jakub Kicinski wrote:  
> > > > For a while now we have been pushing back on stats which have a proper
> > > > interface to be added to ethtool -S. So I'd expect the list of stats
> > > > exposed via ethtool will end up being shorter than in this patch.    
> > > 
> > > Hmm, not sure if that's ever going to be the case. Even with drivers
> > > that are going to expose standardized forms of counters, I'm not sure
> > > it's going to be nice to remove them from ethtool -S.  
> > 
> > Not remove, but also not accept adding them to new drivers.
> >   
> > > Testing teams all
> > > over the world have scripts that grep for those. Unfortunately I think
> > > ethtool -S will always remain a dumping ground of hell, and the place
> > > where you search for a counter based on its name from the hardware block
> > > guide as opposed to its standardized name/function. And that might mean
> > > there's no reason to not accept Oleksij's patch right away. Even if he
> > > might volunteer to actually follow up with a patch where he exposes the
> > > .ndo_get_stats64 from DSA towards drivers, as well as implements
> > > .ndo_has_offload_stats and .ndo_get_offload_stats within DSA, that will
> > > most likely be done as separate patches to this one, and not change in
> > > any way how this patch looks.  
> 
> Ok, so what is the plan for me? Implement .ndo_get_stats64 before this
> one?

Yup. And ethtool_ops::get_pause_stats, preferable 'cause I think you
had pause stats there, too.
diff mbox series

Patch

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index e24a99031b80..f6947fb0182f 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -101,6 +101,9 @@ 
 	 AR9331_SW_PORT_STATUS_RX_FLOW_EN | AR9331_SW_PORT_STATUS_TX_FLOW_EN | \
 	 AR9331_SW_PORT_STATUS_SPEED_M)
 
+/* MIB registers */
+#define AR9331_PORT_MIB_COUNTER(_i)			(0x20000 + (_i) * 0x100)
+
 /* Phy bypass mode
  * ------------------------------------------------------------------------
  * Bit:   | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 |10 |11 |12 |13 |14 |15 |
@@ -165,6 +168,61 @@  struct ar9331_sw_priv {
 	struct reset_control *sw_reset;
 };
 
+struct ar9331_mib_desc {
+	unsigned int size;
+	unsigned int offset;
+	const char *name;
+};
+
+#define MIB_DESC(_s, _o, _n)	\
+	{			\
+		.size = (_s),	\
+		.offset = (_o),	\
+		.name = (_n),	\
+	}
+
+static const struct ar9331_mib_desc ar9331_mib[] = {
+	MIB_DESC(1, 0x00, "RxBroad"),
+	MIB_DESC(1, 0x04, "RxPause"),
+	MIB_DESC(1, 0x08, "RxMulti"),
+	MIB_DESC(1, 0x0c, "RxFcsErr"),
+	MIB_DESC(1, 0x10, "RxAlignErr"),
+	MIB_DESC(1, 0x14, "RxRunt"),
+	MIB_DESC(1, 0x18, "RxFragment"),
+	MIB_DESC(1, 0x1c, "Rx64Byte"),
+	MIB_DESC(1, 0x20, "Rx128Byte"),
+	MIB_DESC(1, 0x24, "Rx256Byte"),
+	MIB_DESC(1, 0x28, "Rx512Byte"),
+	MIB_DESC(1, 0x2c, "Rx1024Byte"),
+	MIB_DESC(1, 0x30, "Rx1518Byte"),
+	MIB_DESC(1, 0x34, "RxMaxByte"),
+	MIB_DESC(1, 0x38, "RxTooLong"),
+	MIB_DESC(2, 0x3c, "RxGoodByte"),
+	MIB_DESC(2, 0x44, "RxBadByte"),
+	MIB_DESC(1, 0x4c, "RxOverFlow"),
+	MIB_DESC(1, 0x50, "Filtered"),
+	MIB_DESC(1, 0x54, "TxBroad"),
+	MIB_DESC(1, 0x58, "TxPause"),
+	MIB_DESC(1, 0x5c, "TxMulti"),
+	MIB_DESC(1, 0x60, "TxUnderRun"),
+	MIB_DESC(1, 0x64, "Tx64Byte"),
+	MIB_DESC(1, 0x68, "Tx128Byte"),
+	MIB_DESC(1, 0x6c, "Tx256Byte"),
+	MIB_DESC(1, 0x70, "Tx512Byte"),
+	MIB_DESC(1, 0x74, "Tx1024Byte"),
+	MIB_DESC(1, 0x78, "Tx1518Byte"),
+	MIB_DESC(1, 0x7c, "TxMaxByte"),
+	MIB_DESC(1, 0x80, "TxOverSize"),
+	MIB_DESC(2, 0x84, "TxByte"),
+	MIB_DESC(1, 0x8c, "TxCollision"),
+	MIB_DESC(1, 0x90, "TxAbortCol"),
+	MIB_DESC(1, 0x94, "TxMultiCol"),
+	MIB_DESC(1, 0x98, "TxSingleCol"),
+	MIB_DESC(1, 0x9c, "TxExcDefer"),
+	MIB_DESC(1, 0xa0, "TxDefer"),
+	MIB_DESC(1, 0xa4, "TxLateCol"),
+};
+
 /* Warning: switch reset will reset last AR9331_SW_MDIO_PHY_MODE_PAGE request
  * If some kind of optimization is used, the request should be repeated.
  */
@@ -475,6 +533,58 @@  static void ar9331_sw_phylink_mac_link_up(struct dsa_switch *ds, int port,
 		dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
 }
 
+static void ar9331_get_strings(struct dsa_switch *ds, int port, u32 stringset,
+			       uint8_t *data)
+{
+	int i;
+
+	if (stringset != ETH_SS_STATS)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(ar9331_mib); i++)
+		strncpy(data + i * ETH_GSTRING_LEN, ar9331_mib[i].name,
+			ETH_GSTRING_LEN);
+}
+
+static void ar9331_get_ethtool_stats(struct dsa_switch *ds, int port,
+				     uint64_t *data)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct regmap *regmap = priv->regmap;
+	const struct ar9331_mib_desc *mib;
+	u32 reg, i, stat;
+	u64 hi;
+	int ret;
+
+	for (i = 0; i < ARRAY_SIZE(ar9331_mib); i++) {
+		mib = &ar9331_mib[i];
+		reg = AR9331_PORT_MIB_COUNTER(port) + mib->offset;
+
+		ret = regmap_read(regmap, reg, &stat);
+		if (ret)
+			dev_err_ratelimited(priv->dev, "%s: %i\n", __func__,
+					    ret);
+		data[i] = stat;
+		if (mib->size == 2) {
+			ret = regmap_read(regmap, reg + 4, &stat);
+			if (ret)
+				dev_err_ratelimited(priv->dev, "%s: %i\n",
+						    __func__, ret);
+
+			hi = stat;
+			data[i] |= hi << 32;
+		}
+	}
+}
+
+static int ar9331_get_sset_count(struct dsa_switch *ds, int port, int sset)
+{
+	if (sset != ETH_SS_STATS)
+		return 0;
+
+	return ARRAY_SIZE(ar9331_mib);
+}
+
 static const struct dsa_switch_ops ar9331_sw_ops = {
 	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
 	.setup			= ar9331_sw_setup,
@@ -483,6 +593,9 @@  static const struct dsa_switch_ops ar9331_sw_ops = {
 	.phylink_mac_config	= ar9331_sw_phylink_mac_config,
 	.phylink_mac_link_down	= ar9331_sw_phylink_mac_link_down,
 	.phylink_mac_link_up	= ar9331_sw_phylink_mac_link_up,
+	.get_strings		= ar9331_get_strings,
+	.get_ethtool_stats	= ar9331_get_ethtool_stats,
+	.get_sset_count		= ar9331_get_sset_count,
 };
 
 static irqreturn_t ar9331_sw_irq(int irq, void *data)