netdev - Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

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: <20211108211110.4ad78e41@thinkpad>
Date:   Mon, 8 Nov 2021 21:11:10 +0100
From:   Marek Behún <kabel@...nel.org>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Ansuel Smith <ansuelsmth@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Jonathan Corbet <corbet@....net>, Pavel Machek <pavel@....cz>,
        John Crispin <john@...ozen.org>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-leds@...r.kernel.org
Subject: Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of
 triggers

On Mon, 8 Nov 2021 20:53:36 +0100
Andrew Lunn <andrew@...n.ch> wrote:

> > I guess I will have to work on this again ASAP or we will end up with
> > solution that I don't like.
> > 
> > Nonetheless, what is your opinion about offloading netdev trigger vs
> > introducing another trigger?  
> 
> It is a solution that fits the general pattern, do it in software, and
> offload it if possible.
> 
> However, i'm not sure the software solution actually works very well.
> At least for switches. The two DSA drivers which implement
> get_stats64() simply copy the cached statistics. The XRS700X updates
> its cached values every 3000ms. The ar9331 is the same. Those are the
> only two switch drivers which implement get_stats64 and none implement
> get_stats. There was also was an issue that get_stats64() cannot
> perform blocking calls. I don't remember if that was fixed, but if
> not, get_stats64() is going to be pretty useless on switches.
> 
> We also need to handle drivers which don't actually implement
> dev_get_stats(). That probably means only supporting offloads, all
> modes which cannot be offloaded need to be rejected. This is pretty
> much the same case of software control of the LEDs is not possible.
> Unfortunately, dev_get_stats() does not return -EOPNOTSUPP, you need
> to look at dev->netdev_ops->ndo_get_stats64 and
> dev->netdev_ops->ndo_get_stats.
> 
> Are you working on Marvell switches? Have you implemented
> get_stats64() for mv88e6xxx? How often do you poll the hardware for
> the stats?
> 
> Given this, i think we need to bias the API so that it very likely
> ends up offloading, if offloading is available.

I am working with Marvell PHYs and Marvell switches. I am aware of the
problem that SW netdev does not work for switches because we don't have
uptodate data.

It seems to me that yes, if the user wants to blink the LEDs on
activity on switch port, the netdev trigger should only work in offload
mode and only on LEDs that are connected to switch pins, unless we
implement a mechanism to get statistics at lest 10 times a second
(which could maybe be done on marvell switches by reading the registers
via ethernet frames instead of MDIO).

Marvell switches don't seem to support rx only / tx only activity
blinking, only rx/tx. I think this could be solved by making rx/tx
sysfs files for netdev trigger behave so that writing to one would also
write to another.

But since currently netdev trigger does not work for these switches, I
think making it so that it works at least to some fashion (in HW
supported modes) would still be better that current situation.

I will try to look into this, maybe work together with Ansuel.

Marek