netdev - Re: [PATCH v1 net-next 2/4] net: dsa: microchip: add MIB counter reading support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8ad1ec10-0f3f-9c66-a799-23df5a7842c0@gmail.com>
Date:   Thu, 14 Feb 2019 11:33:17 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Tristram.Ha@...rochip.com
Cc:     sergio.paracuellos@...il.com, pavel@....cz,
        UNGLinuxDriver@...rochip.com, netdev@...r.kernel.org,
        andrew@...n.ch
Subject: Re: [PATCH v1 net-next 2/4] net: dsa: microchip: add MIB counter
 reading support

On 2/14/19 11:26 AM, Tristram.Ha@...rochip.com wrote:
>>>>> +		/* read only dropped counters when link is not up */
>>>>> +		if (p->link_just_down)
>>>>> +			p->link_just_down = 0;
>>>>> +		else if (!p->phydev.link)
>>>>> +			mib->cnt_ptr = dev->reg_mib_cnt;
>>>>
>>>> This link_just_down stuff is not clear at all. Why can the drop
>>>> counters not be read when the link is up?
>>>
>>> All of the MIB counters, except some that may be marked by driver, do
>>> not get updated when the link is down, so it is a waste of time to read
>>> them.
>>
>> Can you use netif_running() to determine that condition? Maintaining your
>> own set of variables when the PHY state machine should already determine
>> the link state sounds redundant if not error prone.
>>
> 
> The driver can store the PHY device pointer passed to it when the port is enabled.  But I am a little worried that pointer can be changed or completely gone as it is out of control of the driver.

The per-port network device is accessible from dp->slave so you can do
netif_running(dp->slave) from your driver, what are you talking about here?

> 
>>> My intention is the driver eventually reads the MIB counters at least
>>> every second or faster so that the ethtool API called to show MIB
>>> counters gets them from memory rather than starting a read operation.
>>> For now that API is called from user space with the ethtool utility, so
>>> it may not be called too often and too fast.  But theoretically that
>>> API can be called from a program continually.
>>>
>>> For simple switches that do not need to do anything special the MIB
>>> read operation does not cause any issue except CPU load, for more
>>> complicate switches that need to do some background operations too many
>>> read operation can affect some critical functions.
>>
>> Some switches have a MIB autocast feature taking a snapshot which AFAIR is
>> internally implemented as a fast read register with no contention on other
>> registers internally, do you have something similar?
> 
> There is no such function in the switch.  Every MIB counter read has to go through a single SPI transfer using indirect access.  There are no table-like stored values that a single SPI transfer can retrieve all.
> 
> For i2C the access is even slower, but then I do not expect this access mechanism is used when the switch can do more complex things.
> 

Is that something you are considering to change for future designs?
-- 
Florian